Paths

Table of Contentst

Differential D12266

VSE strip thumbnails
ClosedPublic
Actions

Authored by Aditya Y Jeppu (quantimoney) on Aug 19 2021, 12:06 PM.

Details

Reviewers

Richard Antalik (ISS)

Maniphest Tasks

T89143: VSE Strip Thumbnails

Commits

rB997b5fe45dab: VSE strip thumbnails

Summary

The goal is to provide thumbnails in the rectangle coloured strips. Works for
movie clips and image sequences. The thumbnails are loaded from source
using separate thread and stores them in cache. The drawing is called inside the
drawing for each strip, and takes images from cache. Drawing is below the
handles from one end of strip to other, inside view only. The job for caching
is called when images are not available, or there is view change.

All strip operations are valid, and overlap of strip adds transparency to the
image. Images are shown only when strip size is wide enough for clear
visibility of images, and the thumbnails represent source footage clearly.

Cache is limited to 5000 thumbnails and performs cleanup of non visible images
when limit crossed.

Related Task : T89143

Diff Detail

Repository

rB Blender

Branch

arcpatch-D12266_1 (branched from master)

Build Status

Buildable 17140
Build 17140: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Richard Antalik (ISS) added inline comments.Aug 20 2021, 4:18 AM

source/blender/editors/space_sequencer/sequencer_draw.c
1338	I can see that you use this function to calculate frame step, but what it really does is calculate thumbnail and image dimensions. I think it is best to split this function in 2 and give them appropriate names.
1341–1344	Use full words width and height. Same for `r_image_x` -> `r_image_width` and y -> height.
1456–1459	It seems a bit wasteful to free just recently initialized ghash. It should be created in this function. I think, that same should go for `tj->context` I would still suggest to have function dedicated to create ghash, not just move code here directly.
1472	Probably better name would be `sequencer_thumbnail_start_job_if_necessary`. With data initialization removed, this would be quite descriptive name. instead of `leftover` I would suggest `thumbnail_is_missing`
1537–1539	I suggest same thing as above - declare variable where you assign value.
1543–1546	I think having `min_size` variable doesn't help too much, you could write `if (((y2 - y1) / pixely) <= 40 * U.dpi_fac){}` with clarifying comment.
1548	I would add comment here explaining why `rectx` and `recty` are set to 0.
1554–1555	I would move this down to `ED_draw_imbuf_ctx_clipping` to keep variable scope small
1557–1558	Don't reuse passed variables like this, it can get quite confusing, especially in iterative loop. For `y2` I would suggest new variable. Either leave `x1` as is and have offset that you would increment for each iteration. in code then use `x1 + offset` or have some variable like `thumbnail_x_start` and `thumbnail_x_end` or something like that.
1561	This fails for single image strips. I think this should be `float upper_thumb_bound = seq->enddisp;` unconditionally and in such case you probably could use enddisp for comparisons directly.
1581–1585	I think you could say, that `start_frame = seq->startdisp + (seq->startdisp - seq->start) % thumb_w` and you wouldn't need this condition. But I think if you apply hold offset, this won't be true. Having said that hold offset is not reflected in thumbnails correctly until you "refresh" cache with CTRL + R With hold offset you could say, that it does "erase" strip content. We haven't thought about this really, but I think it should behave in same way, that other offsets do, so in that case, `start_frame = seq->startdisp + (seq->startdisp - seq->start - seq->anim_startosf) % thumb_w` should work. Similar math would apply for `v2d->cur.xmin`. These preconditions are more readable however, so I would perhaps leave this as is.
1610	Initialize in `for (int tot; ...)` also better would be `pixel`.
1618	I would imagine this could be done more efficiently, but can't tell myself how. Seems to work fairly fast though. One concern I have is, that when you modify `ibuf` that is cached, you actually modify original image(which is stored to be reused later) - it is not copied, but here it seems, that changes are not persistent, which I don't quite understand why.
2058–2059	unnecessary change
2225	This seems to me unnecessary change
2742–2747	This seems to me unnecessary change
source/blender/editors/space_sequencer/sequencer_edit.c
635–639	There is compile warning for unused variable `ed` - remove it once is unused.
source/blender/makesdna/DNA_space_types.h
626	Better name would be `last_thumbnail_area` or something like that. Also fields like this should be added to special `runtime` field, so it is evident, that this is temporary field, that doesn't have to be saved, but is important for operation. See `EditingRuntime` in `Editing` struct
source/blender/sequencer/intern/image_cache.c
152
1320–1321	I don't think this is good idea to add thumbnails here. they should only be invalidated when limit is reached.
1367	and remove `invalidate_composite` variable.
1486	you could remove this argument, it must always be `SEQ_CACHE_STORE_THUMBNAIL`
1490	`context->skip_cache \|\| context->is_proxy_render \|\| !seq` must never be true, so these conditions could be removed. Not sure if `i == NULL` could happen with current implementation.
1496–1501	`context->is_prefetch_render` must never be true here so this block can be removed.
1503–1508	This shouldn't be necessary, also it's better to use `BLI_ghash_haskey` - I didn't know this in past... But back to key linking - this should be avoided in `seq_cache_put_ex`: if (!key->is_temp_cache \|\| key->type != SEQ_CACHE_STORE_THUMBNAIL) { cache->last_key = key; }
1520–1524	Math like this should be done in `seq_cache_thumbnail_cleanup`
source/blender/sequencer/intern/render.c
86	I would say this should be defined in header, so drawing code can use this value too, but then it should be called `SEQ_THUMB_SIZE` or perhaps `SEQ_RENDER_THUMB_SIZE`. I would use latter as to avoid possible naming conflicts in future.
552	`context->for_render` will never be true here, so use `IMB_FILTER_NEAREST` explicitly
2021–2059
2029	Declare where you assign value.
2034	I guess `end_frame` would be more consistent?
2053	Same suggestion to have offset and add this to start frame.
2053–2056	This would cause black thumbnail to be created if image can not be rendered. I think it is safe to assume that `scaled_ibuf` is never null and return it directly, so I will suggest edit to this whole function (see below).
2065–2066	you can initialize "render state" in `seq_get_uncached_thumbnail`. it is only used for scene strips.
2067	Declare these where you assign value. Also `temp` -> `thumbnail_cropped` or `ibuf_cropped`
2071
2082–2084	This is incorrect I think? You can't return uncropped image if cropping fails. Return `NULL` in such case (same way as suggested edit above)

Richard Antalik (ISS) requested changes to this revision.Aug 20 2021, 4:19 AM

This revision now requires changes to proceed.Aug 20 2021, 4:19 AM

Aditya Y Jeppu (quantimoney) marked 8 inline comments as done.Aug 22 2021, 6:29 PM

Aditya Y Jeppu (quantimoney) added inline comments.

source/blender/editors/space_sequencer/sequencer_draw.c
1338	name change makes sense. I don't think it is necessary to split, thumbnails code wise i use both these things together always. One for dimensions of the image taken from source and another for dimensions of what is displayed.

Aditya Y Jeppu (quantimoney) marked an inline comment as done.Aug 22 2021, 6:29 PM

Aditya Y Jeppu (quantimoney) marked 16 inline comments as done.Aug 25 2021, 4:31 PM

Aditya Y Jeppu (quantimoney) added inline comments.

source/blender/editors/space_sequencer/sequencer_draw.c
1561	Here for movie clips it needs to be exactly to the end of the strip and not enddisp. For that reason I had added this. So i am just adding a condition to check for type of strip and setting limits like that.
1581–1585	Leaving this one. I could modify it in the `seq_thumbnail_get_start_frame`, so that it takes everything into consideration, at the end.
1618	I had talked about this before to you saying it is fast enough for now and how it shouldn't be working correctly but it is. I could just duplicate, but leaving it for now to check why it is working now.
2742–2747	my mistake here, it was old code i guess which i shifted to separate function.

Aditya Y Jeppu (quantimoney) marked 4 inline comments as done.Aug 25 2021, 4:35 PM

Aditya Y Jeppu (quantimoney) marked 10 inline comments as done.Sep 1 2021, 8:39 PM

Aditya Y Jeppu (quantimoney) marked 10 inline comments as done.Sep 2 2021, 3:12 PM

Aditya Y Jeppu (quantimoney) added inline comments.

source/blender/sequencer/intern/render.c
2053	Here i have checked the initial function to obtain the start_frame (`seq_thumbnail_get_start_frame`). if seq->start is inside view then it should just be starting from seq->start. if not then it provides the correct starting frame value (taking into consideration the view's x axis minimum limits). The looping using frame_offset (width of thumbnail) increments for every iteration is required because I do need to go image by image : For rendering, I do have to check if each image is in the cache, so i would have to go one by one to get the last image that is cached. For drawing it does the looping but there it is necessary to loop, as each image is required from the start frame. So the only issue seems to be in hold offsets. I am just adding this explicitly into wherever it is needed (two places, one for drawing and another for rendering) as position of the image needs to be the same as before, but only the image should be from a different frame.

Aditya Y Jeppu (quantimoney) marked an inline comment as done.Sep 2 2021, 3:12 PM

Add changes requested in D1226

Harbormaster completed remote builds in B16784: Diff 41380.Sep 2 2021, 6:58 PM

Very happy to see this being developed! I've been testing out the branch and noticed an extreme flickering when zooming in the VSE. The flickering seems to affect strip labels as well. Do you see this as well?

The flickering is because the thumbnails are updated for each view (to make it accurately represent the video, ie the left edge of the image is the frame that is shown in the image). So as view changes new images are loaded, because its from a different frame. Didn't know about the text flickering. Another user had pointed this out, but during the GSoC period there was time constraint so it was left for later.

I think it requires some way of knowing when i am in a "zooming / view changing" mode and when you have stopped and settled on a view, after which the images could be updated. Rather than do a real time update.

@Aditya Y Jeppu (quantimoney) check RV3D_NAVIGATING we could have an equivalent flag for all View2D.

Missed a small TODO in the previous patch

Small fix of using defined constant instead of immediate value

Harbormaster completed remote builds in B16796: Diff 41399.Sep 3 2021, 11:25 AM

I can't reprotuce text flashing as in video, but for thumbnails this seems bit out of place, I think, that simplest solution would be to draw only consequential strips of thumbnails from beginning of strip. I think this will look much less distracting. But perhaps as @Campbell Barton (campbellbarton) suggests we will need something like RV3D_NAVIGATING. Will post video to show how this would look like with my suggestion.

With following chage it is better, but still not very great. Sometimes it can hit view where all thumbnails are pre-rendered

P2372 (An Untitled Masterwork)

2 index

3 ---

4 +++

5 @@

6 /* Get

7 ImBuf

9 -

10 -

11 -

12 -

13 -

14 -

15 -

16 -

17 -

18 -

19 -

20 -

21 -

22 -

23 -

24 -

25 -

26 -

27 -

28 -

29 -

30 -

31 -

32 -

33 +

34 sequencer_t

35 +

36 }

37 +

38 +

39 +

40 +

41 +

42 +

43 +

44 +

45 +

46 +

47 +

48 +

49 +

50 +

51 +

52 +

53 +

54 +

55 +

56 +

57 +

58 +

59 GPU_blend(GPU

60 cut_off = 0;

61 thumb_x_start

diff --git a/source/blender/editors/space_sequencer/sequencer_draw.c b/source/blender/editors/space_sequencer/sequencer_draw.c 1ec6d9dc4a7..3e744a55183 100644 a/source/blender/editors/space_sequencer/sequencer_draw.c b/source/blender/editors/space_sequencer/sequencer_draw.c -1603,32 +1603,32 @@ static void draw_seq_strip_thumbnail(View2D *v2d, the image. */ *ibuf = SEQ_get_thumbnail(&context, seq, roundf(thumb_x_start), &crop, clipped); if (ibuf) { /* Transparency on overlap. */ if (seq->flag & SEQ_OVERLAP) { GPU_blend(GPU_BLEND_ALPHA); unsigned char *buf = (unsigned char *)ibuf->rect; for (int pixel = ibuf->x * ibuf->y; pixel--; buf += 4) { buf[3] = OVERLAP_ALPHA; } } ED_draw_imbuf_ctx_clipping(C, ibuf, thumb_x_start + cut_off, y1, true, thumb_x_start + cut_off, y1, thumb_x_end, thumb_y_end, zoom_x, zoom_y); IMB_freeImBuf(ibuf); } else { if (!ibuf) { humbnail_start_job_if_necessary(C, scene->ed, v2d, true); break; /* Transparency on overlap. */ if (seq->flag & SEQ_OVERLAP) { GPU_blend(GPU_BLEND_ALPHA); unsigned char *buf = (unsigned char *)ibuf->rect; for (int pixel = ibuf->x * ibuf->y; pixel--; buf += 4) { buf[3] = OVERLAP_ALPHA; } } ED_draw_imbuf_ctx_clipping(C, ibuf, thumb_x_start + cut_off, y1, true, thumb_x_start + cut_off, y1, thumb_x_end, thumb_y_end, zoom_x, zoom_y); IMB_freeImBuf(ibuf); _BLEND_NONE); = thumb_x_end;

Implemented flag that indicates whether navigation is in progress in following diff. This contains changes above since we don't want thumbnails randomly distributed.

P2373 (An Untitled Masterwork)

1	diff --git a/source/blender/editors/interface/view2d_ops.c b/source/blender/editors/interface/view2d_ops.c
2	index 1fd1b6c984d..3afbcf78851 100644
3	--- a/source/blender/editors/interface/view2d_ops.c
4	+++ b/source/blender/editors/interface/view2d_ops.c
5	@@ -140,20 +140,22 @@ static void view_pan_init(bContext C, wmOperator op)
6	vpd->screen = CTX_wm_screen(C);
7	vpd->area = CTX_wm_area(C);
8	vpd->region = CTX_wm_region(C);
9	vpd->v2d = &vpd->region->v2d;
10
11	/* calculate translation factor - based on size of view */
12	const float winx = (float)(BLI_rcti_size_x(&vpd->region->winrct) + 1);
13	const float winy = (float)(BLI_rcti_size_y(&vpd->region->winrct) + 1);
14	vpd->facx = (BLI_rctf_size_x(&vpd->v2d->cur)) / winx;
15	vpd->facy = (BLI_rctf_size_y(&vpd->v2d->cur)) / winy;
16	+
17	+ vpd->v2d->flag \|= V2D_IS_NAVIGATING;
18	}
19
20	/* apply transform to view (i.e. adjust 'cur' rect) */
21	static void view_pan_apply_ex(bContext C, v2dViewPanData vpd, float dx, float dy)
22	{
23	View2D *v2d = vpd->v2d;
24
25	/* calculate amount to move view by */
26	dx *= vpd->facx;
27	dy *= vpd->facy;
28	@@ -183,20 +185,22 @@ static void view_pan_apply_ex(bContext C, v2dViewPanData vpd, float dx, float
29	static void view_pan_apply(bContext C, wmOperator op)
30	{
31	v2dViewPanData *vpd = op->customdata;
32
33	view_pan_apply_ex(C, vpd, RNA_int_get(op->ptr, "deltax"), RNA_int_get(op->ptr, "deltay"));
34	}
35
36	/* Cleanup temp custom-data. */
37	static void view_pan_exit(wmOperator *op)
38	{
39	+ v2dViewPanData *vpd = op->customdata;
40	+ vpd->v2d->flag &= ~V2D_IS_NAVIGATING;
41	MEM_SAFE_FREE(op->customdata);
42	}
43
44	/** \} */
45
46	/* -------------------------------------------------------------------- */
47	/** \name View Pan Operator (modal drag-pan)
48	* \{ */
49
50	/* for 'redo' only, with no user input */
51	@@ -298,21 +302,21 @@ static int view_pan_modal(bContext C, wmOperator op, const wmEvent *event)
52
53	return OPERATOR_FINISHED;
54	}
55	}
56	break;
57	}
58
59	return OPERATOR_RUNNING_MODAL;
60	}
61
62	-static void view_pan_cancel(bContext UNUSED(C), wmOperator op)
63	+static void view_pan_cancel(bContext C, wmOperator op)
64	{
65	view_pan_exit(op);
66	}
67
68	static void VIEW2D_OT_pan(wmOperatorType *ot)
69	{
70	/* identifiers */
71	ot->name = "Pan View";
72	ot->description = "Pan the view";
73	ot->idname = "VIEW2D_OT_pan";
74	@@ -351,33 +355,36 @@ static int view_edge_pan_invoke(bContext C, wmOperator op, const wmEvent *UNUS
75	WM_event_add_modal_handler(C, op);
76
77	return (OPERATOR_RUNNING_MODAL \| OPERATOR_PASS_THROUGH);
78	}
79
80	static int view_edge_pan_modal(bContext C, wmOperator op, const wmEvent *event)
81	{
82	View2DEdgePanData *vpd = op->customdata;
83
84	if (event->val == KM_RELEASE \|\| event->type == EVT_ESCKEY) {
85	+ vpd->v2d->flag &= ~V2D_IS_NAVIGATING;
86	MEM_SAFE_FREE(op->customdata);
87	return (OPERATOR_FINISHED \| OPERATOR_PASS_THROUGH);
88	}
89
90	UI_view2d_edge_pan_apply_event(C, vpd, event);
91
92	/* This operator is supposed to run together with some drag action.
93	* On successful handling, always pass events on to other handlers. */
94	return OPERATOR_PASS_THROUGH;
95	}
96
97	static void view_edge_pan_cancel(bContext UNUSED(C), wmOperator op)
98	{
99	+ v2dViewPanData *vpd = op->customdata;
100	+ vpd->v2d->flag &= ~V2D_IS_NAVIGATING;
101	MEM_SAFE_FREE(op->customdata);
102	}
103
104	static void VIEW2D_OT_edge_pan(wmOperatorType *ot)
105	{
106	/* identifiers */
107	ot->name = "View Edge Pan";
108	ot->description = "Pan the view when the mouse is held at an edge";
109	ot->idname = "VIEW2D_OT_edge_pan";
110
111	@@ -673,20 +680,22 @@ static void view_zoomdrag_init(bContext C, wmOperator op)
112
113	/* set custom-data for operator */
114	v2dViewZoomData *vzd = MEM_callocN(sizeof(v2dViewZoomData), "v2dViewZoomData");
115	op->customdata = vzd;
116
117	/* set pointers to owners */
118	vzd->region = CTX_wm_region(C);
119	vzd->v2d = &vzd->region->v2d;
120	/* False by default. Interactive callbacks (ie invoke()) can set it to true. */
121	vzd->zoom_to_mouse_pos = false;
122	+
123	+ vzd->v2d->flag \|= V2D_IS_NAVIGATING;
124	}
125
126	/* apply transform to view (i.e. adjust 'cur' rect) */
127	static void view_zoomstep_apply_ex(bContext *C,
128	v2dViewZoomData *vzd,
129	const float facx,
130	const float facy)
131	{
132	ARegion *region = CTX_wm_region(C);
133	View2D *v2d = &region->v2d;
134	@@ -802,21 +811,22 @@ static void view_zoomstep_apply(bContext C, wmOperator op)
135	/** \} */
136
137	/* -------------------------------------------------------------------- */
138	/** \name View Zoom Operator (single step)
139	* \{ */
140
141	/* Cleanup temp custom-data. */
142	static void view_zoomstep_exit(wmOperator *op)
143	{
144	UI_view2d_zoom_cache_reset();
145	-
146	+ v2dViewZoomData *vzd = op->customdata;
147	+ vzd->v2d->flag &= ~V2D_IS_NAVIGATING;
148	MEM_SAFE_FREE(op->customdata);
149	}
150
151	/* this operator only needs this single callback, where it calls the view_zoom_() methods /
152	static int view_zoomin_exec(bContext C, wmOperator op)
153	{
154	if (op->customdata == NULL) { /* Might have been setup in _invoke() already. */
155	view_zoomdrag_init(C, op);
156	}
157
158	@@ -1034,20 +1044,21 @@ static void view_zoomdrag_apply(bContext C, wmOperator op)
159	UI_view2d_sync(CTX_wm_screen(C), CTX_wm_area(C), v2d, V2D_LOCK_COPY);
160	}
161
162	/* Cleanup temp custom-data. */
163	static void view_zoomdrag_exit(bContext C, wmOperator op)
164	{
165	UI_view2d_zoom_cache_reset();
166
167	if (op->customdata) {
168	v2dViewZoomData *vzd = op->customdata;
169	+ vzd->v2d->flag &= ~V2D_IS_NAVIGATING;
170
171	if (vzd->timer) {
172	WM_event_remove_timer(CTX_wm_manager(C), CTX_wm_window(C), vzd->timer);
173	}
174
175	MEM_freeN(op->customdata);
176	op->customdata = NULL;
177	}
178	}
179
180	diff --git a/source/blender/editors/space_sequencer/sequencer_draw.c b/source/blender/editors/space_sequencer/sequencer_draw.c
181	index 1ec6d9dc4a7..94c9f8565b5 100644
182	--- a/source/blender/editors/space_sequencer/sequencer_draw.c
183	+++ b/source/blender/editors/space_sequencer/sequencer_draw.c
184	@@ -1498,53 +1498,81 @@ static void sequencer_thumbnail_init_job(const bContext C, View2D v2d, Editing
185	ED_area_tag_redraw(area);
186	}
187
188	static void sequencer_thumbnail_start_job_if_necessary(const bContext *C,
189	Editing *ed,
190	View2D *v2d,
191	bool thumbnail_is_missing)
192	{
193	SpaceSeq *sseq = CTX_wm_space_seq(C);
194
195	+ if ((v2d->flag & V2D_IS_NAVIGATING) != 0) {
196	+ WM_event_add_notifier(C, NC_SCENE \| ND_SEQUENCER, NULL);
197	+ return;
198	+ }
199	+
200	/* Leftover is set to true if missing image in strip. False when normal call to all strips done.
201	*/
202	if (v2d->cur.xmax != sseq->runtime.last_thumbnail_area.xmax \|\|
203	v2d->cur.ymax != sseq->runtime.last_thumbnail_area.ymax \|\| thumbnail_is_missing) {
204
205	/* Stop the job first as view has changed. Pointless to continue old job. */
206	if (v2d->cur.xmax != sseq->runtime.last_thumbnail_area.xmax \|\|
207	v2d->cur.ymax != sseq->runtime.last_thumbnail_area.ymax) {
208	WM_jobs_stop(CTX_wm_manager(C), NULL, thumbnail_start_job);
209	}
210
211	sequencer_thumbnail_init_job(C, v2d, ed);
212	sseq->runtime.last_thumbnail_area = v2d->cur;
213	}
214	}
215
216	+/* Don't display thumbnails only when zooming. Panning doesn't cause issues. */
217	+static bool sequencer_thumbnail_v2d_is_navigating(const bContext *C)
218	+{
219	+ ARegion *region = CTX_wm_region(C);
220	+ View2D *v2d = &region->v2d;
221	+ SpaceSeq *sseq = CTX_wm_space_seq(C);
222	+
223	+ if ((v2d->flag & V2D_IS_NAVIGATING) == 0) {
224	+ return false;
225	+ }
226	+
227	+ double x_diff = fabs(BLI_rctf_size_x(&sseq->runtime.last_thumbnail_area) -
228	+ BLI_rctf_size_x(&v2d->cur));
229	+ double y_diff = fabs(BLI_rctf_size_y(&sseq->runtime.last_thumbnail_area) -
230	+ BLI_rctf_size_y(&v2d->cur));
231	+ return x_diff > 0.01 \|\| y_diff > 0.01;
232	+}
233	+
234	static void draw_seq_strip_thumbnail(View2D *v2d,
235	const bContext *C,
236	Scene *scene,
237	Sequence *seq,
238	float y1,
239	float y2,
240	float pixelx,
241	float pixely)
242	{
243	bool clipped = false;
244	float image_height, image_width, thumb_width, thumb_height;
245	rcti crop;
246
247	/* If width of the strip too small ignore drawing thumbnails. */
248	if ((y2 - y1) / pixely <= 40 * U.dpi_fac)
249	return;
250
251	+ if (sequencer_thumbnail_v2d_is_navigating(C)) {
252	+ WM_event_add_notifier(C, NC_SCENE \| ND_SEQUENCER, NULL);
253	+ return;
254	+ }
255	+
256	SeqRenderData context = sequencer_thumbnail_context_init(C);
257	seq_get_thumb_image_dimensions(
258	seq, pixelx, pixely, &thumb_width, &thumb_height, &image_width, &image_height);
259
260	float thumb_y_end = y1 + thumb_height - pixely;
261	float thumb_x_start = seq->start;
262
263	float cut_off = 0;
264	float upper_thumb_bound = (seq->endstill) ? (seq->start + seq->len) : seq->enddisp;
265	if (seq->type == SEQ_TYPE_IMAGE) {
266	@@ -1596,46 +1624,46 @@ static void draw_seq_strip_thumbnail(View2D *v2d,
267	float cropx_min = (cut_off / pixelx) / (zoom_y / pixely);
268	float cropx_max = ((thumb_x_end - thumb_x_start) / pixelx) / (zoom_y / pixely);
269	if (cropx_max == (thumb_x_end - thumb_x_start)) {
270	cropx_max = cropx_max + 1;
271	}
272	BLI_rcti_init(&crop, (int)(cropx_min), (int)cropx_max, 0, (int)(image_height)-1);
273
274	/* Get the image. */
275	ImBuf *ibuf = SEQ_get_thumbnail(&context, seq, roundf(thumb_x_start), &crop, clipped);
276
277	- if (ibuf) {
278	- /* Transparency on overlap. */
279	- if (seq->flag & SEQ_OVERLAP) {
280	- GPU_blend(GPU_BLEND_ALPHA);
281	- unsigned char buf = (unsigned char )ibuf->rect;
282	- for (int pixel = ibuf->x * ibuf->y; pixel--; buf += 4) {
283	- buf[3] = OVERLAP_ALPHA;
284	- }
285	- }
286	-
287	- ED_draw_imbuf_ctx_clipping(C,
288	- ibuf,
289	- thumb_x_start + cut_off,
290	- y1,
291	- true,
292	- thumb_x_start + cut_off,
293	- y1,
294	- thumb_x_end,
295	- thumb_y_end,
296	- zoom_x,
297	- zoom_y);
298	- IMB_freeImBuf(ibuf);
299	- }
300	- else {
301	+ if (!ibuf) {
302	sequencer_thumbnail_start_job_if_necessary(C, scene->ed, v2d, true);
303	+ break;
304	}
305	+
306	+ /* Transparency on overlap. */
307	+ if (seq->flag & SEQ_OVERLAP) {
308	+ GPU_blend(GPU_BLEND_ALPHA);
309	+ unsigned char buf = (unsigned char )ibuf->rect;
310	+ for (int pixel = ibuf->x * ibuf->y; pixel--; buf += 4) {
311	+ buf[3] = OVERLAP_ALPHA;
312	+ }
313	+ }
314	+
315	+ ED_draw_imbuf_ctx_clipping(C,
316	+ ibuf,
317	+ thumb_x_start + cut_off,
318	+ y1,
319	+ true,
320	+ thumb_x_start + cut_off,
321	+ y1,
322	+ thumb_x_end,
323	+ thumb_y_end,
324	+ zoom_x,
325	+ zoom_y);
326	+ IMB_freeImBuf(ibuf);
327	GPU_blend(GPU_BLEND_NONE);
328	cut_off = 0;
329	thumb_x_start = thumb_x_end;
330	}
331	}
332
333	/* Draw visible strips. Bounds check are already made. */
334	static void draw_seq_strip(const bContext *C,
335	SpaceSeq *sseq,
336	Scene *scene,
337	diff --git a/source/blender/makesdna/DNA_view2d_types.h b/source/blender/makesdna/DNA_view2d_types.h
338	index c385ac04bd3..f8166305fd9 100644
339	--- a/source/blender/makesdna/DNA_view2d_types.h
340	+++ b/source/blender/makesdna/DNA_view2d_types.h
341	@@ -125,20 +125,22 @@ enum {
342	/* global view2d horizontal locking (for showing same time interval) */
343	/* TODO: this flag may be set in old files but is not accessible currently,
344	* should be exposed from RNA - Campbell */
345	V2D_VIEWSYNC_SCREEN_TIME = (1 << 0),
346	/* within area (i.e. between regions) view2d vertical locking */
347	V2D_VIEWSYNC_AREA_VERTICAL = (1 << 1),
348	/* apply pixel offsets on x-axis when setting view matrices */
349	V2D_PIXELOFS_X = (1 << 2),
350	/* apply pixel offsets on y-axis when setting view matrices */
351	V2D_PIXELOFS_Y = (1 << 3),
352	+ /* zoom, pan or similar action is in progress */
353	+ V2D_IS_NAVIGATING = (1 << 9),
354	/* view settings need to be set still... */
355	V2D_IS_INIT = (1 << 10),
356	};
357
358	/* scroller flags for View2D (v2d->scroll) */
359	enum {
360	/* left scrollbar */
361	V2D_SCROLL_LEFT = (1 << 0),
362	V2D_SCROLL_RIGHT = (1 << 1),
363	V2D_SCROLL_VERTICAL = (V2D_SCROLL_LEFT \| V2D_SCROLL_RIGHT),

@Francesco Siddi (fsiddi) what do you think?

EDIT: Panning view is no longer blanking thumbnails. Only dynamic scaling will disable thumbnail drawing

I think we talked about similar issue with @Aditya Y Jeppu (quantimoney), where to prevent blanking thumbnails completely,old thumbnails would be displayed, but they would be blurred. I think this would provide quite pleasant transitions. If we have shaders, I think this would be possible to implement even without too drastic changes.

Very high level idea (so I don't forget) is: To avoid making runtime texture, have array with currently drawn image coords (frame, channel). Then if view is moved, get transformation from original view, get images from cache, apply it to coords from array, push transformed images to GPU, and blur.
That could look like this, but scaling view would look probably much worse.

@Aditya Y Jeppu (quantimoney) Just bit more feedback, I think otherwise this looks OK. I will look for drawing images transparent without overwriting it's buffer, but biggest issue is this blinking on Mac OS. Can you check if this is something you can reproduce on Linux?

source/blender/blenloader/intern/versioning_300.c
1235–1246	It should be enough to do versioning once...
source/blender/editors/space_sequencer/sequencer_draw.c
1618	Checking this again, I realized, that this really modifies original image, but unless you set GPU blending to `GPU_BLEND_ALPHA` it ignores alpha channel. So this could be done for each image, but probably best is to do `if (seq->flag & SEQ_OVERLAP && ibuf->rect[3] != OVERLAP_ALPHA) {` This will ensure, that alpha will be overwritten once and only when needed. But this is not best solution for things like transparent images, so really better would be to do this with shader or not at all. That brings question: how do we want to draw transparent images?

Was asked to test the temp-vse-thumbnail-mod branch. Things seem to be much better there than what you see in Francesco's video:

In D12266#324837, @Richard Antalik (ISS) wrote:

@Aditya Y Jeppu (quantimoney) Just bit more feedback, I think otherwise this looks OK. I will look for drawing images transparent without overwriting it's buffer, but biggest issue is this blinking on Mac OS. Can you check if this is something you can reproduce on Linux?

(Just noticed now) after adding your patch, it works for some time in the starting and then it stops. (here the first black thumb is not displayed in the starting, but later it is.) this is because of zooming using the mouse scroll wheel press+CTRL VS scrollbar / using scroll wheel to scroll gif here -

EDIT : just checked blender.chat will have to check this new branch out

source/blender/blenloader/intern/versioning_300.c
1235–1246	heh idk when that happened, there was some merge conflicts and maybe then it didn't remove.

In D12266#325027, @Aditya Y Jeppu (quantimoney) wrote:

(Just noticed now) after adding your patch, it works for some time in the starting and then it stops. (here the first black thumb is not displayed in the starting, but later it is.) this is because of zooming using the mouse scroll wheel press+CTRL VS scrollbar / using scroll wheel to scroll gif here -

I did not implement flag for zooming with scrollbars in P2373. Only for middle mouse button. It was mostly to quickly check if change works or not.

EDIT : just checked blender.chat will have to check this new branch out

Branch is just This patch with P2373

I can too reproduce flickering when scrolling with middle mouse due to different number of thumbnails available. I think, that this can be avoided only by rendering images sequentially. With very long strips this would be quite impractical unless time resolution is limited. So for example if only every 10th frame could be displayed, for 60min 60fps movie it would take only 6 minutes (here) to render all thumbnails. That is still quite a lot of time, also depends on source file. Also this would take only ~95MB in RAM. Downside is that with way cache is structured currently, when you split strips, you suddenly have 2 strips that are 60 minutes and images are not shared even though it is same source file - this aspect is not simple to change currently.

I guess best would be to have some kind of post-scroll timeout for thumbnails, which is not impossible to implement, but it sounds terrible. Even cycles with viewport renders looks quite glitchy like this.

After some testing together with @Richard Antalik (ISS) and @Julian Eisel (Severin) the recommendation is:

When transforming strips, transform the thumbnails currently visible so that they match the transform. In case of vertical downscaling, repeat the last thumbnail visible to fill the void. Overwrite only when new thumbnails are generated.
Generate the thumbnails and cache them in memory as much as possible (ideally retain some LOD for each strip for different zoom levels)

Minor issue:

When using the slip tool and cancelling the operation, thumbnails are unnecessarily re-rendered

In D12266#325319, @Francesco Siddi (fsiddi) wrote:

When transforming strips, transform the thumbnails currently visible so that they match the transform. In case of vertical downscaling, repeat the last thumbnail visible to fill the void. Overwrite only when new thumbnails are generated.

like this? temporarily I am just keeping a global variable to hold the old values of frame offset and the start frame to take images from cache. I did this yesterday, but it looked slightly comical / glitchy so thought maybe it was bad idea. This isn't perfectly implemented, but same idea is what we will be going towards?

OR keep it so that the thumbnails have the correct look, and just last frame is duplicated to fill everything up. The first way does look like the entire thing is getting stretched, and then it is "popped" to get the new size thumbnails.

Ok this seems to be better, and its a very simple fix too. I will go with this and add those changes to this diff. @Richard Antalik (ISS) about adding the v2d_navigating flag to the other ways to zoom, do you want to do it or should I add it if you are busy? I don't know the code there, and would have to see where to add the changes.

In D12266#325381, @Aditya Y Jeppu (quantimoney) wrote:

In D12266#325319, @Francesco Siddi (fsiddi) wrote:

When transforming strips, transform the thumbnails currently visible so that they match the transform. In case of vertical downscaling, repeat the last thumbnail visible to fill the void. Overwrite only when new thumbnails are generated.

like this? temporarily I am just keeping a global variable to hold the old values of frame offset and the start frame to take images from cache. I did this yesterday, but it looked slightly comical / glitchy so thought maybe it was bad idea. This isn't perfectly implemented, but same idea is what we will be going towards?

OR keep it so that the thumbnails have the correct look, and just last frame is duplicated to fill everything up. The first way does look like the entire thing is getting stretched, and then it is "popped" to get the new size thumbnails.

Yes, this looks like what we would want. Though it would be best if aspect ratio would be kept. Can you share code to this work? Perhaps we could setup branch for this, I made temp-vse-thumbnail-mod for some experimental stuff

In D12266#325389, @Aditya Y Jeppu (quantimoney) wrote:

Ok this seems to be better, and its a very simple fix too. I will go with this and add those changes to this diff. @Richard Antalik (ISS) about adding the v2d_navigating flag to the other ways to zoom, do you want to do it or should I add it if you are busy? I don't know the code there, and would have to see where to add the changes.

Not sure if I understand this comment, I assume, you are using v2d_navigating flag here right? Otherwise you wouldn't know that UI is being rescaled, so I think we definitely need it. So if you rely on that just include it with code samples you share, that would be probably best thing

In D12266#325434, @Richard Antalik (ISS) wrote:

Not sure if I understand this comment, I assume, you are using v2d_navigating flag here right? Otherwise you wouldn't know that UI is being rescaled, so I think we definitely need it. So if you rely on that just include it with code samples you share, that would be probably best thing

I mean for the other ways to zoom, ie scroll bar and using the middle mouse to scroll and zoom.
yeah i will clean up that code, right now it is hacky way of doing it. i could add it to the temp branch

In D12266#325462, @Aditya Y Jeppu (quantimoney) wrote:

In D12266#325434, @Richard Antalik (ISS) wrote:

Not sure if I understand this comment, I assume, you are using v2d_navigating flag here right? Otherwise you wouldn't know that UI is being rescaled, so I think we definitely need it. So if you rely on that just include it with code samples you share, that would be probably best thing

I mean for the other ways to zoom, ie scroll bar and using the middle mouse to scroll and zoom.
yeah i will clean up that code, right now it is hacky way of doing it. i could add it to the temp branch

Ah, right, I think only scrollbar zooming is missing the flag, it's mostly copy-paste work so if you don't add it I will. I wanted to have a look at this functionality later today and see how it works and what limitations are.

First thing that comes to mind is how to prevent those black bars when you zoom to smaller strip size. Yesterday we talked, that there could be some dynamic range switching, similar to vertical grid in timeline, so that you could have more images available when needed. and these don't have to be super precise.

Please never squash and stretch the thumbnails. Always keep the aspect ratio.

I think I found reasonable solution to get maximum number of thumbnails visible with minimum effort:

Zooming in: repeat the images from set of last displayed thumbnails.

Zooming out (I don't have this implemented yet): Any strip does have maximum number of thumbnails that can fit in view. Due to varying display resolution, this number is arbitrary. on 1920px wide display it is 30 thumbnails, so let's round it to 50. Therefore set of 50 images, equally spaces can be kept in memory.

Finally combining these 2 sets, a function can map requested frame to guaranteed frame.
Technically same method could be used for case if precise image is not rendered yet, so there would be no empty space when scrolling with mouse wheel. I will have to check ifwe need v2d flag then still, but I think it will look more solid with the flag.

@Francesco Siddi (fsiddi) does what you saw in first video look usable? With zoom out method I proposed it would look same way as in first video, just in reverse... Another question is if drawing looks OK

Pushed latest changes to temp-vse-thumbnail-mod branch, there is one issue where list of available thumbnails is not managed correctly and it causes thumbnail to be missed and then it constantly tries to start job.

Solution should be to calculate start frame before drawing loop starts and also manage list with correct end frame. Otherwise I think this looks quite solid.

I was workinng following change, but leaving it for tomorrow

P2396 (An Untitled Masterwork)

1	diff --git a/source/blender/editors/space_sequencer/sequencer_draw.c b/source/blender/editors/space_sequencer/sequencer_draw.c
2	index 7d9e424dc20..ae49a46b232 100644
3	--- a/source/blender/editors/space_sequencer/sequencer_draw.c
4	+++ b/source/blender/editors/space_sequencer/sequencer_draw.c
5	@@ -1492,21 +1492,20 @@ static void sequencer_thumbnail_init_job(const bContext C, View2D v2d, Editing
6	G.is_break = false;
7	WM_jobs_start(CTX_wm_manager(C), wm_job);
8	}
9	else {
10	WM_event_add_notifier(C, NC_SCENE \| ND_SEQUENCER, NULL);
11	}
12
13	ED_area_tag_redraw(area);
14	}
15
16	-
17	/* Don't display thumbnails only when zooming. Panning doesn't cause issues. */
18	static bool sequencer_thumbnail_v2d_is_navigating(const bContext *C)
19	{
20	ARegion *region = CTX_wm_region(C);
21	View2D *v2d = &region->v2d;
22	SpaceSeq *sseq = CTX_wm_space_seq(C);
23
24	if ((v2d->flag & V2D_IS_NAVIGATING) == 0) {
25	return false;
26	}
27	@@ -1642,61 +1641,64 @@ static void draw_seq_strip_thumbnail(View2D *v2d,
28
29	float cut_off = 0;
30	float upper_thumb_bound = (seq->endstill) ? (seq->start + seq->len) : seq->enddisp;
31	if (seq->type == SEQ_TYPE_IMAGE) {
32	upper_thumb_bound = seq->enddisp;
33	}
34
35	thumb_x_start = seq_thumbnail_get_start_frame(seq, thumb_width, &v2d->cur);
36	float thumb_x_end;
37
38	+ /* Checks to make sure that thumbs are loaded only when in view and within the confines of the
39	+ * strip. Some may not be required but better to have conditions for safety as x1 here is
40	+ * point to start caching from and not drawing. */
41	+ if (thumb_x_start > v2d->cur.xmax) {
42	+ return;
43	+ }
44	+
45	+ if (thumb_x_start + thumb_width < v2d->cur.xmin) {
46	+ float distance = v2d->cur.xmin - thumb_x_start + thumb_width;
47	+ thumb_x_start += (((int)(distance / thumb_width)) * thumb_width) - thumb_width;
48	+ }
49	+
50	+ /* Ignore thumbs to the left of strip. */
51	+ if (thumb_x_start + thumb_width < seq->startdisp) {
52	+ float distance = seq->startdisp - thumb_x_start + thumb_width;
53	+ thumb_x_start += (((int)(distance / thumb_width)) * thumb_width) - thumb_width;
54	+ }
55	+
56	GSet *last_displayed_thumbnails = last_displayed_thumbnails_list_ensure(C, seq);
57
58	/* Cleanup thumbnail list outside of rendered range, which is cleaned up one by one to prevent
59	* flickering after zooming. */
60	if (!sequencer_thumbnail_v2d_is_navigating(C)) {
61	last_displayed_thumbnails_list_cleanup(last_displayed_thumbnails, -FLT_MAX, thumb_x_start);
62	}
63
64	/* Start drawing. */
65	while (thumb_x_start < upper_thumb_bound) {
66	thumb_x_end = thumb_x_start + thumb_width;
67	clipped = false;
68
69	- /* Checks to make sure that thumbs are loaded only when in view and within the confines of the
70	- * strip. Some may not be required but better to have conditions for safety as x1 here is
71	- * point to start caching from and not drawing. */
72	- if (thumb_x_start > v2d->cur.xmax)
73	- break;
74	-
75	- if (thumb_x_end < v2d->cur.xmin) {
76	- thumb_x_start = thumb_x_end;
77	- continue;
78	- }
79	-
80	- /* Ignore thumbs to the left of strip. */
81	- if (thumb_x_end < seq->startdisp) {
82	- thumb_x_start = thumb_x_end;
83	- continue;
84	- }
85	-
86	/* Set the clipping bound to show the left handle moving over thumbs and not shift thumbs. */
87	if (IN_RANGE_INCL(seq->startdisp, thumb_x_start, thumb_x_end)) {
88	cut_off = seq->startdisp - thumb_x_start;
89	clipped = true;
90	}
91
92	/* Clip if full thumbnail cannot be displayed. */
93	if (thumb_x_end > (upper_thumb_bound)) {
94	thumb_x_end = upper_thumb_bound;
95	clipped = true;
96	if (thumb_x_end - thumb_x_start < 1)
97	+ // last_displayed_thumbnails_list_cleanup(last_displayed_thumbnails, thumb_x_start,
98	+ // FLT_MAX);
99	break;
100	}
101
102	float zoom_x = thumb_width / image_width;
103	float zoom_y = thumb_height / image_height;
104
105	float cropx_min = (cut_off / pixelx) / (zoom_y / pixely);
106	float cropx_max = ((thumb_x_end - thumb_x_start) / pixelx) / (zoom_y / pixely);
107	if (cropx_max == (thumb_x_end - thumb_x_start)) {
108	cropx_max = cropx_max + 1;
109	@@ -1763,21 +1765,22 @@ static void draw_seq_strip_thumbnail(View2D *v2d,
110	y1,
111	thumb_x_end,
112	thumb_y_end,
113	zoom_x,
114	zoom_y);
115	IMB_freeImBuf(ibuf);
116	GPU_blend(GPU_BLEND_NONE);
117	cut_off = 0;
118	thumb_x_start += thumb_width;
119	}
120	- last_displayed_thumbnails_list_cleanup(last_displayed_thumbnails, thumb_x_start - thumb_width, FLT_MAX);
121	+ last_displayed_thumbnails_list_cleanup(
122	+ last_displayed_thumbnails, thumb_x_start - thumb_width, FLT_MAX);
123	}
124
125	/* Draw visible strips. Bounds check are already made. */
126	static void draw_seq_strip(const bContext *C,
127	SpaceSeq *sseq,
128	Scene *scene,
129	ARegion *region,
130	Sequence *seq,
131	float pixelx,
132	bool seq_active)