Details

Reviewers

Sybren A. Stüvel (sybren)
Julian Eisel (Severin)

Summary

Primary reason for this refactor is to provide means to determine if image that is used is actually proxy image.
This is achieved by passing pointer of is_proxy_image to function where proxy is loaded.
Because VSE is using other subsystems to load files (clip editor and IMB_indexer), we must determine whether proxy or original has been served during image loading.

Prior to this commit VSE had weak system to check if loaded image is actually proxy
or fallback to full-size image. That caused problems on operations with absolute positioning.
See T54395, T51828

From function seq_proxy_get_fname() unused code has been removed and function has been split to seq_proxy_get_custom_file_fname() to get path to custom proxy file if used and seq_image_strip_proxy_get_fname() to get path to image strip proxy file.

Movie and image strip rendering functions has been refactored, so explicit proxy loading function exists.
in case of Movie clip (MCE), proxy is loaded by setting user.render_size without using MCLIP_PROXY_RENDER_USE_FALLBACK_RENDER flag.

Movie and image strip rendering functions has been refactored even more to get rid of goto statement that is used to render separated multiview images.
seq_render_image_strip_view() and seq_render_movie_strip_view() is used to render individual views, even monoview.

Diff Detail

Repository

rB Blender

Branch

arcpatch-D6786 (branched from master)

Build Status

Buildable 7528
Build 7528: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) created this revision.Feb 8 2020, 7:36 PM

Harbormaster completed remote builds in B6545: Diff 21532.Feb 8 2020, 7:37 PM

Richard Antalik (ISS) added a project: Video Sequencer.Feb 8 2020, 7:41 PM

Christopher_Anderssarian awarded a token.Feb 8 2020, 7:47 PM

@Christopher_Anderssarian Now when I see you here, I have a suspicion, that this may break multiview proxies... I wanted to test this case as well, but I can not get it to work...
Is it completely broken? Or could you share some simple multiview VSE project so I can test this?

VSE: Remove atomized image duplication for preprocessing stage

Harbormaster completed remote builds in B6547: Diff 21535.Feb 8 2020, 7:56 PM

Oops last commit was meant to be new revision, but I guess I can leave it here.

Revert to original diff, these two changes really shouldn't be together...

Christopher_Anderssarian mentioned this in T73689: VSE's proxy doesn't show up anymore.Feb 9 2020, 2:54 PM

Does this make https://developer.blender.org/D6314 unnecessary.

In D6786#160792, @Eitan Traurig (EitanSomething) wrote:

Does this make https://developer.blender.org/D6314 unnecessary.

No, but it can be simplified to ommit filepath trickery.
You can 100% rely on is_proxy_image.

The token was a reaction to seeing that you added return true; for that "unsupported strip type";)
Here's a ~~couple of~~ working 3D image proxy strip .blend:

D6786 VSE Refactor proxy loading (2.79b) 3D image proxy test.zip216 KBDownload

Your not planning on supporting 3D scence stip proxies any time soon, are you? (pretty please...)

fix some problems
remove atomic cache usage - there was bug, but more importantly, it is useless
Fix proxy usage in image pipeline.
Remove unused vars

As I thought, multiview was broken with this patch. Thanks @Christopher_Anderssarian for sample project.

Harbormaster completed remote builds in B6613: Diff 21695.Feb 13 2020, 11:51 PM

Please extend the patch description; take a look at Ingredients of a Patch. Currently the patch is presented as a "refactor", but it also changes how proxy images are handled, so it's more than just refactoring existing code.

I stopped reviewing when I saw the seq_render_image_strip() function. IMO this function should be refactored before allowing any further change to it. At its heart is this construct:

ImBuf *ibuf = NULL;
if (!s_elem) {
  /* don't do anything */
}
else if (is_multiview) {
  …
  if (/* some condition */) {
    …
    if (/* some condition */) {
        goto monoview_image;
    }
  }
  …
}
else {
  monoview_image:
  …
}
return ibuf;

The if (!s_elem) { /* don't do anything */ } can just be replaced with if (!s_elem) { return NULL; }. But that's not the most horrible. What I really find unacceptable is using goto to jump from an if-in-if-in-if body to the else clause of the outer if. I won't approve any patch that adds things on top of this monstrosity. I know this is harsh, and it's going beyond what is in your patch. However, I do feel that this particular function is already too complex, and that it's not a good idea to add even more complexity on top of it.

source/blender/blenkernel/intern/sequencer.c
1662	`SEQ_EDIT_PROXY_DIR_STORAGE` is renamed to `SEQ_EDIT_PROXY_STORAGE_PER_PROJECT`. Is this a semantic change, i.e. does the constant actually mean something different now? Or is it just a clarification of the name? If it is the latter, I'd keep this change as a separate cleanup commit.
1782	Why can all that deleted code be replaced by these two lines?
2178	This comment is unclear. Starting it with "Only use proxies when" (so no comma, and 'if' → 'when') would be an improvement. The "(even if present)" clause I don't understand. Maybe "Only use proxies when present and enabled" is the right wording?
2603	Code style: Comments should be a sentence, so start with capital and end in period. Also applies to other comments.
2929–2938	Code style: write preconditions at the start of the function, so like if (!seq_proxy_get_fname(ed, seq, cfra, context->preview_render_size, name, context->view_id)) { return NULL; } if (!BLI_exists(name)) { return NULL; } …

This revision now requires changes to proceed.Feb 21 2020, 2:15 PM

Sybren A. Stüvel (sybren) added inline comments.Feb 21 2020, 2:33 PM

source/blender/blenkernel/intern/sequencer.c
2181	Explicit pointer comparisons are preferred, so `if (seq->strip->proxy == NULL) …` Also see https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Pointer_Comparisons

@Sybren A. Stüvel (sybren) I agree with your goto rejection.
I have refactored seq_render_image_strip(), same for seq_render_movie_strip() in old uncommited patch.
Later I was looking for this goto use-case and apparently it is 'acceptable' equivalent to finally: statement, so I thought that perhaps this was thought through...

paul szajner (szap) awarded a token.Mar 2 2020, 6:49 PM

paul szajner (szap) added a subscriber: paul szajner (szap).Mar 4 2020, 9:09 AM

refactor movie and image loading even more
Cleanup - comments
Style: explicit pointer comparison

Got a bit distracted, so updating a bit late, but I guess better than never.
There are some unintended changes, I suspect MSVC is again refusing to use clang-format... Must look into it during weekend, can not get it to work now.

Harbormaster completed remote builds in B7003: Diff 22486.Mar 6 2020, 7:08 PM

MSVC is not saving my files...

Harbormaster completed remote builds in B7005: Diff 22488.Mar 6 2020, 7:16 PM

Richard Antalik (ISS) added inline comments.Mar 6 2020, 7:16 PM

source/blender/blenkernel/intern/sequencer.c
1782	This looks like diff isn't optimal here.. `seq_proxy_get_fname` is not replaced by `seq_proxy_get_custom_file_fname`, it is shifted to line 1806. Though some duplicate (movie proxy filename) and historic (scene proxy filename) code has been removed.

As a reminder, this patch fix a bug in master. Currently proxy are broken and doesn't show up in the VSE : https://developer.blender.org/T73689
In 2.82 it's already the case, it would be great to have them back in master as it's quite helpful for having an appropriate playback for editing.
Thanks !

In D6786#163833, @Sybren A. Stüvel (sybren) wrote:

Please extend the patch description; take a look at Ingredients of a Patch. Currently the patch is presented as a "refactor", but it also changes how proxy images are handled, so it's more than just refactoring existing code.

This remark is still valid. This patch changes a lot of code, and I really need a better context to understand what's going on. Currently the only part of the patch description that is about the actual implementation is:

is_proxy_image var is passed to function where proxy is loaded and set accordingly.

There seems to be much more going on than just adding a variable and passing it to a function.

source/blender/blenkernel/intern/sequencer.c
2185	Use parentheses when mixing logical and bitwise operators, and either explicitly compare to 0 or do not. So either one of these two: return (seq->flag & SEQ_USE_PROXY) && psize != IMB_PROXY_NONE && (size_flags & psize); return (seq->flag & SEQ_USE_PROXY) != 0 && psize != IMB_PROXY_NONE && (size_flags & psize) != 0;
3019	`is_proxy_image` → `r_is_proxy_image`
3021–3023	Declare this variable just before it's used. Right now there is too much separation between "this should be multiview" and "oops, it's multiview with just one file, so let's treat it as non-multiview".
3025	Move this to underneath the `if (s_elem == NULL)` block, so that `seq_rendersize_to_proxysize()` is only called when relevant.
3036–3039	What does "first" mean here? Before what?
3042–3046	It's probably better to move the "Is this multiview? Is it reaaally multiview?" code into a separate function.
3147	`is_proxy_image` → `r_is_proxy_image`
3161	Flip the condition so that you can simply do if (ibuf != NULL) { return NULL; } and un-indent the rest.
3165	`is_proxy_image` → `r_is_proxy_image`
3176–3179	The "the flags say this should be multiview, but is it reaaaaly?" test here is different than above. Why is that?
3222–3226	I would flip the condition and just do if (buff == NULL) { return NULL; } seq->strip->stripdata->orig_width = ibuf->x; seq->strip->stripdata->orig_height = ibuf->y; return ibuf; There is no need to `return ibuf` if you already know that it is `NULL`; being explicit is IMO better in such a case. Same for similar constructs elsewhere in this patch.
3244	`is_proxy_image` → `r_is_proxy_image` ... and same for following declarations.
3281–3285	This comment is ambiguous. "proxy" reads as a verb, but I have a hunch it's used as a noun here. Writing a full sentence would help.
3289	`if (ibuf == NULL)`
3793	This is a linguistic mixup. In `use_preprocess` the word "preprocess" is a noun. In `have_to_preprocess` the same word is a verb. Avoid having ambiguous semantics. I would suggest replacing `have_to_preprocess` and `use_preprocess` by `must_preprocess`.
3797–3798	Maybe as a separate cleanup commit, flip the condition if (ibuf != NULL) { return ibuf; } and unindent the rest of the function.
4620–4621	Keep reformatting as a separate commit.
5656–5657	Keep reformatting as a separate commit.

Sybren A. Stüvel (sybren) requested changes to this revision.Mar 12 2020, 3:21 PM

This revision now requires changes to proceed.Mar 12 2020, 3:21 PM

address inlines
use better names for functions

Harbormaster completed remote builds in B7303: Diff 23112.Mar 26 2020, 2:33 PM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Mar 26 2020, 2:33 PM

Richard Antalik (ISS) added inline comments.

source/blender/blenkernel/intern/sequencer.c
3176–3179	Honestly, I have no idea. I had to ask for sample multiview project to test functionality, because I wasn't able to set it up myself (Technically I knew that Christopher_Anderssarian will probably have one so I haven't tried too much.) One thing I know is that to use `R_IMF_VIEWS_STEREO_3D`, 1 image will contain left and right view, but on the other hand you need 2 separate movies. So there will be differences. And it works, so I haven't touched that.

Fix warnings

Harbormaster completed remote builds in B7528: Diff 23606.Apr 9 2020, 9:59 PM

Pipeliner (Pipeliner) awarded a token.Apr 14 2020, 10:14 PM

Is there enough of non-funcitonal changes which can just go straight to master, making the rest of the patch review easier?

In D6786#178217, @Sergey Sharybin (sergey) wrote:

Is there enough of non-funcitonal changes which can just go straight to master, making the rest of the patch review easier?

Yes, I have discussed this yesterday briefly on chat, that best thing to do will probably be split this to bugfixes(for 2.83), and few smaller patches for 2.9.

In the Sequencer, a red line is drawn on missing media, but there is no flag for missing proxy files and therefore this indication isn't triggered. Maybe this flag could be added to this patch?

Here's an example of the hoops to go through to check if it a proxy is loaded/existing: https://developer.blender.org/T73689

Christopher_Anderssarian mentioned this in T54259: Scene Proxies in the Video Sequence Editor.May 19 2020, 3:25 PM

Will close this and break up the patch

VSE: Refactor proxy loading
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 23606

source/blender/blenkernel/BKE_sequencer.h

source/blender/blenkernel/intern/sequencer.c

VSE: Refactor proxy loadingAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 23606

source/blender/blenkernel/BKE_sequencer.h

source/blender/blenkernel/intern/sequencer.c

VSE: Refactor proxy loading
AbandonedPublic
Actions

Revision Contents
Changeset List