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

video/out/hwdec/hwdec_vaapi: Remove unnecessary explicit synchronization #12980

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

langyuxf
Copy link

Implicit synchronization will handle that.
Extra explicit synchronization here would bring some overheads.

Read this before you submit this pull request:
https://github.com/mpv-player/mpv/blob/master/DOCS/contribute.md

Reading this link and following the rules will get your pull request reviewed
and merged faster. Nobody wants lazy pull requests.

Implicit synchronization will handle that.
Extra explicit synchronization here would bring some overheads.

Signed-off-by: Lang Yu <[email protected]>
@philipl
Copy link
Member

philipl commented Nov 27, 2023

The documentation for vaExportSurfaceHandle explicitly states:

This does not perform any synchronisation. If the contents of the surface will be read, vaSyncSurface() must be called before doing so. If the contents of the surface are written, then all operations must be completed externally before using the surface again by via VA-API functions.

@philipl
Copy link
Member

philipl commented Nov 27, 2023

We've also empirically observed synchronisation problems (displaying old frames) without the sync call.

@langyuxf
Copy link
Author

The documentation for vaExportSurfaceHandle explicitly states:

This does not perform any synchronisation. If the contents of the surface will be read, vaSyncSurface() must be called before doing so. If the contents of the surface are written, then all operations must be completed externally before using the surface again by via VA-API functions.

My understanding is it's the consumer's responsibility to do the sync.
If the consumer is GL, actually it would be implicitly synced in kernel mode driver.
For consumers can't do implicitly sync, vaSyncSurface() is necessary.

vaSyncSurface() for decode and process is not implemented in Mesa until https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20133

@philipl
Copy link
Member

philipl commented Nov 27, 2023

That MR is only relevant for AMD. vaSyncSurface has been meaningful on Intel for much longer. Also remember that we have to support importing to Vulkan, which most assuredly will not do implicit sync - and without a sync object to import (ie: a semaphore) there is no way to make it sync - which is the situation in which I observed the incorrect behaviour mentioned previously.

@langyuxf
Copy link
Author

Ok, it would be better to do the sync in consumer side(e.g., GL, Vulkan) instead of producer side.

@langyuxf
Copy link
Author

As far as I know, if the surface is exported as a dma-buf, it will be implicitly synced when imported and mapped either by GL or Vulkan in Linux KMD.

@philipl
Copy link
Member

philipl commented Nov 27, 2023

If you can point us to either the code or developer statements that guarantee this behaviour for both GL and Vulkan and Intel and AMD, then we can consider it. But as I've said, the only evidence I have, is that things look wrong in at least one combination without it.

@langyuxf
Copy link
Author

langyuxf commented Nov 27, 2023

Please take a look at this https://docs.kernel.org/next/driver-api/dma-buf.html#c.dma_buf_export_sync_file.
This is for Vulkan to do explicitly sync.

Copy link

Download the artifacts for this pull request:

Windows

@philipl
Copy link
Member

philipl commented Nov 27, 2023

Ok. If you want to update the PR to include the necessary sync parts, then we can consider merging it.

@langyuxf
Copy link
Author

Thanks, let me have a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants