[FFmpeg-devel] [PATCH 1/4] fftools/ffmpeg_enc: strip DOVI config record for AV1

[FFmpeg-devel] [PATCH 1/4] fftools/ffmpeg_enc: strip DOVI config record for AV1

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

AV1 streams don't use configuration records, so delete them when
encoding to AV1. Ideally this would be, as the comment suggests, handled
at the frame-level (and stripped by the av1 encoder), but given the
status quo of copying the packet-level data here directly, we should
definitely make an effort to strip it.
---
 fftools/ffmpeg_enc.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/fftools/ffmpeg_enc.c b/fftools/ffmpeg_enc.c
index c9a12af1393..0c21acfafc6 100644
--- a/fftools/ffmpeg_enc.c
+++ b/fftools/ffmpeg_enc.c
@@ -354,17 +354,20 @@ int enc_open(void *opaque, const AVFrame *frame)
      */
     if (ist) {
         for (int i = 0; i < ist->st->codecpar->nb_coded_side_data; i++) {
-            AVPacketSideData *sd_src = &ist->st->codecpar->coded_side_data[i];
-            if (sd_src->type != AV_PKT_DATA_CPB_PROPERTIES) {
-                AVPacketSideData *sd_dst = av_packet_side_data_new(&ost->par_in->coded_side_data,
-                                                                   &ost->par_in->nb_coded_side_data,
-                                                                   sd_src->type, sd_src->size, 0);
-                if (!sd_dst)
-                    return AVERROR(ENOMEM);
-                memcpy(sd_dst->data, sd_src->data, sd_src->size);
-                if (ist->autorotate && sd_src->type == AV_PKT_DATA_DISPLAYMATRIX)
-                    av_display_rotation_set((int32_t *)sd_dst->data, 0);
-            }
+            AVPacketSideData *sd_src, *sd_dst;
+            sd_src = &ist->st->codecpar->coded_side_data[i];
+            if (sd_src->type == AV_PKT_DATA_CPB_PROPERTIES)
+                continue;
+            if (sd_src->type == AV_PKT_DATA_DOVI_CONF && enc->id == AV_CODEC_ID_AV1)
+                continue; /* AV1 doesn't use DOVI configuration records */
+            sd_dst = av_packet_side_data_new(&ost->par_in->coded_side_data,
+                                             &ost->par_in->nb_coded_side_data,
+                                             sd_src->type, sd_src->size, 0);
+            if (!sd_dst)
+                return AVERROR(ENOMEM);
+            memcpy(sd_dst->data, sd_src->data, sd_src->size);
+            if (ist->autorotate && sd_src->type == AV_PKT_DATA_DISPLAYMATRIX)
+                av_display_rotation_set((int32_t *)sd_dst->data, 0);
         }
     }
 
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/4] avcodec/dovi_rpu: implement T.35 payload synthesis

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

This function converts from DOVI_RPU_BUFFER (raw RPU NAL) to the more
standardized T.35 syntax introduced with Dolby Vision Profile 10.
---
 libavcodec/Makefile   |  2 +-
 libavcodec/dovi_rpu.c | 91 +++++++++++++++++++++++++++++++++++++++++++
 libavcodec/dovi_rpu.h | 10 +++++
 3 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 708434ac76c..0de2fc085ef 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -85,7 +85,7 @@ OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
 OBJS-$(CONFIG_CBS_VP8)                 += cbs_vp8.o vp8data.o
 OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
 OBJS-$(CONFIG_DEFLATE_WRAPPER)         += zlib_wrapper.o
-OBJS-$(CONFIG_DOVI_RPU)                += dovi_rpu.o
+OBJS-$(CONFIG_DOVI_RPU)                += dovi_rpu.o h2645_parse.o
 OBJS-$(CONFIG_ERROR_RESILIENCE)        += error_resilience.o
 OBJS-$(CONFIG_EVCPARSE)                += evc_parse.o evc_ps.o
 OBJS-$(CONFIG_EXIF)                    += exif.o tiff_common.o
diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
index 529062be30a..28b414a81e0 100644
--- a/libavcodec/dovi_rpu.c
+++ b/libavcodec/dovi_rpu.c
@@ -23,9 +23,13 @@
 
 #include "libavutil/buffer.h"
 
+#include "avcodec.h"
 #include "dovi_rpu.h"
 #include "golomb.h"
 #include "get_bits.h"
+#include "h2645_parse.h"
+#include "itut35.h"
+#include "put_bits.h"
 #include "refstruct.h"
 
 enum {
@@ -478,3 +482,90 @@ fail:
     ff_dovi_ctx_unref(s); /* don't leak potentially invalid state */
     return AVERROR(EINVAL);
 }
+
+int ff_dovi_rpu_to_t35(void *logctx, const uint8_t *raw_rpu, int raw_rpu_size,
+                       AVBufferRef **out_t35)
+{
+    PutBitContext *pb = &(PutBitContext){0};
+    H2645RBSP rbsp = {0};
+    H2645NAL nal = {0};
+    AVBufferRef *t35 = NULL;
+    int ret = AVERROR_INVALIDDATA;
+    const uint8_t *rpu;
+    int rpu_size, pad;
+
+    av_fast_padded_malloc(&rbsp.rbsp_buffer, &rbsp.rbsp_buffer_alloc_size,
+                          raw_rpu_size);
+    if (!rbsp.rbsp_buffer)
+        return AVERROR(ENOMEM);
+
+    ret = ff_h2645_extract_rbsp(raw_rpu, raw_rpu_size, &rbsp, &nal, 1);
+    if (ret < 0)
+        goto error;
+
+    rpu = rbsp.rbsp_buffer;
+    rpu_size = rbsp.rbsp_buffer_size;
+    if (rpu_size < 2 || rpu_size > 512) {
+        av_log(logctx, AV_LOG_ERROR, "Invalid DOVI RPU size: %d\n", rpu_size);
+        goto error;
+    }
+
+    /* Skip NAL prefix */
+    if (rpu[0] != 25) {
+        av_log(logctx, AV_LOG_ERROR, "Invalid NAL prefix: %d\n", rpu[0]);
+        goto error;
+    }
+    rpu++;
+    rpu_size--;
+
+    /* Skip trailing NUL bytes */
+    while (rpu_size && rpu[rpu_size - 1] == 0)
+        rpu_size--;
+
+    /* Validate trailing byte is the expected 0x80 */
+    if (!rpu_size || rpu[rpu_size - 1] != 0x80) {
+        av_log(logctx, AV_LOG_ERROR, "Missing RPU RBSP terminator, "
+               "possibly truncated?\n");
+        goto error;
+    }
+
+    /* Fixed T.35+EMDF header is 11 bytes, EMDF payload size is up to 3 bytes,
+     * and trailing footer is up to 4 bytes */
+    t35 = av_buffer_alloc(rpu_size + 18);
+    if (!t35) {
+        ret = AVERROR(ENOMEM);
+        goto error;
+    }
+
+    init_put_bits(pb, t35->data, t35->size);
+    put_bits(pb,  8, ITU_T_T35_COUNTRY_CODE_US);
+    put_bits(pb, 16, ITU_T_T35_PROVIDER_CODE_DOLBY);
+    put_bits32(pb, 0x800); /* provider_oriented_code */
+    put_bits(pb, 27, 0x01be6841u); /* EMDF header, see above */
+    if (rpu_size > 0xFF) {
+        av_assert2(rpu_size <= 0x10000);
+        put_bits(pb, 8, (rpu_size >> 8) - 1);
+        put_bits(pb, 1, 1); /* read_more */
+        put_bits(pb, 8, rpu_size & 0xFF);
+        put_bits(pb, 1, 0);
+    } else {
+        put_bits(pb, 8, rpu_size);
+        put_bits(pb, 1, 0);
+    }
+    ff_copy_bits(pb, rpu, rpu_size * 8); /* rpu rbsp payload */
+    put_bits(pb, 17, 0x400); /* emdf payload id + emdf_protection */
+
+    pad = pb->bit_left & 7;
+    put_bits(pb, pad, (1 << pad) - 1); /* pad to next byte with 1 bits */
+    t35->size -= put_bytes_left(pb, 0); /* trim unused space */
+    flush_put_bits(pb);
+
+    *out_t35 = t35;
+    ret = 0;
+
+error:
+    av_freep(&rbsp.rbsp_buffer);
+    if (ret)
+        av_buffer_unref(&t35);
+    return ret;
+}
diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h
index 51c5fdbb879..0e75689b136 100644
--- a/libavcodec/dovi_rpu.h
+++ b/libavcodec/dovi_rpu.h
@@ -84,4 +84,14 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size);
  */
 int ff_dovi_attach_side_data(DOVIContext *s, AVFrame *frame);
 
+/**
+ * Generate a T.35 payload (including country and provider code) from a given
+ * AV_FRAME_DATA_DOVI_RPU_BUFFER, and set *out_t35 to a new reference on
+ * success.
+ *
+ * Returns 0 or a negative error code.
+ */
+int ff_dovi_rpu_to_t35(void *logctx, const uint8_t *raw_rpu, int raw_rpu_size,
+                       AVBufferRef **out_t35);
+
 #endif /* AVCODEC_DOVI_RPU_H */
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/4] avcodec/libaomenc: encode dovi RPUs as T.35 metadata

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

Using the new conversion function implemented in the previous commit.
---
 libavcodec/libaomenc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
index 8c1f84cc9fb..2c8fe524fa7 100644
--- a/libavcodec/libaomenc.c
+++ b/libavcodec/libaomenc.c
@@ -42,6 +42,7 @@
 #include "avcodec.h"
 #include "bsf.h"
 #include "codec_internal.h"
+#include "dovi_rpu.h"
 #include "encode.h"
 #include "internal.h"
 #include "libaom.h"
@@ -1283,6 +1284,7 @@ static int aom_encode(AVCodecContext *avctx, AVPacket *pkt,
     aom_enc_frame_flags_t flags = 0;
 
     if (frame) {
+        AVFrameSideData *sd;
         rawimg                      = &ctx->rawimg;
         rawimg->planes[AOM_PLANE_Y] = frame->data[0];
         rawimg->planes[AOM_PLANE_U] = frame->data[1];
@@ -1318,6 +1320,17 @@ FF_ENABLE_DEPRECATION_WARNINGS
             break;
         }
 
+        if ((sd = av_frame_get_side_data(frame, AV_FRAME_DATA_DOVI_RPU_BUFFER))) {
+            AVBufferRef *t35;
+            if ((res = ff_dovi_rpu_to_t35(avctx, sd->data, sd->size, &t35)) < 0)
+                return res;
+            res = aom_img_add_metadata(rawimg, OBU_METADATA_TYPE_ITUT_T35,
+                                       t35->data, t35->size, AOM_MIF_ANY_FRAME);
+            av_buffer_unref(&t35);
+            if (res != AOM_CODEC_OK)
+                return AVERROR(ENOMEM);
+        }
+
         if (frame->pict_type == AV_PICTURE_TYPE_I)
             flags |= AOM_EFLAG_FORCE_KF;
     }
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/4] avcodec/libx265: encode dovi RPUs

[Niklas Haas]

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(+)

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,
-- 
2.44.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/libx265: encode dovi RPUs

[Derek Buitenhuis]

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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/libx265: encode dovi RPUs

[Cosmin Stejerean via ffmpeg-devel]

> 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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/libx265: encode dovi RPUs

[Niklas Haas]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/libx265: encode dovi RPUs

[Vittorio Giovara]

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?
-- 
Vittorio
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/libx265: encode dovi RPUs

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1511 bytes --]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/libx265: encode dovi RPUs

[Jan Ekström]

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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/ffmpeg_enc: strip DOVI config record for AV1

[Anton Khirnov]

Quoting Niklas Haas (2024-03-19 20:16:39)
> From: Niklas Haas <git@haasn.dev>
> 
> AV1 streams don't use configuration records, so delete them when
> encoding to AV1. Ideally this would be, as the comment suggests, handled
> at the frame-level (and stripped by the av1 encoder), but given the
> status quo of copying the packet-level data here directly, we should
> definitely make an effort to strip it.
> ---
>  fftools/ffmpeg_enc.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)

I'm very much not a fan of having codec-specific code in ffmpeg CLI. It
implies that every single caller must now be aware of this
(undocumented?) interaction of this specific side data with this
specific codec ID.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/libx265: encode dovi RPUs

[Niklas Haas]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/libx265: encode dovi RPUs

[Niklas Haas]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/ffmpeg_enc: strip DOVI config record for AV1

[Niklas Haas]

On Thu, 21 Mar 2024 11:16:57 +0100 Anton Khirnov <anton@khirnov.net> wrote:
> Quoting Niklas Haas (2024-03-19 20:16:39)
> > From: Niklas Haas <git@haasn.dev>
> > 
> > AV1 streams don't use configuration records, so delete them when
> > encoding to AV1. Ideally this would be, as the comment suggests, handled
> > at the frame-level (and stripped by the av1 encoder), but given the
> > status quo of copying the packet-level data here directly, we should
> > definitely make an effort to strip it.
> > ---
> >  fftools/ffmpeg_enc.c | 25 ++++++++++++++-----------
> >  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> I'm very much not a fan of having codec-specific code in ffmpeg CLI. It
> implies that every single caller must now be aware of this
> (undocumented?) interaction of this specific side data with this
> specific codec ID.

Note: This is an existing bug, not introduced by this series. This
series just makes it obvious. The status quo is that, beacuse of this
logic in ffmpeg_enc.c, we incorrectly forward dolby vision configuration
records when transcoding to AV1.

Or, indeed, when transcoding to *any* format - since current FFmpeg also
does not propagate dolby vision RPUs, we generate broken files pretty
much always when transcoding dolby vision. So we definitely need to
strip the metadata from the stream muxer *somewhere*. Where else comes
to mind?

This also gets into another topic I wanted to touch on, which is that
the presence of dynamic dolby vision metadata currently hinders the
ability of libavfilter to treat the video primaries/gamma as
a negotiable colorspace property (the way it is done currently for YUV
matrix/range). This is because when interpreted as such, DV metadata
fundamentally changes the colorspace of the incoming video stream.
Ideally we would like some way to negotiate DV metadata on the
query_formats() level.

Ideally, we'd want something like AVCOL_SPC_DOLBYVISION, but we can't
easily introduce that without breaking ISO/IEC 23091 compatibility..

-------
P.s. I accidentally sent a duplicate of this email from a different
(wrong) mail address, please ignore.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/ffmpeg_enc: strip DOVI config record for AV1

[Anton Khirnov]

Quoting Niklas Haas (2024-03-21 13:11:32)
> On Thu, 21 Mar 2024 11:16:57 +0100 Anton Khirnov <anton@khirnov.net> wrote:
> > Quoting Niklas Haas (2024-03-19 20:16:39)
> > > From: Niklas Haas <git@haasn.dev>
> > > 
> > > AV1 streams don't use configuration records, so delete them when
> > > encoding to AV1. Ideally this would be, as the comment suggests, handled
> > > at the frame-level (and stripped by the av1 encoder), but given the
> > > status quo of copying the packet-level data here directly, we should
> > > definitely make an effort to strip it.
> > > ---
> > >  fftools/ffmpeg_enc.c | 25 ++++++++++++++-----------
> > >  1 file changed, 14 insertions(+), 11 deletions(-)
> > 
> > I'm very much not a fan of having codec-specific code in ffmpeg CLI. It
> > implies that every single caller must now be aware of this
> > (undocumented?) interaction of this specific side data with this
> > specific codec ID.
> 
> Note: This is an existing bug, not introduced by this series. This
> series just makes it obvious. The status quo is that, beacuse of this
> logic in ffmpeg_enc.c, we incorrectly forward dolby vision configuration
> records when transcoding to AV1.

I know pretty much nothing about dolby vision, so could you please
explain why precisely is this incorrect? And at what point in the
transcoding chain does the side data become invalid?

> Or, indeed, when transcoding to *any* format - since current FFmpeg also
> does not propagate dolby vision RPUs, we generate broken files pretty
> much always when transcoding dolby vision. So we definitely need to
> strip the metadata from the stream muxer *somewhere*. Where else comes
> to mind?
> 
> This also gets into another topic I wanted to touch on, which is that
> the presence of dynamic dolby vision metadata currently hinders the
> ability of libavfilter to treat the video primaries/gamma as
> a negotiable colorspace property (the way it is done currently for YUV
> matrix/range). This is because when interpreted as such, DV metadata
> fundamentally changes the colorspace of the incoming video stream.
> Ideally we would like some way to negotiate DV metadata on the
> query_formats() level.
> 
> Ideally, we'd want something like AVCOL_SPC_DOLBYVISION, but we can't
> easily introduce that without breaking ISO/IEC 23091 compatibility..

In principle it could be yet another negotiated field, could it not? You
just added a bunch of those recently, what's another one?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/ffmpeg_enc: strip DOVI config record for AV1

[Niklas Haas]

On Fri, 22 Mar 2024 10:41:13 +0100 Anton Khirnov <anton@khirnov.net> wrote:
> Quoting Niklas Haas (2024-03-21 13:11:32)
> > On Thu, 21 Mar 2024 11:16:57 +0100 Anton Khirnov <anton@khirnov.net> wrote:
> > > Quoting Niklas Haas (2024-03-19 20:16:39)
> > > > From: Niklas Haas <git@haasn.dev>
> > > > 
> > > > AV1 streams don't use configuration records, so delete them when
> > > > encoding to AV1. Ideally this would be, as the comment suggests, handled
> > > > at the frame-level (and stripped by the av1 encoder), but given the
> > > > status quo of copying the packet-level data here directly, we should
> > > > definitely make an effort to strip it.
> > > > ---
> > > >  fftools/ffmpeg_enc.c | 25 ++++++++++++++-----------
> > > >  1 file changed, 14 insertions(+), 11 deletions(-)
> > > 
> > > I'm very much not a fan of having codec-specific code in ffmpeg CLI. It
> > > implies that every single caller must now be aware of this
> > > (undocumented?) interaction of this specific side data with this
> > > specific codec ID.
> > 
> > Note: This is an existing bug, not introduced by this series. This
> > series just makes it obvious. The status quo is that, beacuse of this
> > logic in ffmpeg_enc.c, we incorrectly forward dolby vision configuration
> > records when transcoding to AV1.
> 
> I know pretty much nothing about dolby vision, so could you please
> explain why precisely is this incorrect? And at what point in the
> transcoding chain does the side data become invalid?

Dolby Vision basically consists of two separate pieces of metadata:

1. The (per-stream) configuration struct, AV_PKT_DATA_DOVI_CONF
2. The per-frame structs (RPUs), AV_FRAME_DATA_DOVI_METADATA
     (ditto AV_FRAME_DATA_DOVI_RPU_BUFFER, which is the same)

A valid HEVC dolby vision file should contain both - the configuration
struct tells the decoder that hey, this file is dolby vision (and what
profile to expect, whether there's an enhancement layer, etc.). The RPUs
contain the actual DV-specific details of how each frame is encoded.

A valid AV1 dolby vision file, on the other hand, only uses the
per-frame RPUs, it does not have a configuration struct at all.

The current logic in ffmpeg_enc.c copies over all stream-level metadata,
including the DOVI_CONF struct, to the output file. This generates
a stream which is *marked* as being Dolby Vision, but in which none of
the frames actually contain DV RPUs. This *probably* violates some spec
somewhere, and at the very least is not desirable behavior. (And for
AV1, the configuration struct's presence is definitely a no-go)

Basically, we want to handle all of these scenarios:

1. When transcoding DV HEVC (profile 8) to DV AV1 (profile 10), we need
   to strip the configuration struct somewhere
2. When transcoding DV HEVC (profile 8) to HEVC, we need to strip the
   configuration struct IFF we're also stripping the per-frame RPUs
   (e.g. as a result of filtering).
3. When transcoding DV AV1 (profile 10) to HEVC, we need to *synthesize*
   a configuration struct containing the correct values.

I think the best way forward for now is:

1. Always strip the dovi configuration record when transcoding
2. Have the encoder generate (and attach to avctx.coded_side_data) the
   correct configuration record.

I will write a patch for #2.

> > Or, indeed, when transcoding to *any* format - since current FFmpeg also
> > does not propagate dolby vision RPUs, we generate broken files pretty
> > much always when transcoding dolby vision. So we definitely need to
> > strip the metadata from the stream muxer *somewhere*. Where else comes
> > to mind?
> > 
> > This also gets into another topic I wanted to touch on, which is that
> > the presence of dynamic dolby vision metadata currently hinders the
> > ability of libavfilter to treat the video primaries/gamma as
> > a negotiable colorspace property (the way it is done currently for YUV
> > matrix/range). This is because when interpreted as such, DV metadata
> > fundamentally changes the colorspace of the incoming video stream.
> > Ideally we would like some way to negotiate DV metadata on the
> > query_formats() level.
> > 
> > Ideally, we'd want something like AVCOL_SPC_DOLBYVISION, but we can't
> > easily introduce that without breaking ISO/IEC 23091 compatibility..
> 
> In principle it could be yet another negotiated field, could it not? You
> just added a bunch of those recently, what's another one?

Adding more fields to this negotiation process is a very obnoxious and
tedious process, with lots of boilerplate for each new field added.
Maybe we can come up with some better mechanism first?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/ffmpeg_enc: strip DOVI config record for AV1

[Niklas Haas]

On Fri, 22 Mar 2024 14:08:07 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> On Fri, 22 Mar 2024 10:41:13 +0100 Anton Khirnov <anton@khirnov.net> wrote:
> > Quoting Niklas Haas (2024-03-21 13:11:32)
> > > On Thu, 21 Mar 2024 11:16:57 +0100 Anton Khirnov <anton@khirnov.net> wrote:
> > > > Quoting Niklas Haas (2024-03-19 20:16:39)
> > > > > From: Niklas Haas <git@haasn.dev>
> > > > > 
> > > > > AV1 streams don't use configuration records, so delete them when
> > > > > encoding to AV1. Ideally this would be, as the comment suggests, handled
> > > > > at the frame-level (and stripped by the av1 encoder), but given the
> > > > > status quo of copying the packet-level data here directly, we should
> > > > > definitely make an effort to strip it.
> > > > > ---
> > > > >  fftools/ffmpeg_enc.c | 25 ++++++++++++++-----------
> > > > >  1 file changed, 14 insertions(+), 11 deletions(-)
> > > > 
> > > > I'm very much not a fan of having codec-specific code in ffmpeg CLI. It
> > > > implies that every single caller must now be aware of this
> > > > (undocumented?) interaction of this specific side data with this
> > > > specific codec ID.
> > > 
> > > Note: This is an existing bug, not introduced by this series. This
> > > series just makes it obvious. The status quo is that, beacuse of this
> > > logic in ffmpeg_enc.c, we incorrectly forward dolby vision configuration
> > > records when transcoding to AV1.
> > 
> > I know pretty much nothing about dolby vision, so could you please
> > explain why precisely is this incorrect? And at what point in the
> > transcoding chain does the side data become invalid?
> 
> Dolby Vision basically consists of two separate pieces of metadata:
> 
> 1. The (per-stream) configuration struct, AV_PKT_DATA_DOVI_CONF
> 2. The per-frame structs (RPUs), AV_FRAME_DATA_DOVI_METADATA
>      (ditto AV_FRAME_DATA_DOVI_RPU_BUFFER, which is the same)
> 
> A valid HEVC dolby vision file should contain both - the configuration
> struct tells the decoder that hey, this file is dolby vision (and what
> profile to expect, whether there's an enhancement layer, etc.). The RPUs
> contain the actual DV-specific details of how each frame is encoded.
> 
> A valid AV1 dolby vision file, on the other hand, only uses the
> per-frame RPUs, it does not have a configuration struct at all.

Correction: AV1 still uses configuration records, I was not looking at
spec-compliant sample files. So at least, this difference evaporates.

> The current logic in ffmpeg_enc.c copies over all stream-level metadata,
> including the DOVI_CONF struct, to the output file. This generates
> a stream which is *marked* as being Dolby Vision, but in which none of
> the frames actually contain DV RPUs. This *probably* violates some spec
> somewhere, and at the very least is not desirable behavior. (And for
> AV1, the configuration struct's presence is definitely a no-go)
> 
> Basically, we want to handle all of these scenarios:
> 
> 1. When transcoding DV HEVC (profile 8) to DV AV1 (profile 10), we need
>    to strip the configuration struct somewhere
> 2. When transcoding DV HEVC (profile 8) to HEVC, we need to strip the
>    configuration struct IFF we're also stripping the per-frame RPUs
>    (e.g. as a result of filtering).
> 3. When transcoding DV AV1 (profile 10) to HEVC, we need to *synthesize*
>    a configuration struct containing the correct values.
> 
> I think the best way forward for now is:
> 
> 1. Always strip the dovi configuration record when transcoding
> 2. Have the encoder generate (and attach to avctx.coded_side_data) the
>    correct configuration record.

As discussed on IRC, removing this logic entirely will cover the first
point.

> I will write a patch for #2.
> 
> > > Or, indeed, when transcoding to *any* format - since current FFmpeg also
> > > does not propagate dolby vision RPUs, we generate broken files pretty
> > > much always when transcoding dolby vision. So we definitely need to
> > > strip the metadata from the stream muxer *somewhere*. Where else comes
> > > to mind?
> > > 
> > > This also gets into another topic I wanted to touch on, which is that
> > > the presence of dynamic dolby vision metadata currently hinders the
> > > ability of libavfilter to treat the video primaries/gamma as
> > > a negotiable colorspace property (the way it is done currently for YUV
> > > matrix/range). This is because when interpreted as such, DV metadata
> > > fundamentally changes the colorspace of the incoming video stream.
> > > Ideally we would like some way to negotiate DV metadata on the
> > > query_formats() level.
> > > 
> > > Ideally, we'd want something like AVCOL_SPC_DOLBYVISION, but we can't
> > > easily introduce that without breaking ISO/IEC 23091 compatibility..
> > 
> > In principle it could be yet another negotiated field, could it not? You
> > just added a bunch of those recently, what's another one?
> 
> Adding more fields to this negotiation process is a very obnoxious and
> tedious process, with lots of boilerplate for each new field added.
> Maybe we can come up with some better mechanism first?

Jan Ekström pointed out to me that the newest iteration of H.273
contains markers for dolby vision, so we can support this in the obvious
way (at least for profile 5). Getting the colorspace selection exactly
right for profiles 8.1/8.4 is not so important. That leaves only profile
8.2 as the annoying special case, but it's a rarely used profile so
maybe we can just ignore it.

The downside of not having negotiation here really boils down to just
-vf libplacebo doing tone-mapping to SDR where it may perhaps not be
expected.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/4] fftools/ffmpeg_enc: strip DOVI config record for AV1

[Niklas Haas]

On Tue, 19 Mar 2024 20:16:39 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> From: Niklas Haas <git@haasn.dev>
> 
> AV1 streams don't use configuration records, so delete them when
> encoding to AV1. Ideally this would be, as the comment suggests, handled
> at the frame-level (and stripped by the av1 encoder), but given the
> status quo of copying the packet-level data here directly, we should
> definitely make an effort to strip it.

I decided to abandon this series in favor of synthesizing the RPU fully
from the parsed AVDOVIMetadata side data. I have a series for this
prepared and mostly ready, will submit it sometime in the next week.

That series also includes a new option to avoid sending RPU data when
dolby vision is not enabled.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".