[FFmpeg-devel] [PATCH v3 0/2] avcodec/dovi - correctly read el_bit_depth_minus8 and ext_mapping_idc

[FFmpeg-devel] [PATCH v3 0/2] avcodec/dovi - correctly read el_bit_depth_minus8 and ext_mapping_idc

[Cosmin Stejerean via ffmpeg-devel]

From: Cosmin Stejerean <cosmin@cosmin.at>

Some DolbyVision samples fail to parse currently due to mis-reading the
el_bit_depth_minus8 field. Upon investigation it seems that the RPU syntax has
been extended in an as of yet undocumented way by adding ext_mapping_idc and
coding it together with el_bit_depth_minus8 together into a single 16 bit
integer with the upper 8 bits for ext_mapping_idc and the lower 8 bits for
el_bit_depth_minus8.

This can be observed in the output of the DoVi verifier, which shows how this
is laid out. This patchset adds the new fields to dovi_meta and implements the
code to parse and write this back out.

Compared to the previous version it moves the fields to the end for ABI
compatibility, bumps the minor version in lavu and splits this into a separate
commit.

Cosmin Stejerean (2):
  lavu/dovi_meta - add fields for ext_mapping_idc
  avcodec/dovi - correctly read el_bit_depth_minus8 and ext_mapping_idc

 libavcodec/dovi_rpudec.c | 7 ++++++-
 libavcodec/dovi_rpuenc.c | 4 +++-
 libavutil/dovi_meta.h    | 2 ++
 libavutil/version.h      | 2 +-
 4 files changed, 12 insertions(+), 3 deletions(-)

-- 
2.42.1


_______________________________________________
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 v3 0/2] avcodec/dovi - correctly read el_bit_depth_minus8 and ext_mapping_idc

[Niklas Haas]

On Wed, 22 May 2024 15:50:34 +0000 Cosmin Stejerean via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote:
> From: Cosmin Stejerean <cosmin@cosmin.at>
> 
> Some DolbyVision samples fail to parse currently due to mis-reading the
> el_bit_depth_minus8 field. Upon investigation it seems that the RPU syntax has
> been extended in an as of yet undocumented way by adding ext_mapping_idc and
> coding it together with el_bit_depth_minus8 together into a single 16 bit
> integer with the upper 8 bits for ext_mapping_idc and the lower 8 bits for
> el_bit_depth_minus8.
> 
> This can be observed in the output of the DoVi verifier, which shows how this
> is laid out. This patchset adds the new fields to dovi_meta and implements the
> code to parse and write this back out.
> 
> Compared to the previous version it moves the fields to the end for ABI
> compatibility, bumps the minor version in lavu and splits this into a separate
> commit.
> 
> Cosmin Stejerean (2):
>   lavu/dovi_meta - add fields for ext_mapping_idc
>   avcodec/dovi - correctly read el_bit_depth_minus8 and ext_mapping_idc
> 
>  libavcodec/dovi_rpudec.c | 7 ++++++-
>  libavcodec/dovi_rpuenc.c | 4 +++-
>  libavutil/dovi_meta.h    | 2 ++
>  libavutil/version.h      | 2 +-
>  4 files changed, 12 insertions(+), 3 deletions(-)
> 
> -- 
> 2.42.1
> 
> 
> _______________________________________________
> 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".

As discussed on IRC, this needs an extra patch to update the `last
field` entry in `dovi_rpu.c`.

Apart from that, LGTM. Will apply tomorrow (with the mentioned change)
if there is no objection.
_______________________________________________
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 v3 1/2] lavu/dovi_meta - add fields for ext_mapping_idc

[Cosmin Stejerean via ffmpeg-devel]

From: Cosmin Stejerean <cosmin@cosmin.at>

---
 libavutil/dovi_meta.h | 2 ++
 libavutil/version.h   | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavutil/dovi_meta.h b/libavutil/dovi_meta.h
index e10332f8d7..e168075a24 100644
--- a/libavutil/dovi_meta.h
+++ b/libavutil/dovi_meta.h
@@ -91,6 +91,8 @@ typedef struct AVDOVIRpuDataHeader {
     uint8_t spatial_resampling_filter_flag;
     uint8_t el_spatial_resampling_filter_flag;
     uint8_t disable_residual_flag;
+    uint8_t ext_mapping_idc_0_4; /* extended base layer inverse mapping indicator */
+    uint8_t ext_mapping_idc_5_7; /* reserved */
 } AVDOVIRpuDataHeader;
 
 enum AVDOVIMappingMethod {
diff --git a/libavutil/version.h b/libavutil/version.h
index 3221c4c592..9c7146c228 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  59
-#define LIBAVUTIL_VERSION_MINOR  19
+#define LIBAVUTIL_VERSION_MINOR  20
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
-- 
2.42.1


_______________________________________________
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 v3 1/2] lavu/dovi_meta - add fields for ext_mapping_idc

[Niklas Haas]

On Wed, 22 May 2024 15:50:43 +0000 Cosmin Stejerean via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote:
> From: Cosmin Stejerean <cosmin@cosmin.at>
> 
> ---
>  libavutil/dovi_meta.h | 2 ++
>  libavutil/version.h   | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/dovi_meta.h b/libavutil/dovi_meta.h
> index e10332f8d7..e168075a24 100644
> --- a/libavutil/dovi_meta.h
> +++ b/libavutil/dovi_meta.h
> @@ -91,6 +91,8 @@ typedef struct AVDOVIRpuDataHeader {
>      uint8_t spatial_resampling_filter_flag;
>      uint8_t el_spatial_resampling_filter_flag;
>      uint8_t disable_residual_flag;
> +    uint8_t ext_mapping_idc_0_4; /* extended base layer inverse mapping indicator */
> +    uint8_t ext_mapping_idc_5_7; /* reserved */
>  } AVDOVIRpuDataHeader;

What value ranges have you seen for this indicator? Is it possible that
some values would extend the RPU in other ways, adding more bits that we
need to parse?

Maybe we should enforce this to be a well-known value just to be on the
safe side, until we receive public documentation on its purpose.

>  
>  enum AVDOVIMappingMethod {
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 3221c4c592..9c7146c228 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  59
> -#define LIBAVUTIL_VERSION_MINOR  19
> +#define LIBAVUTIL_VERSION_MINOR  20
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
> -- 
> 2.42.1
> 
> 
> _______________________________________________
> 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 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 v3 1/2] lavu/dovi_meta - add fields for ext_mapping_idc

[Cosmin Stejerean via ffmpeg-devel]

> On May 23, 2024, at 4:03 AM, Niklas Haas <ffmpeg@haasn.xyz> wrote:
> 
> On Wed, 22 May 2024 15:50:43 +0000 Cosmin Stejerean via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> wrote:
>> From: Cosmin Stejerean <cosmin@cosmin.at>
>> 
>> ---
>> libavutil/dovi_meta.h | 2 ++
>> libavutil/version.h   | 2 +-
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/libavutil/dovi_meta.h b/libavutil/dovi_meta.h
>> index e10332f8d7..e168075a24 100644
>> --- a/libavutil/dovi_meta.h
>> +++ b/libavutil/dovi_meta.h
>> @@ -91,6 +91,8 @@ typedef struct AVDOVIRpuDataHeader {
>>     uint8_t spatial_resampling_filter_flag;
>>     uint8_t el_spatial_resampling_filter_flag;
>>     uint8_t disable_residual_flag;
>> +    uint8_t ext_mapping_idc_0_4; /* extended base layer inverse mapping indicator */
>> +    uint8_t ext_mapping_idc_5_7; /* reserved */
>> } AVDOVIRpuDataHeader;
> 
> What value ranges have you seen for this indicator? Is it possible that
> some values would extend the RPU in other ways, adding more bits that we
> need to parse?

So far I've only seen two sets of values. 

The samples from the iPhone:

ext_mapping_idc[4:0] = 0
ext_mapping_idc[7:5] = 0

The samples from Xiaomi and Oppo:

ext_mapping_idc[4:0] = 1
ext_mapping_idc[7:5] = 4

I don't have any additional information on what the possible range of values could be so far.
As far as I can tell it has no impact on the size of the subsequent fields, at least for what I've encountered so far.


> 
> Maybe we should enforce this to be a well-known value just to be on the
> safe side, until we receive public documentation on its purpose.
> 

I think to avoid being brittle we should allow the entire range of values for now until we know better, and just ensure it roundtrips correctly (which I've verified by running the validator on the 10.4 output transcoded from 8.4 input).

Once we track down more info we can try to add more validation on top of it.

- 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".

[FFmpeg-devel] [PATCH v3 2/2] avcodec/dovi - correctly read el_bit_depth_minus8 and ext_mapping_idc

[Cosmin Stejerean via ffmpeg-devel]

From: Cosmin Stejerean <cosmin@cosmin.at>

These two fields are coded together into a single 16 bit integer with upper 8
bits for ext_mapping_idc and lower 8 bits for el_bit_depth_minus8.

Furthermore ext_mapping_idc has two components, upper 3 bits and lower 5 bits.
---
 libavcodec/dovi_rpudec.c | 7 ++++++-
 libavcodec/dovi_rpuenc.c | 4 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
index 7c7eda9d09..af41ab5827 100644
--- a/libavcodec/dovi_rpudec.c
+++ b/libavcodec/dovi_rpudec.c
@@ -411,13 +411,18 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
 
         if ((hdr->rpu_format & 0x700) == 0) {
             int bl_bit_depth_minus8 = get_ue_golomb_31(gb);
-            int el_bit_depth_minus8 = get_ue_golomb_31(gb);
+            int el_bit_depth_minus8_and_ext_mapping_idc = get_ue_golomb_long(gb);
+            int el_bit_depth_minus8 = el_bit_depth_minus8_and_ext_mapping_idc & 0xFF; // lowest 8 bits
+            int ext_mapping_idc = (el_bit_depth_minus8_and_ext_mapping_idc & 0xFF00) >> 8; // upper 8 bits
+
             int vdr_bit_depth_minus8 = get_ue_golomb_31(gb);
             VALIDATE(bl_bit_depth_minus8, 0, 8);
             VALIDATE(el_bit_depth_minus8, 0, 8);
             VALIDATE(vdr_bit_depth_minus8, 0, 8);
             hdr->bl_bit_depth = bl_bit_depth_minus8 + 8;
             hdr->el_bit_depth = el_bit_depth_minus8 + 8;
+            hdr->ext_mapping_idc_0_4 = ext_mapping_idc & 0x1F; // lowest 5 bits of ext_mapping_idc
+            hdr->ext_mapping_idc_5_7 = (ext_mapping_idc & 0xE0) >> 5; // upper 3 bits of ext_mapping_idc
             hdr->vdr_bit_depth = vdr_bit_depth_minus8 + 8;
             hdr->spatial_resampling_filter_flag = get_bits1(gb);
             skip_bits(gb, 3); /* reserved_zero_3bits */
diff --git a/libavcodec/dovi_rpuenc.c b/libavcodec/dovi_rpuenc.c
index 3c3e0f84c0..91c0a85050 100644
--- a/libavcodec/dovi_rpuenc.c
+++ b/libavcodec/dovi_rpuenc.c
@@ -444,6 +444,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
     int vdr_dm_metadata_changed, vdr_rpu_id, use_prev_vdr_rpu, profile,
         buffer_size, rpu_size, pad, zero_run;
     int num_ext_blocks_v1, num_ext_blocks_v2;
+    uint8_t ext_mapping_idc;
     uint32_t crc;
     uint8_t *dst;
     if (!metadata) {
@@ -551,7 +552,8 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
     put_bits(pb, 1, hdr->bl_video_full_range_flag);
     if ((hdr->rpu_format & 0x700) == 0) {
         set_ue_golomb(pb, hdr->bl_bit_depth - 8);
-        set_ue_golomb(pb, hdr->el_bit_depth - 8);
+        ext_mapping_idc = (hdr->ext_mapping_idc_5_7 << 5) | hdr->ext_mapping_idc_0_4;
+        set_ue_golomb(pb, (ext_mapping_idc << 8) | hdr->el_bit_depth - 8);
         set_ue_golomb(pb, hdr->vdr_bit_depth - 8);
         put_bits(pb, 1, hdr->spatial_resampling_filter_flag);
         put_bits(pb, 3, 0); /* reserved_zero_3bits */
-- 
2.42.1


_______________________________________________
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 v3 2/2] avcodec/dovi - correctly read el_bit_depth_minus8 and ext_mapping_idc

[James Almer]

On 5/22/2024 12:50 PM, Cosmin Stejerean via ffmpeg-devel wrote:
> From: Cosmin Stejerean <cosmin@cosmin.at>
> 
> These two fields are coded together into a single 16 bit integer with upper 8
> bits for ext_mapping_idc and lower 8 bits for el_bit_depth_minus8.
> 
> Furthermore ext_mapping_idc has two components, upper 3 bits and lower 5 bits.
> ---
>   libavcodec/dovi_rpudec.c | 7 ++++++-
>   libavcodec/dovi_rpuenc.c | 4 +++-
>   2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
> index 7c7eda9d09..af41ab5827 100644
> --- a/libavcodec/dovi_rpudec.c
> +++ b/libavcodec/dovi_rpudec.c
> @@ -411,13 +411,18 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
>   
>           if ((hdr->rpu_format & 0x700) == 0) {
>               int bl_bit_depth_minus8 = get_ue_golomb_31(gb);
> -            int el_bit_depth_minus8 = get_ue_golomb_31(gb);
> +            int el_bit_depth_minus8_and_ext_mapping_idc = get_ue_golomb_long(gb);
> +            int el_bit_depth_minus8 = el_bit_depth_minus8_and_ext_mapping_idc & 0xFF; // lowest 8 bits
> +            int ext_mapping_idc = (el_bit_depth_minus8_and_ext_mapping_idc & 0xFF00) >> 8; // upper 8 bits

nit: we usually do (foo >> 8) & 0xFF.

Also, coding two (maybe three) values in a single golomb code is evil.

> +
>               int vdr_bit_depth_minus8 = get_ue_golomb_31(gb);
>               VALIDATE(bl_bit_depth_minus8, 0, 8);
>               VALIDATE(el_bit_depth_minus8, 0, 8);
>               VALIDATE(vdr_bit_depth_minus8, 0, 8);
>               hdr->bl_bit_depth = bl_bit_depth_minus8 + 8;
>               hdr->el_bit_depth = el_bit_depth_minus8 + 8;
> +            hdr->ext_mapping_idc_0_4 = ext_mapping_idc & 0x1F; // lowest 5 bits of ext_mapping_idc
> +            hdr->ext_mapping_idc_5_7 = (ext_mapping_idc & 0xE0) >> 5; // upper 3 bits of ext_mapping_idc

Ditto.

>               hdr->vdr_bit_depth = vdr_bit_depth_minus8 + 8;
>               hdr->spatial_resampling_filter_flag = get_bits1(gb);
>               skip_bits(gb, 3); /* reserved_zero_3bits */
> diff --git a/libavcodec/dovi_rpuenc.c b/libavcodec/dovi_rpuenc.c
> index 3c3e0f84c0..91c0a85050 100644
> --- a/libavcodec/dovi_rpuenc.c
> +++ b/libavcodec/dovi_rpuenc.c
> @@ -444,6 +444,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
>       int vdr_dm_metadata_changed, vdr_rpu_id, use_prev_vdr_rpu, profile,
>           buffer_size, rpu_size, pad, zero_run;
>       int num_ext_blocks_v1, num_ext_blocks_v2;
> +    uint8_t ext_mapping_idc;
>       uint32_t crc;
>       uint8_t *dst;
>       if (!metadata) {
> @@ -551,7 +552,8 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
>       put_bits(pb, 1, hdr->bl_video_full_range_flag);
>       if ((hdr->rpu_format & 0x700) == 0) {
>           set_ue_golomb(pb, hdr->bl_bit_depth - 8);
> -        set_ue_golomb(pb, hdr->el_bit_depth - 8);
> +        ext_mapping_idc = (hdr->ext_mapping_idc_5_7 << 5) | hdr->ext_mapping_idc_0_4;
> +        set_ue_golomb(pb, (ext_mapping_idc << 8) | hdr->el_bit_depth - 8);
>           set_ue_golomb(pb, hdr->vdr_bit_depth - 8);
>           put_bits(pb, 1, hdr->spatial_resampling_filter_flag);
>           put_bits(pb, 3, 0); /* reserved_zero_3bits */
_______________________________________________
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".