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

avfilter: add vf_tonemap_videotoolbox #369

Merged
merged 3 commits into from
Mar 23, 2024

Conversation

gnattu
Copy link
Member

@gnattu gnattu commented Mar 23, 2024

This filter does HDR(HDR10/HLG/Dolby Vision) to SDR conversion with tone-mapping using Metal.

For example:

ffmpeg \
  -init_hw_device videotoolbox \
  -i INPUT \
  -vf hwupload,tonemap_videotoolbox=t=bt2020:tonemap=linear:format=p010 \
  -c:v h264_videotoolbox \
  -b:v 2000k \
  -c:a copy \
  -y OUTPUT

Changes

Issues

Fixes #357

This filter does HDR(HDR10/HLG/Dolby Vision) to SDR conversion with tone-mapping
using Metal.

For example:

ffmpeg \
  -init_hw_device videotoolbox \
  -i INPUT \
  -vf hwupload,tonemap_videotoolbox=t=bt2020:tonemap=linear:format=p010 \
  -c:v h264_videotoolbox \
  -b:v 2000k \
  -c:a copy \
  -y OUTPUT

Signed-off-by: gnattu <gnattuoc@me.com>
@gnattu
Copy link
Member Author

gnattu commented Mar 23, 2024

A lot of the context used in this file is missing in the upstream. Is it possible to have this upstreamed?

@gnattu gnattu requested a review from nyanmisaka March 23, 2024 01:51
@nyanmisaka
Copy link
Member

A lot of the context used in this file is missing in the upstream. Is it possible to have this upstreamed?

The existence of vf_libplacebo makes it more difficult to upstream new GPGPU-based tonemap filters. They may require your filter to behave like libplacebo, even though ours is a stripped-down version. In addition, new DoVi-related structures and funcs need to be discussed and defined in colorspace.h. Finally, there are very few developers in upstream who can review Metal/Obj-C code, and such huge filters may be ignored for months or even forever. . .

@nyanmisaka
Copy link
Member

Here are some clips I collected for testing DoVi/HDR. You can play it to see if the color is normal and whether it will cause ffmpeg to fail.

@gnattu
Copy link
Member Author

gnattu commented Mar 23, 2024

Here are some clips I collected for testing DoVi/HDR. You can play it to see if the color is normal and whether it will cause ffmpeg to fail.

These are all P010 videos, and I've already tested a lot of them. I'm leaving non P010 and NV12 formats off until we have samples that confirmed to work.

@nyanmisaka
Copy link
Member

Here are some clips I collected for testing DoVi/HDR. You can play it to see if the color is normal and whether it will cause ffmpeg to fail.

These are all P010 videos, and I've already tested a lot of them. I'm leaving non P010 and NV12 formats off until we have samples that confirmed to work.

What I mean is that it contains clips that need to reset the filter. As for non-(4:2:0 10-bit) DoVi, I think it won't happen any time soon.

@gnattu
Copy link
Member Author

gnattu commented Mar 23, 2024

What I mean is that it contains clips that need to reset the filter.

I tested all of them and every sample looks good.

As for non-(4:2:0 10-bit) DoVi, I think it won't happen any time soon.

Then keep p010 and nv12 as only supported formats for now.

One interesting thing is, during my testing I found an ffmpeg issue on bufsize and maxrate option, where setting them will cause performance regression and even encoder hangs when the video bitrate is high enough. I'm going to push a server side change as well.

@nyanmisaka
Copy link
Member

One interesting thing is, during my testing I found an ffmpeg issue on bufsize and maxrate option, where setting them will cause performance regression and even encoder hangs when the video bitrate is high enough. I'm going to push a server side change as well.

These are generic lavc options. Encoders should adapt to these options and throw an error if the user settings exceed the hardware specs, or automatically roll back to the default values.

@gnattu
Copy link
Member Author

gnattu commented Mar 23, 2024

It's not about "exceeding hardware specs"; it's a supported value, but it just works buggy. The encoder would succeed, albeit with a much slower speed (up to a 50% regression), and in some circumstances, the ffmpeg process just hangs and doesn't proceed. This happens randomly. Removing the maxrate and bufsize , set only -b:v(-b:v and maxrate are the same value in this case, and bufsize=maxrate *2 like what we are doing in jellyfin) and letting VideoToolbox handle it automatically mitigates this issue.

@gnattu
Copy link
Member Author

gnattu commented Mar 23, 2024

2024-03-23T15:51:40.8291924Z + git clone -b stripped4 --depth=1 https://gitlab.freedesktop.org/wtaymans/fdk-aac-stripped.git
2024-03-23T15:51:40.8305011Z Cloning into 'fdk-aac-stripped'...
2024-03-23T15:54:34.6733077Z error: RPC failed; HTTP 502 curl 22 The requested URL returned error: 502

CI failure due to remote issue

@nyanmisaka
Copy link
Member

It's not about "exceeding hardware specs"; it's a supported value, but it just works buggy. The encoder would succeed, albeit with a much slower speed (up to a 50% regression), and in some circumstances, the ffmpeg process just hangs and doesn't proceed. This happens randomly. Removing the maxrate and bufsize , set only -b:v(-b:v and maxrate are the same value in this case, and bufsize=maxrate *2 like what we are doing in jellyfin) and letting VideoToolbox handle it automatically mitigates this issue.

Ideally this undefined behavior should not occur. Either work or fail. If removing them does not cause rate control regression, then there is no problem.

// not supported
bufsize(avctx->rc_buffer_size)

// https://github.com/FFmpeg/FFmpeg/blob/bfbf0f4e82eff7fc6f15205b71e97322e7681dcb/libavcodec/videotoolboxenc.c#L1273-L1318
maxrate(avctx->rc_max_rate)

@gnattu
Copy link
Member Author

gnattu commented Mar 23, 2024

If removing them does not cause rate control regression, then there is no problem.

VT obeys the b:v option quite well and would not cause the bitrate to be much higher than that, so the maxrate can be safely removed.

My suggested changes:

jellyfin/jellyfin@2933ec2

Copy link
Member

@nyanmisaka nyanmisaka left a comment

Choose a reason for hiding this comment

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

yolo

@gnattu gnattu merged commit 202de8d into jellyfin:jellyfin Mar 23, 2024
25 checks passed
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.

Improve tone mapping performance on Apple Silicon
2 participants