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.

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

vo_opengl: fix blue screen issues and image stalls #3773

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@haasn
Copy link
Member

commented Nov 6, 2016

The logic seems to have been flipped around for some reason. (Judging by
the comment, the intent was to force a redraw when using blend-subtitles
so that a changed subtitle would still register on a redrawn frame.
Although I doubt the legitimacy of this intent, this fix simply makes
the logic match the comment)

The second check that was in gl_video_upload_image was removed because
it was self-defeating: by duplicating the check from is_new, it skipped
the upload entirely even in cases where it was necessary (e.g. due to
is_new being set to true by this branch, or by the output FBO being
invalid). The skip logic is supposed to be handled in
gl_video_render_frame, not gl_video_upload_image.

Fixes #3758 and #3764.

vo_opengl: fix blue screen issues and image stalls
The logic seems to have been flipped around for some reason. (Judging by
the comment, the intent was to force a redraw when using blend-subtitles
so that a changed subtitle would still register on a redrawn frame.
Although I doubt the legitimacy of this intent, this fix simply makes
the logic match the comment)

The second check that was in gl_video_upload_image was removed because
it was self-defeating: by duplicating the check from is_new, it skipped
the upload entirely even in cases where it was necessary (e.g. due to
is_new being set to true by this branch, or by the output FBO being
invalid). The skip logic is supposed to be handled in
gl_video_render_frame, not gl_video_upload_image.

Fixes #3758 and #3764.
@haasn

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2016

By the way, as mentioned, I'm pretty sure that check for blend_subs etc. is redundant; because the only thing blended by the blend_video code path is stuff that's dependent on the video frame itself (i.e. actual subtitles, not OSD). The OSD always gets drawn at screen res at the end even with blend_subs enabled.

Since the only case where the video gets repeated is in the case where the same video frame gets shown twice, I can't imagine a scenario in which we'd need to flush the cache for subtitles.

But it could be that I'm misunderstanding the intent of that original check. (That said, whatever the intent was, it doesn't seem to be implemented correctly at any rate)

@wm4

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

it skipped the upload entirely even in cases where it was necessary (e.g. due to
is_new being set to true by this branch, or by the output FBO being
invalid).

The upload isn't necessary in these cases, and that's why it was done this way. We actually want to try hard to avoid redundant uploads even in corner cases, because cover art mode is one big corner case (where reuploading might cause noticeable delays).

The thing is just that hardware decoding mode "unmaps" the textures after rendering, so trying to use the textures agains without re-"upload" will not work.

By the way, as mentioned, I'm pretty sure that check for blend_subs etc. is redundant; because the only thing blended by the blend_video code path is stuff that's dependent on the video frame itself (i.e. actual subtitles, not OSD). The OSD always gets drawn at screen res at the end even with blend_subs enabled.

Probably true, since vo.c now resets the repeat flag properly on redraws. So feel free to drop that code. In theory it's still needed for frames which are very long: i.e. frames repeated for longer than an acceptable UI response delay - if all frames have repeat=true set, the subs would never be redrawn, even if things like changing subtitle position would require this. But you could argue that vo.c or the core should force redrawing by unsetting repeat once in a while. So feel free to remove this code.

@wm4

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

Actually, forget the latter part. I think my discussion applies mostly to the behavior according to your interpretation. But the intention was actually completely different. This code was supposed to trigger always redrawing the frame it blend_subs is used - except when a frame is repeated. Repeated frames should still be cheap, because forcing subtitle redrawing would be expensive for not much gain. So I think the original condition in the code is correct, but it should have always set is_new = true;.

@haasn

This comment has been minimized.

Copy link
Member Author

commented Nov 7, 2016

This code was supposed to trigger always redrawing the frame it blend_subs is used

I think we need to clarify something - what is the relationship between frame->repeat and is_new normally? I think this relationship is somewhat muddy and also perhaps somewhat incorrectly handled.

When the user is watching a hypothetical low-FPS video and changes a subtitle setting, would the frame ID stay the same or be incremented? And what would happen to the frame->repeat flag?

I think that maybe the intent was to use the frame ID to distinguish actual source pictures (a change of which would always trigger a redraw) and use the frame->repeat flag to signal a required re-render of the same image (without requiring a re-upload)

In this case, I think that maybe frame->repeat ought to be renamed to frame->force_redraw and set to true where the calling code wants to force a redraw of the same frame ID, rather than being false when we want to deny a same ID implying a redraw.

Either way, I guess that means we want to use a logic like this:

bool upload_needed = frame->id != last_id;
bool redraw_needed = upload_needed || frame->force_redraw;

if (upload_needed) {
    upload(frame); // don't need to pass the ID here, since the upload function doesn't really need to care
    last_id = frame->id;
}

if (redraw_needed) {
    hwdec_map(); // handle this in a separate function instead of doing it inside upload(), because we need to remap even identical frames (#3764)
                 // NOTE: or perhaps we can instead delay the unmap? not sure about this code
    render();
}

Also, I think mixing the question of whether or not a given setting changing requires a redraw or not into the frame drawing logic is a bit wrong; instead the interaction should be something like the player code telling the VO that it needs to redraw subtitles, and the VO (vo_opengl) deciding what to do based on that specific information. (Because only the VO knows whether the subtitle result is cached or not)

wm4 pushed a commit that referenced this pull request Nov 7, 2016

vo_opengl: fix redrawing with hardware decoding
unmap_current_image() is called after rendering. This essentially
invalidates the textures, so we can't assume that the image is still
present.

Also see PR #3773.

wm4 pushed a commit that referenced this pull request Nov 7, 2016

wm4
vo_opengl: fix --blend-subtitles handling
The intention was that if --blend-subtitles is enabled, the frame should
always be re-rendered instead of using e.g. a cached scaled frame. The
reason is that subtitles can change anyway, e.g. if you pause and change
subtitle size and such.

On the other hand, if the frame is marked as repeated, it should always
use the cached copy. Actually "simplify" this and drop the cache only if
playback is paused (which frame->still indicates indirectly).

Also see PR #3773.
@wm4

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2016

The logic is already somewhat separate, because the upload function checks the frame ID. It just happens that the upload function is also called on the redraw path.

I think we need to clarify something - what is the relationship between frame->repeat and is_new normally? I think this relationship is somewhat muddy and also perhaps somewhat incorrectly handled.

Yes, many things are muddy, software is a mudbath.

When the user is watching a hypothetical low-FPS video and changes a subtitle setting, would the frame ID stay the same or be incremented? And what would happen to the frame->repeat flag?

The frame ID is essentially connected to the frame contents. If we were webdevs, we'd use a md5sum of the frame's bitmap data instead.

The repeat flag is essentially an older way to signal that the frame data didn't change. But it also can have other purposes, such as signaling that drawing an exact duplicate is ok. So the repeat flag isn't only about the frame contents, but also other things that could influence final output, like OSD.

I think that maybe the intent was to use the frame ID to distinguish actual source pictures (a change of which would always trigger a redraw) and use the frame->repeat flag to signal a required re-render of the same image (without requiring a re-upload)

Yes, except that the repeat flag is the opposite.

In this case, I think that maybe frame->repeat ought to be renamed to frame->force_redraw and set to true where the calling code wants to force a redraw of the same frame ID, rather than being false when we want to deny a same ID implying a redraw.

Yes, I'm not happy with those vo_frame flags. They all seem so specific and corner-caseish.

wm4 pushed a commit that referenced this pull request Nov 7, 2016

wm4
vo_opengl: fix redrawing with hardware decoding
unmap_current_image() is called after rendering. This essentially
invalidates the textures, so we can't assume that the image is still
present.

Also see PR #3773.

wm4 pushed a commit that referenced this pull request Nov 7, 2016

wm4
vo_opengl: fix --blend-subtitles handling
The intention was that if --blend-subtitles is enabled, the frame should
always be re-rendered instead of using e.g. a cached scaled frame. The
reason is that subtitles can change anyway, e.g. if you pause and change
subtitle size and such.

On the other hand, if the frame is marked as repeated, it should always
use the cached copy. Actually "simplify" this and drop the cache only if
playback is paused (which frame->still indicates indirectly).

Also see PR #3773.
@wm4

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2016

Seems this is completely obsoleted by my changes.

@wm4 wm4 closed this Nov 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.