| Message ID | 20240319191642.95217-4-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] fftools/ffmpeg_enc: strip DOVI config record for AV1 | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 3/19/2024 7:16 PM, Niklas Haas wrote: > From: Niklas Haas <git@haasn.dev> > > libx265 supports these natively, we just need to forward them to the > x265picture. > --- > libavcodec/libx265.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) The reason I never implemented this back when I adde RPU side data is that there is a strong chance of generating broken files. That's because if we do anything to the video with swscale, etc., we're now encoding RPUs that aren't meant to be applied to that converted video. For example, this could end up propagating RPUs when the user is tonemapping. - Derek
> On Mar 19, 2024, at 2:39 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > > The reason I never implemented this back when I adde RPU side data is that > there is a strong chance of generating broken files. > > That's because if we do anything to the video with swscale, etc., we're > now encoding RPUs that aren't meant to be applied to that converted video. > > For example, this could end up propagating RPUs when the user is tonemapping. Would it be possible to only propagate RPUs if the color params are not changing? If there's any change from say PQ to HLG or HLG to PQ or tonemapping then we wouldn't want to propagate RPUs. If the color params are not changing then propagating RPUs by default seems sensible, and perhaps a filter can be added to explicitly clear RPUs if they should not be propagated. - Cosmin
On Tue, 19 Mar 2024 21:59:53 +0000 Cosmin Stejerean via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote: > > > > On Mar 19, 2024, at 2:39 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > > > > The reason I never implemented this back when I adde RPU side data is that > > there is a strong chance of generating broken files. > > > > That's because if we do anything to the video with swscale, etc., we're > > now encoding RPUs that aren't meant to be applied to that converted video. > > > > For example, this could end up propagating RPUs when the user is tonemapping. > > Would it be possible to only propagate RPUs if the color params are not changing? If there's any change from say PQ to HLG or HLG to PQ or tonemapping then we wouldn't want to propagate RPUs. If the color params are not changing then propagating RPUs by default seems sensible, and perhaps a filter can be added to explicitly clear RPUs if they should not be propagated. One way to accomplish this would be to simply strip the metadata in all filters that can change the colorspace. Maybe we should do the same for HDR+ etc. metadata. Probably it would make sense to add a common helper function for this. I'll see what I can do.
On Tue, Mar 19, 2024 at 7:04 PM Niklas Haas <ffmpeg@haasn.xyz> wrote: > On Tue, 19 Mar 2024 21:59:53 +0000 Cosmin Stejerean via ffmpeg-devel < > ffmpeg-devel@ffmpeg.org> wrote: > > > > > > > On Mar 19, 2024, at 2:39 PM, Derek Buitenhuis < > derek.buitenhuis@gmail.com> wrote: > > > > > > The reason I never implemented this back when I adde RPU side data is > that > > > there is a strong chance of generating broken files. > > > > > > That's because if we do anything to the video with swscale, etc., we're > > > now encoding RPUs that aren't meant to be applied to that converted > video. > > > > > > For example, this could end up propagating RPUs when the user is > tonemapping. > > > > Would it be possible to only propagate RPUs if the color params are not > changing? If there's any change from say PQ to HLG or HLG to PQ or > tonemapping then we wouldn't want to propagate RPUs. If the color params > are not changing then propagating RPUs by default seems sensible, and > perhaps a filter can be added to explicitly clear RPUs if they should not > be propagated. > > One way to accomplish this would be to simply strip the metadata in all > filters > that can change the colorspace. Maybe we should do the same for HDR+ etc. > metadata. > > Probably it would make sense to add a common helper function for this. > I'll see > what I can do. > In the meantime maybe just adding an encoder option to preserve existing metadata would help?
On Tue, Mar 19, 2024 at 08:16:42PM +0100, Niklas Haas wrote: > From: Niklas Haas <git@haasn.dev> > > libx265 supports these natively, we just need to forward them to the > x265picture. > --- > libavcodec/libx265.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) breaks build here CC libavcodec/libx265.o libavcodec/libx265.c: In function ‘free_picture’: libavcodec/libx265.c:563:16: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ av_free(pic->rpu.payload); ^~ libavcodec/libx265.c: In function ‘libx265_encode_frame’: libavcodec/libx265.c:701:20: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ x265pic.rpu.payload = av_memdup(sd->data, sd->size); ^ libavcodec/libx265.c:702:25: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ if (!x265pic.rpu.payload) { ^ libavcodec/libx265.c:706:20: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ x265pic.rpu.payloadSize = sd->size; ^ ffbuild/common.mak:81: recipe for target 'libavcodec/libx265.o' failed make: *** [libavcodec/libx265.o] Error 1 [...]
On Wed, Mar 20, 2024 at 9:30 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Tue, Mar 19, 2024 at 08:16:42PM +0100, Niklas Haas wrote: > > From: Niklas Haas <git@haasn.dev> > > > > libx265 supports these natively, we just need to forward them to the > > x265picture. > > --- > > libavcodec/libx265.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > breaks build here > > CC libavcodec/libx265.o > libavcodec/libx265.c: In function ‘free_picture’: > libavcodec/libx265.c:563:16: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ > av_free(pic->rpu.payload); > ^~ > libavcodec/libx265.c: In function ‘libx265_encode_frame’: > libavcodec/libx265.c:701:20: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ > x265pic.rpu.payload = av_memdup(sd->data, sd->size); > ^ > libavcodec/libx265.c:702:25: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ > if (!x265pic.rpu.payload) { > ^ > libavcodec/libx265.c:706:20: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ > x265pic.rpu.payloadSize = sd->size; > ^ > ffbuild/common.mak:81: recipe for target 'libavcodec/libx265.o' failed > make: *** [libavcodec/libx265.o] Error 1 > The RPU structure and its location in x265_picture was added in 5a7d027d82821a8b4d9c768d6b8fb0560557e2bd , bumping X265_BUILD to 167 (2018-09-27) . Configure check is currently for 89, and we already check in the wrapper for 130 and 159. So possibly it makes sense to just bump the requirement to a version from 2018? Otherwise just another #if X265_BUILD >= 167 ? Jan
On Wed, 20 Mar 2024 23:22:03 +0200 Jan Ekström <jeebjp@gmail.com> wrote: > On Wed, Mar 20, 2024 at 9:30 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Tue, Mar 19, 2024 at 08:16:42PM +0100, Niklas Haas wrote: > > > From: Niklas Haas <git@haasn.dev> > > > > > > libx265 supports these natively, we just need to forward them to the > > > x265picture. > > > --- > > > libavcodec/libx265.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > breaks build here > > > > CC libavcodec/libx265.o > > libavcodec/libx265.c: In function ‘free_picture’: > > libavcodec/libx265.c:563:16: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ > > av_free(pic->rpu.payload); > > ^~ > > libavcodec/libx265.c: In function ‘libx265_encode_frame’: > > libavcodec/libx265.c:701:20: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ > > x265pic.rpu.payload = av_memdup(sd->data, sd->size); > > ^ > > libavcodec/libx265.c:702:25: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ > > if (!x265pic.rpu.payload) { > > ^ > > libavcodec/libx265.c:706:20: error: ‘x265_picture {aka struct x265_picture}’ has no member named ‘rpu’ > > x265pic.rpu.payloadSize = sd->size; > > ^ > > ffbuild/common.mak:81: recipe for target 'libavcodec/libx265.o' failed > > make: *** [libavcodec/libx265.o] Error 1 > > > > The RPU structure and its location in x265_picture was added in > 5a7d027d82821a8b4d9c768d6b8fb0560557e2bd , bumping X265_BUILD to 167 > (2018-09-27) . > > Configure check is currently for 89, and we already check in the > wrapper for 130 and 159. So possibly it makes sense to just bump the > requirement to a version from 2018? Otherwise just another #if > X265_BUILD >= 167 ? Added an X265_BUILD >= 167 check.
On Tue, 19 Mar 2024 19:19:29 -0400 Vittorio Giovara <vittorio.giovara@gmail.com> wrote: > On Tue, Mar 19, 2024 at 7:04 PM Niklas Haas <ffmpeg@haasn.xyz> wrote: > > > On Tue, 19 Mar 2024 21:59:53 +0000 Cosmin Stejerean via ffmpeg-devel < > > ffmpeg-devel@ffmpeg.org> wrote: > > > > > > > > > > On Mar 19, 2024, at 2:39 PM, Derek Buitenhuis < > > derek.buitenhuis@gmail.com> wrote: > > > > > > > > The reason I never implemented this back when I adde RPU side data is > > that > > > > there is a strong chance of generating broken files. > > > > > > > > That's because if we do anything to the video with swscale, etc., we're > > > > now encoding RPUs that aren't meant to be applied to that converted > > video. > > > > > > > > For example, this could end up propagating RPUs when the user is > > tonemapping. > > > > > > Would it be possible to only propagate RPUs if the color params are not > > changing? If there's any change from say PQ to HLG or HLG to PQ or > > tonemapping then we wouldn't want to propagate RPUs. If the color params > > are not changing then propagating RPUs by default seems sensible, and > > perhaps a filter can be added to explicitly clear RPUs if they should not > > be propagated. > > > > One way to accomplish this would be to simply strip the metadata in all > > filters > > that can change the colorspace. Maybe we should do the same for HDR+ etc. > > metadata. > > > > Probably it would make sense to add a common helper function for this. > > I'll see > > what I can do. > > > > In the meantime maybe just adding an encoder option to preserve existing > metadata would help? Adding a flag to the encoder to control whether to write dolby vision RPUs (defaulting to off) seems like a good idea. At some level, we fundamentally have to rely on the user to tell us whether dolby vision metadata is still valid after filtering. There is still the separate concern of how to control whether or not a dovi *configuration* record should be emitted when muxing, which should be done on a remux but should not be done on a decode or when stripping DV metadata. That said, including a dolby configuration record but without corresponding RPUs at the very least appears to be harmless, though I have not verified with actual hardware.
diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c index 92183b9ca26..92b25844ef6 100644 --- a/libavcodec/libx265.c +++ b/libavcodec/libx265.c @@ -560,6 +560,7 @@ static av_cold int libx265_encode_set_roi(libx265Context *ctx, const AVFrame *fr static void free_picture(libx265Context *ctx, x265_picture *pic) { x265_sei *sei = &pic->userSEI; + av_free(pic->rpu.payload); for (int i = 0; i < sei->numPayloads; i++) av_free(sei->payloads[i].payload); @@ -594,6 +595,7 @@ static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt, sei->numPayloads = 0; if (pic) { + AVFrameSideData *sd; ReorderedData *rd; int rd_idx; @@ -694,6 +696,15 @@ static int libx265_encode_frame(AVCodecContext *avctx, AVPacket *pkt, sei->numPayloads++; } } + + if ((sd = av_frame_get_side_data(pic, AV_FRAME_DATA_DOVI_RPU_BUFFER))) { + x265pic.rpu.payload = av_memdup(sd->data, sd->size); + if (!x265pic.rpu.payload) { + free_picture(ctx, &x265pic); + return AVERROR(ENOMEM); + } + x265pic.rpu.payloadSize = sd->size; + } } ret = ctx->api->encoder_encode(ctx->encoder, &nal, &nnal,
From: Niklas Haas <git@haasn.dev> libx265 supports these natively, we just need to forward them to the x265picture. --- libavcodec/libx265.c | 11 +++++++++++ 1 file changed, 11 insertions(+)