Page MenuHome

Cleanup: Remove goto statements from strip rendering functions
ClosedPublic

Authored by Richard Antalik (ISS) on May 29 2020, 12:56 AM.

Details

Summary

Remove goto statement from seq_render_image_strip() and seq_render_movie_strip().
seq_render_image_strip_view() and seq_render_movie_strip_view() is used to render individual views in both monoview and multiview branch.

I have included seq_can_use_proxy() for convinience

Diff Detail

Repository
rB Blender
Branch
arcpatch-D7870 (branched from master)
Build Status
Buildable 8392
Build 8392: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) requested review of this revision.May 29 2020, 12:56 AM
Richard Antalik (ISS) created this revision.
  • Fix variable name
  • Remove goto from seq_render_image_strip
Richard Antalik (ISS) retitled this revision from Cleanup: Remove goto statements from rendering functions to Cleanup: Remove goto statements from strip rendering functions.May 29 2020, 2:24 AM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
Sybren A. Stüvel (sybren) requested changes to this revision.Jun 2 2020, 3:21 PM

Good work :)

source/blender/blenkernel/intern/sequencer.c
3041–3042

Maybe move this line to directly above the if (s_elem == NULL) { function, making their relation clearer.

3074

prefix and ext seem to be return parameters, so need an r_ prefix.

3080

Here you can just return false;
The check on totfiles is also independent of the bool is_multiview line above it, so by moving the if-statement one line up, things get even clearer:

{
  if (totfiles > 1) {
    BKE_scene_multiview_view_prefix_get(scene, name, prefix, &ext);
    if (prefix[0] == '\0') {
      return false;
    }
  }
  else {
    prefix[0] = '\0';
  }

  bool is_multiview = (seq->flag & SEQ_USE_VIEWS) != 0 && (scene->r.scemode & R_MULTIVIEW) != 0;
  return is_multiview;
}

(or remove the bool is_multiview variable altogether, also fine by me)

3084

I'm a bit puzzled by the role of prefix and the rest of the logic here. It would seem that the returned value can be true even when totfiles == 1, and it can then also have prefix = "".

This is probably my lack of knowledge about the whole multiview system, though. Is it correct that you can have multi-view with an empty prefix?

The code seems to be semantically equivalent to the old code, though.

3158

Now there are two very similarly named functions, seq_render_movie_strip and seq_render_movie_strip_view. I think a comment is in order to explain what exactly this function is doing, i.e. how "view" in the name relates to "monoview" or "multiview".

3177–3182

This can be condensed even more into:

bool is_multiview_render = (seq->flag & SEQ_USE_VIEWS) != 0 &&
                           (context->scene->r.scemode & R_MULTIVIEW) != 0 &&
                           BLI_listbase_count_at_most(&seq->anims, totfiles + 1) == totfiles;

This probably should be its own function for clarity. It can then be placed next to seq_image_strip_is_multiview_render() so that it's clearer how these two functions differ, even though they basically try to express the same thing. I'm fine if you do that in a separate commit, though, as the way it is now is easier to review.

3229–3237

This is a functional change. Previously the NULL-check was only done when seq->views_format == R_IMF_VIEWS_STEREO_3D. Even though the new code may actually do the correct thing, separate functional from non-functional changes into separate patches. Or add a note to explain how this is a non-functional change after all ;-)

This revision now requires changes to proceed.Jun 2 2020, 3:21 PM
Richard Antalik (ISS) marked 6 inline comments as done.
  • Address inlines

LGTM. I haven't gone over all the changes with a toothpick; I'm assuming you've gone over tests yourself to ensure the behaviour is still the same.

This revision is now accepted and ready to land.Jun 8 2020, 3:02 PM