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

drm: use VOCTRL_REDRAW when flipping buffers #15949

Merged
merged 2 commits into from
Feb 23, 2025
Merged

drm: use VOCTRL_REDRAW when flipping buffers #15949

merged 2 commits into from
Feb 23, 2025
+10 −34

Conversation

Dudemanguy
Copy link
Member

@Dudemanguy Dudemanguy commented Feb 23, 2025 •
edited
Loading

I changed my mind. These are the first two commits from #15948 extracted to here since it's just a plain better solution and fixes 0959cde

@Dudemanguy
vo: signal VOCTRL_REDRAW to backends
e6d8106 
Realistically this is for drm_egl, but instead of overriding
submit_frame, it can use this event instead to know that it should
advance its buffers.
@Dudemanguy Dudemanguy force-pushed the fix-vulkan-force-window branch from 4a8e262 to 1efe183 Compare February 23, 2025 21:08
@Dudemanguy Dudemanguy changed the title vulkan/context: no-op on submit frame for vo_gpu_next drm: use VOCTRL_REDRAW when flipping buffers Feb 23, 2025
@Dudemanguy
drm: use VOCTRL_REDRAW when flipping buffers
81e744e 
Basically whenever the VO does a redraw, we always have to flip buffers
for vo_drm and drm_egl. The old way of doing this was checking if the
video was paused and if the frame was still. This does work, but what
really needs to be known is simply if the VO is redrawing. Since the
added VOCTRL_REDRAW tells us this, we can shuffle around some logic and
simplify greatly. drm_egl no longer needs its custom override for the
submit_frame step so that can be eliminated along with the vo_gpu_next
hack.

Fixes 0959cde.
@Dudemanguy Dudemanguy force-pushed the fix-vulkan-force-window branch from 1efe183 to 81e744e Compare February 23, 2025 21:10
@github-actions GitHub Actions
Copy link

Download the artifacts for this pull request:

Windows
macOS

@Dudemanguy Dudemanguy merged commit 1bcc76d into mpv-player:master Feb 23, 2025
26 checks passed
@Dudemanguy Dudemanguy deleted the fix-vulkan-force-window branch February 23, 2025 22:01
@kasper93
Copy link
Contributor

kasper93 commented Feb 25, 2025 •
edited
Loading

Sorry about swiping two commits before you had a chance to review. Wanted to fix breakage I caused.

After merge of this PR still images are breaking in mpv when resizing the window for example:

  1. mpv -v foo.jpg --no-config --keep-open --vo=gpu --gpu-api=d3d11

    Flashing black screen on window resize.

  2. mpv -v foo.jpg --no-config --keep-open --vo=gpu-next --gpu-api=d3d11

[vo/gpu-next/d3d11] Couldn't resize swapchain: The application made a call that is invalid. Either the parameters of the call or the state of some object was incorrect.
...
[vo/gpu-next/libplacebo] Tried resizing the swapchain while a frame was in progress! Please submit the current frame first.
  1. mpv -v foo.jpg --no-config --keep-open --vo=gpu-next --gpu-api=vulkan
[vo/gpu-next/libplacebo] Attempting to hold an already held image!
[vo/gpu-next/libplacebo] Failed holding swapchain image for presentation
[vo/gpu-next] Failed presenting frame!

@Dudemanguy
Copy link
Member Author

Are you sure? I can understand that happening with 0959cde but this PR removed all of that code.

@kasper93
Copy link
Contributor

kasper93 commented Feb 25, 2025 •
edited
Loading

fc9d1ca is the first bad commit for d3d11. (as described in #15949 (comment))

0959cde is the first bad commit for Vulkan. (Assertion p->last_imgidx >= 0 failed.)

Both are broken in current version, as described in #15949 (comment).

@Dudemanguy
Copy link
Member Author

I don't get that behavior on vulkan on my end on wayland or x11 though. Hence my confusion. I know what caused the assertion error which is why I removed the offending line in vo_gpu_next so it should be exactly the same as before.

fc9d1ca is the first bad commit for d3d11.

If so, then that means that VOCTRL is somehow not a no-op on Windows? That seems weird though.

@Dudemanguy
Copy link
Member Author

I guess it's this?

mpv/video/out/w32_common.c

Lines 2412 to 2416 in 5459b0f

// Safe access, since caller (owner of vo) is blocked.
if (*events & VO_EVENT_RESIZE) {
w32->vo->dwidth = rect_w(w32->windowrc);
w32->vo->dheight = rect_h(w32->windowrc);
}

Isn't that redundant with vo_w32_control?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Dudemanguy @kasper93