Page MenuHome
Differential D13043 Diff 44271 source/blender/editors/space_sequencer/sequencer_thumbnails.c

Changeset View

Standalone View

View Options

source/blender/editors/space_sequencer/sequencer_thumbnails.c

Show First 20 Lines • Show All 49 Lines • ▼ Show 20 Lines
typedef struct ThumbnailDrawJob { typedef struct ThumbnailDrawJob {
SeqRenderData context; SeqRenderData context;
GHash *sequences_ghash; GHash *sequences_ghash;
Scene *scene; Scene *scene;
rctf *view_area; rctf *view_area;
float pixelx; float pixelx;
float pixely; float pixely;
float thumb_height;
} ThumbnailDrawJob; } ThumbnailDrawJob;
typedef struct ThumbDataItem { typedef struct ThumbDataItem {
Sequence *seq_dupli; Sequence *seq_dupli;
Scene *scene; Scene *scene;
} ThumbDataItem; } ThumbDataItem;
static void thumbnail_hash_data_free(void *val) static void thumbnail_hash_data_free(void *val)
Show All 36 Linesstatic bool check_seq_need_thumbnails(Sequence *seq, rctf *view_area)
} }
return true; return true;
} }
static void seq_get_thumb_image_dimensions(Sequence *seq, static void seq_get_thumb_image_dimensions(Sequence *seq,
float pixelx, float pixelx,
float pixely, float pixely,
float thumb_height,
float *r_thumb_width, float *r_thumb_width,
float *r_thumb_height, float *r_thumb_height,
ISSUnsubmitted
Not Done
Inline Actions

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.

ISS: It's very weird to have this return variable then, it should be removed. Could be simplified…
float *r_image_width, float *r_image_width,
float *r_image_height) float *r_image_height)
{ {
float image_width = seq->strip->stripdata->orig_width; float image_width = seq->strip->stripdata->orig_width;
float image_height = seq->strip->stripdata->orig_height; float image_height = seq->strip->stripdata->orig_height;
/* Fix the dimensions to be max SEQ_RENDER_THUMB_SIZE (256) for x or y. */ /* Fix the dimensions to be max SEQ_RENDER_THUMB_SIZE (256) for x or y. */
float aspect_ratio = (float)image_width / image_height; float aspect_ratio = (float)image_width / image_height;
if (image_width > image_height) { if (image_width > image_height) {
image_width = SEQ_RENDER_THUMB_SIZE; image_width = SEQ_RENDER_THUMB_SIZE;
image_height = round_fl_to_int(image_width / aspect_ratio); image_height = round_fl_to_int(image_width / aspect_ratio);
} }
else { else {
image_height = SEQ_RENDER_THUMB_SIZE; image_height = SEQ_RENDER_THUMB_SIZE;
image_width = round_fl_to_int(image_height * aspect_ratio); image_width = round_fl_to_int(image_height * aspect_ratio);
} }
/* Calculate thumb dimensions. */ /* Calculate thumb dimensions. */
float thumb_height = (SEQ_STRIP_OFSTOP - SEQ_STRIP_OFSBOTTOM) - (20 * U.dpi_fac * pixely);
aspect_ratio = ((float)image_width) / image_height; aspect_ratio = ((float)image_width) / image_height;
becoAuthorUnsubmitted
Done
Inline Actions

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.

beco: I am not really happy with this change, I would prefer to retrieve the `y2 - y1` range without…
ISSUnsubmitted
Not Done
Inline Actions

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.

ISS: Getting values from running job is quite bad idea - it can have outdated information.
float thumb_h_px = thumb_height / pixely; float thumb_h_px = thumb_height / pixely;
float thumb_width = aspect_ratio * thumb_h_px * pixelx; float thumb_width = aspect_ratio * thumb_h_px * pixelx;
if (r_thumb_height == NULL) { if (r_thumb_height == NULL) {
*r_thumb_width = thumb_width; *r_thumb_width = thumb_width;
return; return;
} }
Show All 29 Linesstatic void thumbnail_start_job(void *data,
/* First pass: render visible images. */ /* First pass: render visible images. */
BLI_ghashIterator_init(&gh_iter, tj->sequences_ghash); BLI_ghashIterator_init(&gh_iter, tj->sequences_ghash);
while (!BLI_ghashIterator_done(&gh_iter) & !*stop) { while (!BLI_ghashIterator_done(&gh_iter) & !*stop) {
Sequence *seq_orig = BLI_ghashIterator_getKey(&gh_iter); Sequence *seq_orig = BLI_ghashIterator_getKey(&gh_iter);
ThumbDataItem *val = BLI_ghash_lookup(tj->sequences_ghash, seq_orig); ThumbDataItem *val = BLI_ghash_lookup(tj->sequences_ghash, seq_orig);
if (check_seq_need_thumbnails(seq_orig, tj->view_area)) { if (check_seq_need_thumbnails(seq_orig, tj->view_area)) {
seq_get_thumb_image_dimensions( seq_get_thumb_image_dimensions(
val->seq_dupli, tj->pixelx, tj->pixely, &frame_step, NULL, NULL, NULL); val->seq_dupli, tj->pixelx, tj->pixely, tj->thumb_height, &frame_step, NULL, NULL, NULL);
start_frame = seq_thumbnail_get_start_frame(seq_orig, frame_step, tj->view_area); start_frame = seq_thumbnail_get_start_frame(seq_orig, frame_step, tj->view_area);
SEQ_render_thumbnails( SEQ_render_thumbnails(
&tj->context, val->seq_dupli, seq_orig, start_frame, frame_step, tj->view_area, stop); &tj->context, val->seq_dupli, seq_orig, start_frame, frame_step, tj->view_area, stop);
SEQ_relations_sequence_free_anim(val->seq_dupli); SEQ_relations_sequence_free_anim(val->seq_dupli);
} }
BLI_ghashIterator_step(&gh_iter); BLI_ghashIterator_step(&gh_iter);
} }
/* Second pass: render "guaranteed" set of images. */ /* Second pass: render "guaranteed" set of images. */
BLI_ghashIterator_init(&gh_iter, tj->sequences_ghash); BLI_ghashIterator_init(&gh_iter, tj->sequences_ghash);
while (!BLI_ghashIterator_done(&gh_iter) & !*stop) { while (!BLI_ghashIterator_done(&gh_iter) & !*stop) {
Sequence *seq_orig = BLI_ghashIterator_getKey(&gh_iter); Sequence *seq_orig = BLI_ghashIterator_getKey(&gh_iter);
ThumbDataItem *val = BLI_ghash_lookup(tj->sequences_ghash, seq_orig); ThumbDataItem *val = BLI_ghash_lookup(tj->sequences_ghash, seq_orig);
if (check_seq_need_thumbnails(seq_orig, tj->view_area)) { if (check_seq_need_thumbnails(seq_orig, tj->view_area)) {
seq_get_thumb_image_dimensions( seq_get_thumb_image_dimensions(
val->seq_dupli, tj->pixelx, tj->pixely, &frame_step, NULL, NULL, NULL); val->seq_dupli, tj->pixelx, tj->pixely, tj->thumb_height, &frame_step, NULL, NULL, NULL);
start_frame = seq_thumbnail_get_start_frame(seq_orig, frame_step, tj->view_area); start_frame = seq_thumbnail_get_start_frame(seq_orig, frame_step, tj->view_area);
SEQ_render_thumbnails_base_set(&tj->context, val->seq_dupli, seq_orig, tj->view_area, stop); SEQ_render_thumbnails_base_set(&tj->context, val->seq_dupli, seq_orig, tj->view_area, stop);
SEQ_relations_sequence_free_anim(val->seq_dupli); SEQ_relations_sequence_free_anim(val->seq_dupli);
} }
BLI_ghashIterator_step(&gh_iter); BLI_ghashIterator_step(&gh_iter);
} }
} }
Show All 35 Lines else {
val_need_update->seq_dupli->startdisp = seq->startdisp; val_need_update->seq_dupli->startdisp = seq->startdisp;
} }
} }
} }
return thumb_data_hash; return thumb_data_hash;
} }
static void sequencer_thumbnail_init_job(const bContext *C, View2D *v2d, Editing *ed) static void sequencer_thumbnail_init_job(const bContext *C,
View2D *v2d,
Editing *ed,
float thumb_height)
{ {
wmJob *wm_job; wmJob *wm_job;
ThumbnailDrawJob *tj = NULL; ThumbnailDrawJob *tj = NULL;
ScrArea *area = CTX_wm_area(C); ScrArea *area = CTX_wm_area(C);
wm_job = WM_jobs_get(CTX_wm_manager(C), wm_job = WM_jobs_get(CTX_wm_manager(C),
CTX_wm_window(C), CTX_wm_window(C),
CTX_data_scene(C), CTX_data_scene(C),
"Draw Thumbnails", "Draw Thumbnails",
Show All 13 Lines if (!tj) {
view_area->ymin = v2d->cur.ymin; view_area->ymin = v2d->cur.ymin;
tj->scene = CTX_data_scene(C); tj->scene = CTX_data_scene(C);
tj->view_area = view_area; tj->view_area = view_area;
tj->context = sequencer_thumbnail_context_init(C); tj->context = sequencer_thumbnail_context_init(C);
tj->sequences_ghash = sequencer_thumbnail_ghash_init(C, v2d, ed); tj->sequences_ghash = sequencer_thumbnail_ghash_init(C, v2d, ed);
tj->pixelx = BLI_rctf_size_x(&v2d->cur) / BLI_rcti_size_x(&v2d->mask); tj->pixelx = BLI_rctf_size_x(&v2d->cur) / BLI_rcti_size_x(&v2d->mask);
tj->pixely = BLI_rctf_size_y(&v2d->cur) / BLI_rcti_size_y(&v2d->mask); tj->pixely = BLI_rctf_size_y(&v2d->cur) / BLI_rcti_size_y(&v2d->mask);
tj->thumb_height = thumb_height;
WM_jobs_customdata_set(wm_job, tj, thumbnail_freejob); WM_jobs_customdata_set(wm_job, tj, thumbnail_freejob);
WM_jobs_timer(wm_job, 0.1, NC_SCENE | ND_SEQUENCER, NC_SCENE | ND_SEQUENCER); WM_jobs_timer(wm_job, 0.1, NC_SCENE | ND_SEQUENCER, NC_SCENE | ND_SEQUENCER);
WM_jobs_callbacks(wm_job, thumbnail_start_job, NULL, NULL, thumbnail_endjob); WM_jobs_callbacks(wm_job, thumbnail_start_job, NULL, NULL, thumbnail_endjob);
} }
if (!WM_jobs_is_running(wm_job)) { if (!WM_jobs_is_running(wm_job)) {
G.is_break = false; G.is_break = false;
WM_jobs_start(CTX_wm_manager(C), wm_job); WM_jobs_start(CTX_wm_manager(C), wm_job);
} }
else { else {
WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, NULL); WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, NULL);
} }
ED_area_tag_redraw(area); ED_area_tag_redraw(area);
} }
static bool sequencer_thumbnail_v2d_is_navigating(const bContext *C) static bool sequencer_thumbnail_v2d_is_navigating(const bContext *C)
{ {
ARegion *region = CTX_wm_region(C); ARegion *region = CTX_wm_region(C);
View2D *v2d = &region->v2d; View2D *v2d = &region->v2d;
return (v2d->flag & V2D_IS_NAVIGATING) != 0; return (v2d->flag & V2D_IS_NAVIGATING) != 0;
} }
static void sequencer_thumbnail_start_job_if_necessary(const bContext *C, static void sequencer_thumbnail_start_job_if_necessary(
Editing *ed, const bContext *C, Editing *ed, View2D *v2d, bool thumbnail_is_missing, float thumb_height)
View2D *v2d,
bool thumbnail_is_missing)
{ {
SpaceSeq *sseq = CTX_wm_space_seq(C); SpaceSeq *sseq = CTX_wm_space_seq(C);
if (sequencer_thumbnail_v2d_is_navigating(C)) { if (sequencer_thumbnail_v2d_is_navigating(C)) {
WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, NULL); WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, NULL);
return; return;
} }
Show All 10 Linesstatic void sequencer_thumbnail_start_job_if_necessary(
} }
/* Stop the job first as view has changed. Pointless to continue old job. */ /* Stop the job first as view has changed. Pointless to continue old job. */
if (v2d->cur.xmax != sseq->runtime.last_thumbnail_area.xmax || if (v2d->cur.xmax != sseq->runtime.last_thumbnail_area.xmax ||
v2d->cur.ymax != sseq->runtime.last_thumbnail_area.ymax) { v2d->cur.ymax != sseq->runtime.last_thumbnail_area.ymax) {
WM_jobs_stop(CTX_wm_manager(C), NULL, thumbnail_start_job); WM_jobs_stop(CTX_wm_manager(C), NULL, thumbnail_start_job);
} }
sequencer_thumbnail_init_job(C, v2d, ed); sequencer_thumbnail_init_job(C, v2d, ed, thumb_height);
sseq->runtime.last_thumbnail_area = v2d->cur; sseq->runtime.last_thumbnail_area = v2d->cur;
} }
void last_displayed_thumbnails_list_free(void *val) void last_displayed_thumbnails_list_free(void *val)
{ {
BLI_gset_free(val, NULL); BLI_gset_free(val, NULL);
} }
▲ Show 20 Lines • Show All 122 Lines • ▼ Show 20 Linesvoid draw_seq_strip_thumbnail(View2D *v2d,
SeqRenderData context = sequencer_thumbnail_context_init(C); SeqRenderData context = sequencer_thumbnail_context_init(C);
if ((seq->flag & SEQ_FLAG_SKIP_THUMBNAILS) != 0) { if ((seq->flag & SEQ_FLAG_SKIP_THUMBNAILS) != 0) {
return; return;
} }
seq_get_thumb_image_dimensions( seq_get_thumb_image_dimensions(
seq, pixelx, pixely, &thumb_width, &thumb_height, &image_width, &image_height); seq, pixelx, pixely, y2 - y1, &thumb_width, &thumb_height, &image_width, &image_height);
ISSUnsubmitted
Not Done
Inline Actions

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.

ISS: I would declare `const float thumb_height = y2 - y1;` here, and remove argument from…
float thumb_y_end = y1 + thumb_height - pixely; float thumb_y_end = y1 + thumb_height;
becoAuthorUnsubmitted
Done
Inline Actions

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.

beco: I am not sure this change is right. I could not understand why we had that `- pixely` in the…
ISSUnsubmitted
Not Done
Inline Actions

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

ISS: It looks like 1px offset being applied to height on top of thumbnail. Not quite sure why, but…
float cut_off = 0; float cut_off = 0;
float upper_thumb_bound = (seq->endstill) ? (seq->start + seq->len) : seq->enddisp; float upper_thumb_bound = (seq->endstill) ? (seq->start + seq->len) : seq->enddisp;
if (seq->type == SEQ_TYPE_IMAGE) { if (seq->type == SEQ_TYPE_IMAGE) {
upper_thumb_bound = seq->enddisp; upper_thumb_bound = seq->enddisp;
} }
float thumb_x_start = seq_thumbnail_get_start_frame(seq, thumb_width, &v2d->cur); float thumb_x_start = seq_thumbnail_get_start_frame(seq, thumb_width, &v2d->cur);
▲ Show 20 Lines • Show All 53 Lines • ▼ Show 20 Lines while (thumb_x_start < upper_thumb_bound) {
BLI_rcti_init(&crop, (int)(cropx_min), (int)cropx_max, 0, (int)(image_height)-1); BLI_rcti_init(&crop, (int)(cropx_min), (int)cropx_max, 0, (int)(image_height)-1);
int timeline_frame = round_fl_to_int(thumb_x_start); int timeline_frame = round_fl_to_int(thumb_x_start);
/* Get the image. */ /* Get the image. */
ImBuf *ibuf = SEQ_get_thumbnail(&context, seq, timeline_frame, &crop, clipped); ImBuf *ibuf = SEQ_get_thumbnail(&context, seq, timeline_frame, &crop, clipped);
if (!ibuf) { if (!ibuf) {
sequencer_thumbnail_start_job_if_necessary(C, scene->ed, v2d, true); sequencer_thumbnail_start_job_if_necessary(C, scene->ed, v2d, true, y2 - y1);
ibuf = sequencer_thumbnail_closest_from_memory( ibuf = sequencer_thumbnail_closest_from_memory(
&context, seq, timeline_frame, last_displayed_thumbnails, &crop, clipped); &context, seq, timeline_frame, last_displayed_thumbnails, &crop, clipped);
} }
/* Store recently rendered frames, so they can be reused when zooming. */ /* Store recently rendered frames, so they can be reused when zooming. */
else if (!sequencer_thumbnail_v2d_is_navigating(C)) { else if (!sequencer_thumbnail_v2d_is_navigating(C)) {
/* Clear images in frame range occupied by new thumbnail. */ /* Clear images in frame range occupied by new thumbnail. */
last_displayed_thumbnails_list_cleanup( last_displayed_thumbnails_list_cleanup(
▲ Show 20 Lines • Show All 45 Lines • Show Last 20 Lines