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: hwdec: fallback to hw_ingmtfmt match if no hw_device matches #15586

Closed
wants to merge 1 commit into from

Conversation

hbiyik
Copy link

@hbiyik hbiyik commented Dec 28, 2024

with the introduction of looking up hwcodecs the AV_HWDEVICE_TYPE support for hwdec=rkmpp is broken.

Rkmpp is a downstream bsp specific acceleration mechanism for rockchip based boards, by design it is nothing different than an DRM_PRIME based ffmpeg accelerator, and it handles hw_device_context by itself without any client opeartion if not allrady hw_context is not applied.

ffmpeg-rkmpp is being used by jellyfin intensively, and has a lot of users around it but can not be merged to mainline ffmpeg at the time being, but mpv was being used together with thats to mpv's modular design.

without this commit --hwdec=rkmpp will fail simply due to the fact that AV_HWDEVICE_TYPE_RKMPP is a downstream definition and can not be a part of mpv in a maintainable way.

This commit allows to fallback to matching algo to an hwdec where only hw_imgfmt matches.

I would appreciate if this commit gets merged, but i would also understand if you wouldn't like to as well.

PS: i noticed that i have made a typo in commit message, i can fix it gets ok to merge.

Copy link

github-actions bot commented Dec 28, 2024

Download the artifacts for this pull request:

Windows
macOS

@philipl
Copy link
Member

philipl commented Dec 28, 2024

Why is rkmpp still relevant? There is upstream support for the hardware via the v4l2request API and even though that also isn't in upstream ffmpeg, if you're prepared to use an ffmpeg fork, why not use a different one?

@hbiyik
Copy link
Author

hbiyik commented Dec 28, 2024

v4l2 requests support is very limited, it is not making use of dual cores of h264/hevc, so speed is crippled, vp9 is non-existent, there no afbc support to make use of high bitrate decoding in neither of av1/h26x. So we want to use requests, but the thing delivered is just for playing 1080p big buck bunny.

ah: also there is no 10bit hdr support as well, forgot to mention

@philipl
Copy link
Member

philipl commented Dec 28, 2024

@Kwiboo Do you have any perspective on that?

@hbiyik
Copy link
Author

hbiyik commented Dec 31, 2024

may be a cleaner approach would be to implement the workflow with AV_CODEC_HW_CONFIG_METHOD_INTERNAL

when a decoder declares this, the client which uses the decoder actually does nothing, no hwdevice or frames context creation, everything is handled by the decoder internally.

rkmpp is using this method, also cuvid decoder. I think for mpv it is the most flexible way..

@Kwiboo
Copy link
Contributor

Kwiboo commented Dec 31, 2024

Do you have any perspective on that?

Use of rkmpp on vendor kernel allow use of multi core and 8K decoding on newer Rockchip SoCs (RK35xx). There was only an initial attempt by Collabora at adding support for rkvdec2 (the main hw decoder used in RK35xx) in mainline a few months back. To my knowledge Collbora has intention to resume/continuing to work on upstream rkvdec2 support in 2025.

Also my work on ffmpeg v4l2-request and rkvdec1 (h264 hi10 and hevc) got put on temporary hold for a few months due to my day-job, leaving very little time for hobby projects. Hoping to once again have more time early in the new year to complete all pending series.

may be a cleaner approach would be to implement the workflow with AV_CODEC_HW_CONFIG_METHOD_INTERNAL

Making use of the device_type in the AVCodecHWConfig and also adding support for AV_CODEC_HW_CONFIG_METHOD_INTERNAL in methods could benefit mpv, and is what Kodi try to do with xbmc/xbmc#25467 now merged.

@hbiyik hbiyik force-pushed the fallback_to_hw_imgfmt branch from d25736a to d845c5c Compare December 31, 2024 12:44
@hbiyik
Copy link
Author

hbiyik commented Dec 31, 2024

something like this may be helpfull: d845c5c

can be even prettier instead using magic AV_HWDEVICE_TYPE_NONE as skipargument, i just wanted to keep it short.

@hbiyik hbiyik changed the title video: hwdec: fallback to hw_ingmtfmt match if now hw_device matches video: hwdec: fallback to hw_ingmtfmt match if no hw_device matches Dec 31, 2024
Copy link
Member

@philipl philipl left a comment

Choose a reason for hiding this comment

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

We already have a METHOD_INTERNAL block (it's an else block below the if you modified) so you will need to rationalise that. Why does it not work in the first place? Seems like the fix here is to make the METHOD_INTERNAL code path work for the rkmpp decoder.

@hbiyik
Copy link
Author

hbiyik commented Dec 31, 2024

sorry no thats just me overlooking at the code, i will send an update. happy new year btw.

@Kwiboo
Copy link
Contributor

Kwiboo commented Jan 1, 2025

Why does it not work in the first place? Seems like the fix here is to make the METHOD_INTERNAL code path work for the rkmpp decoder.

Look like it is handled in an else if, and ffmpeg-rkmpp report all HW_DEVICE_CTX, HW_FRAMES_CTX and INTERNAL bits in methods, this should probably be reworked in mpv to support falling back to use the INTERNAL mode if HW_DEVICE_CTX/HW_FRAMES_CTX mode failed, and/or ffmpeg-rkmpp could possible only expose the INTERNAL flag and not all methods.

@hbiyik does mpv (and other players/decoders/encoders) work with just the INTERNAL flag set in ffmpeg-rkmpp?

@nyanmisaka
Copy link

and ffmpeg-rkmpp report all HW_DEVICE_CTX, HW_FRAMES_CTX and INTERNAL bits

The addition of METHOD_HW_FRAMES_CTX is a hack made for older mpv & kodi versions. Only METHOD_HW_DEVICE_CTX and METHOD_INTERNAL actually work.

ffmpeg-rkmpp could probably only expose the INTERNAL flag

METHOD_HW_DEVICE_CTX is required for DRM_PRIME -> OPENCL deriving in FFmpeg CLI, and I have a use case for this.

@hbiyik
Copy link
Author

hbiyik commented Jan 1, 2025

Why does it not work in the first place? Seems like the fix here is to make the METHOD_INTERNAL code path work for the rkmpp decoder.

Look like it is handled in an else if, and ffmpeg-rkmpp report all HW_DEVICE_CTX, HW_FRAMES_CTX and INTERNAL bits in methods, this should probably be reworked in mpv to support falling back to use the INTERNAL mode if HW_DEVICE_CTX/HW_FRAMES_CTX mode failed, and/or ffmpeg-rkmpp could possible only expose the INTERNAL flag and not all methods.

@hbiyik does mpv (and other players/decoders/encoders) work with just the INTERNAL flag set in ffmpeg-rkmpp?

Yeah 2 wrongs dont make 1 right, initially rkmpp was either AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX or AV_CODEC_HW_CONFIG_METHOD_INTERNAL.

Since hwdecs in mpv is only detected as internal when the codec is not device or frames context accelerated, i had proposed the hack to provide fake frames context, just to make mpv utilize the rkmpp codec without creating a drm_hwdevice and rkmpp will create its own rkmpp_device. So this is and was just hack not to change mpv code. And i think thats gonna be removed. Other than that mpp already works with internal flag and this is how i am testing..

But i am scratching my head around this line of code. It only flags the hwdec as internal if it is not already frames or device accel flagged which i can not make sense. Because there is no such codec. All codecs which have internal flag already has a device flag. Yet this is not the norm, it can also happen tht there might be a codec with internal flag only, but this would still not mean that a codec should be either internal or device/frame accel flagged as in mpv code. (sorry if this sounds confusing i tried my best to simplify the statement)

I have checked the blame of those commits, actually it is dated back to 2017, so i am concerned that there would be an mpv workflow there.

With those being said. I would propose the following.

  1. Keep the hwdec internal flag in the hwaccelinfo struct.
  2. Keep the existing behaviour that an hwdec can be either frame or device accel flagged but not both.
  3. But a codec can be in parallel flagged as internal hw accelled. (ie: remove the else if, be carefull how yo get the name from wrapper or hwdevice name etc..)
  4. when hwdec_devices_get_by_imgfmt_and_type is called, search for a matching device_type if hwdec is frames or device accelled, in case not found and and hwdec is also internal accelled, dont match the device type at all, because as client mpv doesn't care at this point, providing a matching frame format should be enough.

And that above is an intrusive change.

With current state, mpp will not be tagged as internal because it also supports device accel. Even mpp was tagged as internal hwaccel by mpv somehow, img format will not match due missing mpv functionality in hwdec_devices_get_by_imgfmt_and_type with latest change which requires a matching device defined device type.

@hbiyik
Copy link
Author

hbiyik commented Jan 7, 2025

since there is no other feedback how to achieve this properly i am closing the PR.

@hbiyik hbiyik closed this Jan 7, 2025
@philipl
Copy link
Member

philipl commented Jan 8, 2025

You have to have reasonable expectations on when anyone will have time to respond on any particular issue or PR.

In principle it makes sense to have a fallback path for hwaccels that support INTERNAL if DEVICE_CTX doesn't work.

As for what actually exists in the world, the only upstream hwaccel that uses INTERNAL is cuviddec which also supports DEVICE_CTX and that is the path that we use because we have to initialise the device for interop to work.

It will be some time before I can sit down and take a detailed look at the best way to implement this kind of fallback, but I do believe it would generally look like what you outlined.

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

Successfully merging this pull request may close these issues.

5 participants