search

jellyfin / jellyfin-ffmpeg

FFmpeg for Jellyfin
https://jellyfin.org
Other
477 stars 127 forks source link

avfilter: add vf_tonemap_videotoolbox #369

Closed gnattu closed 6 months ago

gnattu commented 6 months ago

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

gnattu commented 6 months ago

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

nyanmisaka commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago 
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 commented 6 months ago

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 commented 6 months ago

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:

https://github.com/jellyfin/jellyfin/pull/11198/commits/2933ec2eb7e409b295e67e0e4333b161f9a00726