Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

vo: Fix race condition causing redraw requests to be ignored #9735

Closed
wants to merge 1 commit into from
Closed

vo: Fix race condition causing redraw requests to be ignored #9735

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions video/out/vo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -938,12 +938,23 @@ static bool render_frame(struct vo *vo)
in->prev_vsync = now;
in->expecting_vsync = use_vsync;

bool prev_request_redraw = in->request_redraw;

if (in->dropped_frame) {
in->drop_count += 1;
wakeup_core(vo);
} else {
in->rendering = true;
in->hasframe_rendered = true;

// We have to clear the request_redraw flag now, even though we don't
// know whether we have successfully rendered the requested frame yet.
// This is so that we can detect new redraw requests whilst we're
// rendering the frame with the lock relinquished. If we later find
// that we had to drop this frame, we set redraw_request flag again if
// it was previously set (see prev_request_redraw).
in->request_redraw = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clears in->request_redraw unconditionally, but originally it was only cleared if the frame was not dropped. I think it's a (new) bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true, see line 941.

Copy link
Member

@avih avih Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes true. In (new) line 988 (after in->request_redraw is cleared with the patch, and before it originally checked in->dropped_frame to decide whether to clear) it can change:

    in->dropped_frame = prev_drop_count < vo->in->drop_count;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're totally right, I didn't spot that. Thanks a lot. I force-pushed a change that I believe addresses this. The logic feels very subtle so I tried to explain what's going on by adding a comment to the code.


int64_t prev_drop_count = vo->in->drop_count;
// Can the core queue new video now? Non-display-sync uses a separate
// timer instead, but possibly benefits from preparing a frame early.
Expand Down Expand Up @@ -997,8 +1008,7 @@ static bool render_frame(struct vo *vo)

if (in->dropped_frame) {
MP_STATS(vo, "drop-vo");
} else {
in->request_redraw = false;
in->request_redraw |= prev_request_redraw;
}

if (in->current_frame && in->current_frame->num_vsyncs &&
Expand Down