blender/blender
148
473
Fork 693
Code Issues 6.7k Pull Requests 908 Projects 18 Wiki Activity

VSE: Skip rendering lower strips that are behind opaque strips above them #118396

Merged
Aras Pranckevicius merged 6 commits from aras_p/blender:vse_strip_occlusion into main 2024-02-21 20:16:53 +01:00
Conversation 16 Commits 6 Files Changed 3 +114 -33
Aras Pranckevicius commented 2024-02-16 21:25:43 +01:00
Member
Copy Link

Often in previs setups, you have several "variations" of image/movie strips for review, where the topmost one completely covers the others under it. VSE rendering already had an optimization where if there's a fully opaque strip that covers the whole screen, then the lower strips are skipped from processing/rendering. However, it was not handling the case of non-fullscreen strips (e.g. you'd have a Color strip near top & bottom for letterboxing, and an opaque strip "in the middle").

This adds a simple "occluder tracking", and skips strips that are completely covered by any single opaque strip that is above it (like outlined in #117790 task).

Playback of Gold previs between 1:42 and 1:55, on Windows/Ryzen5950X:

  • Average frame time 28.5ms -> 23.8ms
  • Median frame time 24.1ms -> 21.5ms
  • Two slowest frames: 263->189ms, 194->178ms

Rendering the Gold previs movie: 325s -> 304s (93% of previous time)

Often in previs setups, you have several "variations" of image/movie strips for review, where the topmost one completely covers the others under it. VSE rendering already had an optimization where if there's a fully opaque strip that covers the whole screen, then the lower strips are skipped from processing/rendering. However, it was not handling the case of non-fullscreen strips (e.g. you'd have a Color strip near top & bottom for letterboxing, and an opaque strip "in the middle"). This adds a simple "occluder tracking", and skips strips that are completely covered by any single opaque strip that is above it (like outlined in #117790 task). Playback of Gold previs between 1:42 and 1:55, on Windows/Ryzen5950X: - Average frame time 28.5ms -> 23.8ms - Median frame time 24.1ms -> 21.5ms - Two slowest frames: 263->189ms, 194->178ms Rendering the Gold previs movie: 325s -> 304s (93% of previous time)
Aras Pranckevicius added 1 commit 2024-02-16 21:25:53 +01:00
VSE: Skip rendering lower strips that are behind opaque strips above them
All checks were successful
buildbot/vexp-code-patch-lint Build done.
Details
buildbot/vexp-code-patch-linux-x86_64 Build done.
Details
buildbot/vexp-code-patch-windows-amd64 Build done.
Details
buildbot/vexp-code-patch-darwin-arm64 Build done.
Details
buildbot/vexp-code-patch-darwin-x86_64 Build done.
Details
buildbot/vexp-code-patch-coordinator Build done.
Details
362b5a3f00 
Often in previs setups, you have several "variations" of image/movie
strips for review, where the topmost one completely covers the others
under it. VSE rendering alreayd had an optimization where if there's
a fully opaque strip that covers the whole screen, then the lower
strips are skipped from processing/rendering. However, it was not
handling the case of non-fullscreen strips (e.g. you'd have a Color
strip near top & bottom for letterboxing, and an opaque strip "in the
middle").

This adds a simple "occluder tracking", and skips strips that are
completely covered by any single opaque strip that is above it.

Playback of Gold previs between 1:42 and 1:55, on Windows/Ryzen5950X:
- Average frame time 28.5ms -> 23.8ms
- Median frame time 24.1ms -> 21.5ms
- Two slowest frames: 263->189ms, 194->178ms

Rendering the Gold previs movie: 325s -> 304s (93% of previous time)
Aras Pranckevicius commented 2024-02-16 21:26:08 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Aras Pranckevicius added this to the Video Sequencer project 2024-02-16 21:27:13 +01:00
Aras Pranckevicius requested review from Richard Antalik 2024-02-16 21:58:06 +01:00
Richard Antalik commented 2024-02-18 18:43:53 +01:00
Member
Copy Link

Just checked simple case with color strip covering movie strip image, and this doesn't seem to prevent postprocessing of the movie strip.

The ImBuf *test = seq_render_strip(context, state, seq, timeline_frame); does complete strip render, so you would have to stop postprocessing from within the pipeline. But it also doesn't seem to stop alpha over blending of occluded strips.

Did not check the code in detail, unfortunately the pipeline is ugly, so I will have to prepare for my head hurting a bit here :/

You could simplify the code a tiny bit by using ibuf = seq_cache_get(..., SEQ_CACHE_STORE_RAW) to get ImBuf::planes instead of passing this data via SeqRenderState::planes_before_pp. The seq_render_preprocess_ibuf() would have to be modified a bit to store images from 0-input effects in cache for seq_cache_put(..., SEQ_CACHE_STORE_RAW).

Personally I would remove SeqRenderState completely, since I already do have better mechanism to prevent infinite render loops in place...

Just checked simple case with color strip covering movie strip image, and this doesn't seem to prevent postprocessing of the movie strip. The `ImBuf *test = seq_render_strip(context, state, seq, timeline_frame);` does complete strip render, so you would have to stop postprocessing from within the pipeline. But it also doesn't seem to stop alpha over blending of occluded strips. Did not check the code in detail, unfortunately the pipeline is ugly, so I will have to prepare for my head hurting a bit here :/ You could simplify the code a tiny bit by using `ibuf = seq_cache_get(..., SEQ_CACHE_STORE_RAW)` to get `ImBuf::planes` instead of passing this data via `SeqRenderState::planes_before_pp`. The `seq_render_preprocess_ibuf()` would have to be modified a bit to store images from 0-input effects in cache for `seq_cache_put(..., SEQ_CACHE_STORE_RAW)`. Personally I would remove `SeqRenderState` completely, since I already do have better mechanism to prevent infinite render loops in place...
Aras Pranckevicius commented 2024-02-18 20:16:25 +01:00
Author
Member
Copy Link

Just checked simple case with color strip covering movie strip image, and this doesn't seem to prevent postprocessing of the movie strip.

Hmm, can't reproduce that. If I have a color strip that completely covers the movie strip, then the seq_render_strip_stack does a single seq_render_strip which goes into do_solid_color. When the color strip only partially covers the movie strip (or on main branch), the work that seq_render_strip_stack does ends up being: seq_render_strip for color (does solid color and preprocess), plus seq_render_strip for movie (does all the movie strip image render, plus preprocess), plus seq_render_strip_stack_apply_effect to blend the two.

Did not check the code in detail, unfortunately the pipeline is ugly, so I will have to prepare for my head hurting a bit here :/

Yeah to be fair I don't quite understand the code flow in there either. It does two loops over visible strips, once from top to bottom, then from bottom to top again. And the loops do different things. Why, it's not entirely clear.

You could simplify the code a tiny bit by using ibuf = seq_cache_get(..., SEQ_CACHE_STORE_RAW) to get ImBuf::planes instead of passing this data via SeqRenderState::planes_before_pp. The seq_render_preprocess_ibuf() would have to be modified a bit to store images from 0-input effects in cache for seq_cache_put(..., SEQ_CACHE_STORE_RAW).

Personally I would remove SeqRenderState completely, since I already do have better mechanism to prevent infinite render loops in place...

I'll look at that.

> Just checked simple case with color strip covering movie strip image, and this doesn't seem to prevent postprocessing of the movie strip. Hmm, can't reproduce that. If I have a color strip that completely covers the movie strip, then the `seq_render_strip_stack` does a single `seq_render_strip` which goes into `do_solid_color`. When the color strip only partially covers the movie strip (or on main branch), the work that `seq_render_strip_stack` does ends up being: `seq_render_strip` for color (does solid color and preprocess), plus `seq_render_strip` for movie (does all the movie strip image render, plus preprocess), plus `seq_render_strip_stack_apply_effect` to blend the two. > Did not check the code in detail, unfortunately the pipeline is ugly, so I will have to prepare for my head hurting a bit here :/ Yeah to be fair I don't quite understand the code flow in there either. It does two loops over visible strips, once from top to bottom, then from bottom to top again. And the loops do different things. Why, it's not entirely clear. > You could simplify the code a tiny bit by using `ibuf = seq_cache_get(..., SEQ_CACHE_STORE_RAW)` to get `ImBuf::planes` instead of passing this data via `SeqRenderState::planes_before_pp`. The `seq_render_preprocess_ibuf()` would have to be modified a bit to store images from 0-input effects in cache for `seq_cache_put(..., SEQ_CACHE_STORE_RAW)`. > > Personally I would remove `SeqRenderState` completely, since I already do have better mechanism to prevent infinite render loops in place... I'll look at that.
Aras Pranckevicius added 2 commits 2024-02-18 20:40:07 +01:00
VSE: Simplify occlusion checking code based on review suggestion
Some checks failed
buildbot/vexp-code-patch-lint Build done.
Details
buildbot/vexp-code-patch-linux-x86_64 Build done.
Details
buildbot/vexp-code-patch-darwin-x86_64 Build done.
Details
buildbot/vexp-code-patch-darwin-arm64 Build done.
Details
buildbot/vexp-code-patch-windows-amd64 Build done.
Details
buildbot/vexp-code-patch-coordinator Build done.
Details
63d0f100c8 
Instead of tracking via separate planes_before_pp map, look at the
SEQ_CACHE_STORE_RAW cached image.
Aras Pranckevicius commented 2024-02-18 20:42:11 +01:00
Author
Member
Copy Link

@iss changed code to use SEQ_CACHE_STORE_RAW image for determining "was this opaque" instead of a separate planes_before_pp map, indeed it works well and is simpler. Made "generator" (zero input) effects put raw image into the cache like you suggested.

@iss changed code to use `SEQ_CACHE_STORE_RAW` image for determining "was this opaque" instead of a separate `planes_before_pp` map, indeed it works well and is simpler. Made "generator" (zero input) effects put raw image into the cache like you suggested.
Aras Pranckevicius commented 2024-02-18 20:42:24 +01:00
Author
Member
Copy Link

@blender-bot build

@blender-bot build
Richard Antalik commented 2024-02-19 21:15:50 +01:00
Member
Copy Link

I still not see this actually working... Granted I am not testing this with Gold edit as you mention, but rather very simple setup.
Here it says, that movie is not occluded.

See video for FPS. Also attached .blend file that uses 1920x1080 video.

When I wanted to step through is_occluded() I get segfault in line_point_side_v2(). I stepped through the code for unrelated reason, and noticed that it takes different path now. Not sure what the issue here is...

I have added debug print

diff --git a/source/blender/sequencer/intern/render.cc b/source/blender/sequencer/intern/render.cc
index 283b4d4d7a7..c44ad7cb912 100644
--- a/source/blender/sequencer/intern/render.cc
+++ b/source/blender/sequencer/intern/render.cc
@@ -1988,6 +1988,10 @@ static ImBuf *seq_render_strip_stack(const SeqRenderData *context,
       }
     }
 
+    if (seq->type == SEQ_TYPE_MOVIE) {
+      printf("got %d early out\n", int(early_out));
+    }
+
     switch (early_out) {
       case StripEarlyOut::NoInput:
       case StripEarlyOut::UseInput2:

And got output for occluded part:

got 0 early out
got 0 early out
got 1 early out
got 1 early out
got 0 early out
got 1 early out
got 1 early out
got 0 early out
got 1 early out
got 1 early out
got 0 early out
got 1 early out
got 0 early out

Seems like uninitialized memory or buffer overread somewhere. But ASAN is silent... Shame, the file no longer crashes for me.

Only now I realized, that you are using get_strip_screen_quad() which gets quad from stored metadata. I think, that this is OK thing to do, but these values are not guaranteed to be initialized. I wouldn't complicate matters here, IMO this is responsibility of get_strip_screen_quad() itself, even though it makes the code even worse...

Now to comment on why seq_render_strip_stack() does first iterate from top down and then it reverses: In descending loop it looks for first strip that produces image on it's own - out. As soon as it gets it, it then goes in ascending order and blends images together. Perhaps this could be improved, since out is found by pretty much brute forcing.

I still not see this actually working... Granted I am not testing this with Gold edit as you mention, but rather very simple setup. Here it says, that movie is not occluded. See video for FPS. Also attached .blend file that uses 1920x1080 video. <video src="/attachments/d0e70480-359a-4fac-bdb9-16b7c6fcda16" title="2024-02-19 20-35-42.mkv" controls></video> When I wanted to step through `is_occluded()` I get segfault in `line_point_side_v2()`. I stepped through the code for unrelated reason, and noticed that it takes different path now. Not sure what the issue here is... I have added debug print ``` diff --git a/source/blender/sequencer/intern/render.cc b/source/blender/sequencer/intern/render.cc index 283b4d4d7a7..c44ad7cb912 100644 --- a/source/blender/sequencer/intern/render.cc +++ b/source/blender/sequencer/intern/render.cc @@ -1988,6 +1988,10 @@ static ImBuf *seq_render_strip_stack(const SeqRenderData *context, } } + if (seq->type == SEQ_TYPE_MOVIE) { + printf("got %d early out\n", int(early_out)); + } + switch (early_out) { case StripEarlyOut::NoInput: case StripEarlyOut::UseInput2: ``` And got output for occluded part: ``` got 0 early out got 0 early out got 1 early out got 1 early out got 0 early out got 1 early out got 1 early out got 0 early out got 1 early out got 1 early out got 0 early out got 1 early out got 0 early out ``` Seems like uninitialized memory or buffer overread somewhere. But ASAN is silent... Shame, the file no longer crashes for me. Only now I realized, that you are using `get_strip_screen_quad()` which gets quad from stored metadata. I think, that this is OK thing to do, but these values are not guaranteed to be initialized. I wouldn't complicate matters here, IMO this is responsibility of `get_strip_screen_quad()` itself, even though it makes the code even worse... Now to comment on why `seq_render_strip_stack()` does first iterate from top down and then it reverses: In descending loop it looks for first strip that produces image on it's own - `out`. As soon as it gets it, it then goes in ascending order and blends images together. Perhaps this could be improved, since `out` is found by pretty much brute forcing.
untitled.blend
664 KiB
2024-02-19 20-35-42.mkv
2.0 MiB
Aras Pranckevicius commented 2024-02-20 08:51:17 +01:00
Author
Member
Copy Link

I still not see this actually working... Granted I am not testing this with Gold edit as you mention, but rather very simple setup.
Here it says, that movie is not occluded.

Huh, I can't reproduce this at all. And yes I've tried simple setups as well. Now I tried your file, with your patch that adds a print, and it, as expected, prints "got 0 early out" for the non-occluded timeline portion, and "got 1 early out" for the occluded one. Tested both on Mac (clang 15) and Windows (vs2022). So I'm a bit at loss why you see what you see. Broken build for some reason?

Only now I realized, that you are using get_strip_screen_quad() which gets quad from stored metadata.
I think, that this is OK thing to do, but these values are not guaranteed to be initialized. I wouldn't complicate
matters here, IMO this is responsibility of get_strip_screen_quad() itself, even though it makes the code
even worse...

I'm using exact same data that already existing "is the strip covering whole screen?" alpha-over optimization was using. So if that data might be missing or bogus, then the already existing optimization would not have worked either.

> I still not see this actually working... Granted I am not testing this with Gold edit as you mention, but rather very simple setup. > Here it says, that movie is not occluded. Huh, I can't reproduce this at all. And yes I've tried simple setups as well. Now I tried your file, with your patch that adds a print, and it, as expected, prints "got 0 early out" for the non-occluded timeline portion, and "got 1 early out" for the occluded one. Tested both on Mac (clang 15) and Windows (vs2022). So I'm a bit at loss why you see what you see. Broken build for some reason? > Only now I realized, that you are using `get_strip_screen_quad()` which gets quad from stored metadata. > I think, that this is OK thing to do, but these values are not guaranteed to be initialized. I wouldn't complicate > matters here, IMO this is responsibility of `get_strip_screen_quad()` itself, even though it makes the code > even worse... I'm using exact same data that already existing "is the strip covering whole screen?" alpha-over optimization was using. So if that data might be missing or bogus, then the already existing optimization would not have worked either.
Richard Antalik commented 2024-02-20 17:06:10 +01:00
Member
Copy Link

Huh, I can't reproduce this at all. And yes I've tried simple setups as well. Now I tried your file, with your patch that adds a print, and it, as expected, prints "got 0 early out" for the non-occluded timeline portion, and "got 1 early out" for the occluded one. Tested both on Mac (clang 15) and Windows (vs2022). So I'm a bit at loss why you see what you see. Broken build for some reason?

Could be, that another RAM stick is dying.. Will run memtest. The code seems OK at least. And will do full rebuild too.

Only now I realized, that you are using get_strip_screen_quad() which gets quad from stored metadata.
I think, that this is OK thing to do, but these values are not guaranteed to be initialized. I wouldn't complicate
matters here, IMO this is responsibility of get_strip_screen_quad() itself, even though it makes the code
even worse...

I'm using exact same data that already existing "is the strip covering whole screen?" alpha-over optimization was using. So if that data might be missing or bogus, then the already existing optimization would not have worked either.

The data is guaranteed to be correct, after movie is rendered, which alpha-over optimization was doing.
This may be good question for @Sergey - whether this should be made foolproof before this patch can land. I would say, that it would be a good idea.

> Huh, I can't reproduce this at all. And yes I've tried simple setups as well. Now I tried your file, with your patch that adds a print, and it, as expected, prints "got 0 early out" for the non-occluded timeline portion, and "got 1 early out" for the occluded one. Tested both on Mac (clang 15) and Windows (vs2022). So I'm a bit at loss why you see what you see. Broken build for some reason? Could be, that another RAM stick is dying.. Will run memtest. The code seems OK at least. And will do full rebuild too. > > Only now I realized, that you are using `get_strip_screen_quad()` which gets quad from stored metadata. > > I think, that this is OK thing to do, but these values are not guaranteed to be initialized. I wouldn't complicate > > matters here, IMO this is responsibility of `get_strip_screen_quad()` itself, even though it makes the code > > even worse... > > I'm using exact same data that already existing "is the strip covering whole screen?" alpha-over optimization was using. So if that data might be missing or bogus, then the already existing optimization would not have worked either. The data is guaranteed to be correct, after movie is rendered, which alpha-over optimization was doing. This may be good question for @Sergey - whether this should be made foolproof before this patch can land. I would say, that it would be a good idea.
Sergey Sharybin commented 2024-02-20 17:16:00 +01:00
Owner
Copy Link

I am not sure what exactly question is. What exactly you mean by foolproof? And without such proofing, is the current PR not working reliably?

I am not sure what exactly question is. What exactly you mean by foolproof? And without such proofing, is the current PR not working reliably?
Richard Antalik commented 2024-02-20 18:28:23 +01:00
Member
Copy Link

I am not sure what exactly question is. What exactly you mean by foolproof? And without such proofing, is the current PR not working reliably?

get_strip_screen_quad() gets quad from stored metadata. But these values are not guaranteed to be initialized, unless strip is rendered. Normally this is done, when strip is added to timeline, but you can add placeholder strips via python API. In such case, the strip would not be rendered, unless it would be the topmost strip.

So I would suggest, that if values are not initialized, get_strip_screen_quad() will render raw image to ensure, that these values are initialized properly. This would mean, that it has to set up temporary variables needed for rendering, lock rendering mutex and render the raw image without further processing.

> I am not sure what exactly question is. What exactly you mean by foolproof? And without such proofing, is the current PR not working reliably? `get_strip_screen_quad()` gets quad from stored metadata. But these values are not guaranteed to be initialized, unless strip is rendered. Normally this is done, when strip is added to timeline, but you can add placeholder strips via python API. In such case, the strip would not be rendered, unless it would be the topmost strip. So I would suggest, that if values are not initialized, `get_strip_screen_quad()` will render raw image to ensure, that these values are initialized properly. This would mean, that it has to set up temporary variables needed for rendering, lock rendering mutex and render the raw image without further processing.
Richard Antalik requested changes 2024-02-20 18:40:36 +01:00
Richard Antalik left a comment
Member
Copy Link

Ok, I have done full review now. I have 2 suggestions mostly, one fix request :)

Ok, I have done full review now. I have 2 suggestions mostly, one fix request :)
source/blender/sequencer/intern/render.cc
@ -295,0 +325,4 @@
struct OpaqueQuad {
StripScreenQuad quad;
int order_index;
Richard Antalik commented 2024-02-20 17:50:39 +01:00
Member
Copy Link

I would use Sequence::machine value (this is the channel strip is in) here. No strip in lower channel will occlude strip in higher channel. Rendering order seems bit weird to me, but I guess it doesn't matter that much.

But then I would probably use blender::Map<Sequence, StripScreenQuad> just to avoid a wrapper struct and passing order as argument to OpaqueQuadTracker functions.

I would use `Sequence::machine` value (this is the channel strip is in) here. No strip in lower channel will occlude strip in higher channel. Rendering order seems bit weird to me, but I guess it doesn't matter that much. But then I would probably use `blender::Map<Sequence, StripScreenQuad>` just to avoid a wrapper struct and passing order as argument to `OpaqueQuadTracker` functions.
source/blender/sequencer/intern/render.cc Outdated
@ -498,0 +531,4 @@
const float y = float(context->recty);
StripScreenQuad quad = get_strip_screen_quad(context, seq);
StripScreenQuad screen{float2(0, 0), float2(x, 0), float2(0, y), float2(x, y)};
return !is_quad_a_inside_b(screen, quad);
Richard Antalik commented 2024-02-20 18:13:54 +01:00
Member
Copy Link

This is bug in original code - screenspace can be modified by aspect ratio without changing resolution (it changes actual ratio of pixel size), see viewport_pixel_aspect in seq_image_transform_quad_get_ex(). So you need to multiply context->rect* by aspect ratio as well.

This is bug in original code - screenspace can be modified by aspect ratio without changing resolution (it changes actual ratio of pixel size), see `viewport_pixel_aspect` in `seq_image_transform_quad_get_ex()`. So you need to multiply `context->rect*` by aspect ratio as well.
Aras Pranckevicius commented 2024-02-21 12:00:22 +01:00
Author
Member
Copy Link

It's not simply a multiplication, the "screen rect" has to be scaled around mid-point on both sides. But yeah, done and checked that it works correctly with various aspect ratio configs.

It's not simply a multiplication, the "screen rect" has to be scaled around mid-point on both sides. But yeah, done and checked that it works correctly with various aspect ratio configs.
aras_p marked this conversation as resolved
source/blender/sequencer/intern/render.cc Outdated
@ -1932,0 +1981,4 @@
* was opaque. */
ImBuf *ibuf_raw = seq_cache_get(context, seq, timeline_frame, SEQ_CACHE_STORE_RAW);
if (ibuf_raw != nullptr) {
if (ibuf_raw->planes != R_IMF_PLANES_RGBA) {
Richard Antalik commented 2024-02-20 18:36:13 +01:00
Member
Copy Link

Tu further simplify things, I would suggest to store ibuf_raw->planes value along resolution metadata (Sequence::Strip::StripElem::orig_*), we are already using for this optimization.

This would also allow for checking alphaover early out without strip rendering just by calling seq_image_transform_transparency_gained(), which could be renamed to something like seq_image_has_transparency() But this can be done separately.

Tu further simplify things, I would suggest to store `ibuf_raw->planes` value along resolution metadata (`Sequence::Strip::StripElem::orig_*`), we are already using for this optimization. This would also allow for checking alphaover early out without strip rendering just by calling `seq_image_transform_transparency_gained()`, which could be renamed to something like `seq_image_has_transparency()` But this can be done separately.
Sergey Sharybin commented 2024-02-20 18:55:08 +01:00
Owner
Copy Link

Can these fields which are needed by get_strip_screen_quad be initialized on the strip's creation and path changes? Similar to how we store size and duration of MovieClip on add/reload?

Can these fields which are needed by `get_strip_screen_quad ` be initialized on the strip's creation and path changes? Similar to how we store size and duration of MovieClip on add/reload?
Richard Antalik commented 2024-02-20 19:17:33 +01:00
Member
Copy Link

@Sergey Well they definitely could. It would involve some refactoring perhaps to allow editor code to do that, which is fine. But this wouldn't solve issue where there are python placeholder strips (they do have path setup, but file doesn't exist yet) and also older files(before transform system) may not have these values initialized.

@Sergey Well they definitely could. It would involve some refactoring perhaps to allow editor code to do that, which is fine. But this wouldn't solve issue where there are python placeholder strips (they do have path setup, but file doesn't exist yet) and also older files(before transform system) may not have these values initialized.
Richard Antalik commented 2024-02-20 19:28:31 +01:00
Member
Copy Link

@aras_p I still had issues with functionality. Stepping the code, I have seen NaN values in quad. Following change fixed that:

diff --git a/source/blender/sequencer/intern/strip_transform.cc b/source/blender/sequencer/intern/strip_transform.cc
index 0edc52a8ae0..72e8ac27bf7 100644
--- a/source/blender/sequencer/intern/strip_transform.cc
+++ b/source/blender/sequencer/intern/strip_transform.cc
@@ -676,7 +676,7 @@ static void seq_image_transform_quad_get_ex(const Scene *scene,
 
   float quad_temp[4][3];
   for (int i = 0; i < 4; i++) {
-    zero_v2(quad_temp[i]);
+    zero_v3(quad_temp[i]);
   }
 
   quad_temp[0][0] = (image_size[0] / 2) - crop->right;

Right now I don't feel like going over matrix multiplication code. I can check this on Windows / Mac as well. Perhaps something has changed in that codebase, but sounds weird, that it would be broken on Linux only. Will check that tomorrow.

@aras_p I still had issues with functionality. Stepping the code, I have seen NaN values in quad. Following change fixed that: ``` diff --git a/source/blender/sequencer/intern/strip_transform.cc b/source/blender/sequencer/intern/strip_transform.cc index 0edc52a8ae0..72e8ac27bf7 100644 --- a/source/blender/sequencer/intern/strip_transform.cc +++ b/source/blender/sequencer/intern/strip_transform.cc @@ -676,7 +676,7 @@ static void seq_image_transform_quad_get_ex(const Scene *scene, float quad_temp[4][3]; for (int i = 0; i < 4; i++) { - zero_v2(quad_temp[i]); + zero_v3(quad_temp[i]); } quad_temp[0][0] = (image_size[0] / 2) - crop->right; ``` Right now I don't feel like going over matrix multiplication code. I can check this on Windows / Mac as well. Perhaps something has changed in that codebase, but sounds weird, that it would be broken on Linux only. Will check that tomorrow.
Aras Pranckevicius commented 2024-02-21 11:23:50 +01:00
Author
Member
Copy Link

Stepping the code, I have seen NaN values in quad. Following change fixed that

Ah, a classic uninitialized value. I'll apply your change. Sometime later it would be nice to change all that code to use C++ math types...

> Stepping the code, I have seen NaN values in quad. Following change fixed that Ah, a classic uninitialized value. I'll apply your change. Sometime later it would be nice to change all that code to use C++ math types...
Aras Pranckevicius added 2 commits 2024-02-21 11:49:56 +01:00
Aras Pranckevicius added 1 commit 2024-02-21 11:58:40 +01:00
Aras Pranckevicius commented 2024-02-21 12:02:17 +01:00
Author
Member
Copy Link

@iss changes done:

  • Applied your uninitialized .z component in seq_image_transform_quad_get_ex (a bug that has existed outside of this branch already),
  • Applied missing render aspect ratio (also a but that has already existed),
  • Made it so that strips where "size" is not known for whatever reason are never considered to be "occluded".
@iss changes done: - Applied your uninitialized .z component in seq_image_transform_quad_get_ex (a bug that has existed outside of this branch already), - Applied missing render aspect ratio (also a but that has already existed), - Made it so that strips where "size" is not known for whatever reason are never considered to be "occluded".
Aras Pranckevicius requested review from Richard Antalik 2024-02-21 12:07:43 +01:00
Richard Antalik commented 2024-02-21 17:27:46 +01:00
Member
Copy Link

Stepping the code, I have seen NaN values in quad. Following change fixed that

Ah, a classic uninitialized value. I'll apply your change. Sometime later it would be nice to change all that code to use C++ math types...

What I am surprised by is, that it only fails when called by get_strip_screen_quad().

Also sorry for requesting fixes here. I should fix main branch and you can just merge these. It would be better that way if this feature was for whatever reason reverted.

> > Stepping the code, I have seen NaN values in quad. Following change fixed that > > Ah, a classic uninitialized value. I'll apply your change. Sometime later it would be nice to change all that code to use C++ math types... > What I am surprised by is, that it only fails when called by `get_strip_screen_quad()`. Also sorry for requesting fixes here. I should fix main branch and you can just merge these. It would be better that way if this feature was for whatever reason reverted.
Richard Antalik approved these changes 2024-02-21 17:43:32 +01:00
Richard Antalik left a comment
Member
Copy Link

Looks good now.

If I find some time, I should checke, whether this could use pixel space coords instead of screen space. Along with remaining inline comment.

Looks good now. If I find some time, I should checke, whether this could use pixel space coords instead of screen space. Along with remaining inline comment.
Aras Pranckevicius merged commit f4f708a54f into main 2024-02-21 20:16:53 +01:00
Aras Pranckevicius deleted branch vse_strip_occlusion 2024-02-21 20:16:56 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Richard Antalik
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset System
Asset datablocks, libraries, browser and shelf

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
Backward and forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Viewport & EEVEE

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Wayland windowing on Unix

  
Interest
Workbench

  
Interest: X11

Xorg/X11 windowing

  
Legacy
Asset Browser Project

Archived

  
Legacy
Blender 2.8 Project

Archived

  
Legacy
Milestone 1: Basic, Local Asset Browser

Archived

  
Legacy
OpenGL Error

Archived

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Related to security, https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Module
Viewport & EEVEE

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Severity
High

  
Severity
Low

  
Severity
Normal

  
Severity
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

Archived

  
Type
Report

Archived

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Video Sequencer
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#118396
No description provided.
Delete Branch "aras_p/blender:vse_strip_occlusion"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?