Details

Reviewers

Maniphest Tasks

T92445: VSE: Thumbnails are not full height without overlay texts

Commits

rBdaaa43232d34: Fix T92445: Thumbnail height without overlay text

Summary

This pattch changes the thumbnails' height used for image and movie
strips by choosing the proper size according to the VSE's text overlay
settings: i.e. thumbnails use the whole strip's height when no overlay
text is displayed; otherwise, some space is left for the overlay.

This also changes the thumb_y_end value in draw_seq_strip_thumbnail
by removing a useless subtraction of pixel_y from it.

This patch fixes T92445

Diff Detail

Repository

rB Blender

Branch

full_height_thumbnails (branched from master)

Build Status

Buildable 18488
Build 18488: arc lint + arc unit

Event Timeline

Andrea Beconcini (beco) requested review of this revision.Oct 30 2021, 12:18 PM

Andrea Beconcini (beco) created this revision.

Harbormaster completed remote builds in B18333: Diff 44139.Oct 30 2021, 12:18 PM

Andrea Beconcini (beco) added inline comments.Oct 30 2021, 1:00 PM

source/blender/editors/space_sequencer/sequencer_draw.c
1393	These `0.05f` are arbitrary constants i put there because I wanted to leave a partial view of the color of the underlying strip.
source/blender/editors/space_sequencer/sequencer_thumbnails.c
131–132	I am not really happy with this change, I would prefer to retrieve the `y2 - y1` range without passing two new parameters to `seq_get_thumb_image_dimensions()` but this method is also used by the job that computes the thumbnail asynchronously. I don't think I can retrieve the height I need from that job, because there could be no active sequence during the thumbnail creation (thus I have no access to the actual sequence and the channel it belongs to), that's why I added`y1` and `y2` to `ThumbnailDrawJob`.
473	I am not sure this change is right. I could not understand why we had that `- pixely` in the first place. AFAIK, `pixely` is the height of a pixel in screen-space units, and it is used to convert distances from pixels to openGL units. Am I missing something? I kept this change in a separate commit, so that it can easily be removed in case if wrong.

Andrea Beconcini (beco) added a reviewer: Richard Antalik (ISS).Oct 30 2021, 1:03 PM

Pratik Borhade (PratikPB2123) retitled this revision from fix thumbnail height w/out overlay text to Fix T92445: thumbnail height w/out overlay text.Oct 30 2021, 1:10 PM

Pratik Borhade (PratikPB2123) added a project: Video Sequencer.

Pratik Borhade (PratikPB2123) awarded a token.

To summarize my position, main objection is reduction of thumbnail size with this patch. I think it is unnecessary. Otherwise I could accept, though if you can eliminate passing additional arguments it would be great.

source/blender/editors/space_sequencer/sequencer_draw.c
1393	I don't really like this change. It makes thumbnails smaller even when text is visible - requires even taller strips to see the thumbnails. Perhaps there could be small offset when there is no text, and this offset does not need to be on both sides. But I don't think there is need for this offset really.
source/blender/editors/space_sequencer/sequencer_thumbnails.c
131–132	Getting values from running job is quite bad idea - it can have outdated information. Previously this used `SEQ_STRIP_OFSTOP` and `SEQ_STRIP_OFSBOTTOM` defines, which is fine. But `(20 * U.dpi_fac * pixely)` looks like it is just approximation of text height which is not very great. I didn't notice this during review, I would object to this line then. Passing these `y2` and `y1` values is not as terrible really. Alternative would be to define text size similar to `SEQ_STRIP_OFSBOTTOM` and use this instead. Even if you added `#define SEQ_STRIP_TEXT_HEIGHT (20 * U.dpi_fac * pixely)` it would be better than inlining the code directly. Then you need just way to know if text is drawn, this could be abstracted away too, and check can be reused where needed from context instead of passing boundaries. I don't have really strong opinion here, in practice you still need to mix these workflows, but I am not fan of passing 30 variables either. Issue with adding utility functions to get these parameters is, that it could really use some general drawing code cleanup, because utility functions that look reasonable now may be completely unnecessary if it was structured better.
473	It looks like 1px offset being applied to height on top of thumbnail. Not quite sure why, but doesn't look necessary on first glance. Perhaps @Aditya Y Jeppu (quantimoney) does remember why/if this is necessary. I don't see visible difference here

This revision now requires changes to proceed.Nov 1 2021, 11:04 PM

In D13043#344619, @Richard Antalik (ISS) wrote:

It looks like 1px offset being applied to height on top of thumbnail. Not quite sure why, but doesn't look necessary on first glance.

i had put this initially, because the thumbnails used to just touch the text, and intersect the bottom edge of the letters, looked bad. so at the time brute force i think i just added a single pixel width of space and it worked. Doesn't seem to be an issue now

Fix T92445: thumbnails height w/out overlay text

This commit changes the thumbnails' height used for image and movie
strips by choosing the proper size according to the VSE's text overlay
settings: i.e. thumbnails use almost the whole strip's height when no
overlay text is displayed; otherwise, some space is left for the
overlay.

This also changes the thumb_y_end value in draw_seq_strip_thumbnail
by removing a useless subtraction of pixel_y from it.

These commits fix T92445

Maniphest Tasks: T92445

Differential Revision: https://developer.blender.org/D13043

Harbormaster completed remote builds in B18408: Diff 44271.Nov 2 2021, 10:21 PM

Fix T92445: thumbnail height w/out overlay text

This also changes the thumb_y_end value in draw_seq_strip_thumbnail
by removing a useless subtraction of pixel_y from it.

These commits fix T92445

Maniphest Tasks: T92445

Differential Revision: https://developer.blender.org/D13043

Harbormaster completed remote builds in B18409: Diff 44273.Nov 2 2021, 10:24 PM

@Andrea Beconcini (beco) seems like you have merged master into patch branch? I usually do rebase and don't trust arc with any other workflow much... Let me know if you need help. You can always upload diff manually though.

In D13043#345164, @Richard Antalik (ISS) wrote:

@Andrea Beconcini (beco) seems like you have merged master into patch branch? I usually do rebase and don't trust arc with any other workflow much... Let me know if you need help. You can always upload diff manually though.

I'm sorry, I had some hard time rebasing the latest changes. I checked out the release branch, run make update (I think it should take care of updating submodules properly), and rebase my local branch on the release one. I am going to fix this within tomorrow and leave a comment when done!

Andrea Beconcini (beco) updated this revision to Diff 44389.Nov 4 2021, 9:57 PM

Harbormaster completed remote builds in B18488: Diff 44389.Nov 4 2021, 9:57 PM

rebased and updated.

Thumbnails take the whole strip height when no overlay text is displayed.
The latest revision adds only one extra parameter to ThumbnailDrawJob, thumb_height.

Thanks for update, will check this patch next week, I want to focus on going through backlog of bugs reported in 3.0 this week.

Sorry I forgot this is bugfix, so I will check this out...

I got one nitpick, otherwise I think this is good change. I think even better than my suggestions so far :)

source/blender/editors/space_sequencer/sequencer_thumbnails.c
113	It's very weird to have this return variable then, it should be removed. Could be simplified to 1 return value IMO, but that's not point of this patch.
471	I would declare `const float thumb_height = y2 - y1;` here, and remove argument from `seq_get_thumb_image_dimensions` as I mentioned in inline above. Then pass it to `sequencer_thumbnail_start_job_if_necessary` too. Passing `y2 - y1` is redundant math and not quite as readable.

This revision now requires changes to proceed.Nov 5 2021, 5:36 AM

I removed the extra parameter, r_thumb_height, from seq_get_thumb_image_dimensions. Now, there is only thumb_height, which is passed by value.

Moreover, now, the value y2 - y1 is stored in a local variable (const float thumb_height) of draw_seq_strip_thumbnail for re-use.

Harbormaster completed remote builds in B18742: Diff 44888.Nov 17 2021, 9:44 PM

Seems to be good now, Thanks for patch!

This revision is now accepted and ready to land.Nov 18 2021, 2:06 AM

Closed by commit rBdaaa43232d34: Fix T92445: Thumbnail height without overlay text (authored by Andrea Beconcini (beco), committed by Richard Antalik (ISS)). · Explain WhyNov 18 2021, 2:23 AM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rBdaaa43232d34: Fix T92445: Thumbnail height without overlay text.

Fix T92445: thumbnail height w/out overlay text
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 44389

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/editors/space_sequencer/sequencer_thumbnails.c

Fix T92445: thumbnail height w/out overlay textClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 44389

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/editors/space_sequencer/sequencer_thumbnails.c

Fix T92445: thumbnail height w/out overlay text
ClosedPublic
Actions

Revision Contents
Changeset List