diff mbox series

[FFmpeg-devel] avdovi/dovi_rpu{enc, dec}: fix ms_weight handling

Message ID 20240525161839.34867-1-ffmpeg@haasn.xyz
State New
Headers show
Series [FFmpeg-devel] avdovi/dovi_rpu{enc, dec}: fix ms_weight handling | expand

Checks

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

Commit Message

Niklas Haas May 25, 2024, 4:18 p.m. UTC 
From: Niklas Haas <git@haasn.dev>

The code as written was wrong. The l2.ms_weight value is coded as
a *signed* integer, rather than a shifted unsigned integer. (And even
so, the offset of 8192 would be too large, since 2^12 = 4096. Ditto for
l8.ms_weight, which should have an offset of 2048, not 8192)

In addition, because the l8.ms_weight struct member is unsigned, these
shifting semantics ended up overflowing the field, leading to undefined
behavior when transcoding. Fortunately, the damage was relatively
contained in practice, because it just corrupts the coding of this
field, which is ignored in practice in all implementations I have seen.

For some reason, Dolby decided to add a sign bit to l2.ms_weight but not
l3.ms_weight. In either case, it's quite probably that this is still
a shifted unsigned integer in disguise, since all samples seem to have
a value of 2048 for these fields. But without specs, that's guesswork,
and the official DV validator also reports these fields as unsigned
integers with a value of 2048.
---
 libavcodec/dovi_rpudec.c | 4 ++--
 libavcodec/dovi_rpuenc.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
diff mbox series

Patch

diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
index 7c7eda9d09..a477dbd4e3 100644
--- a/libavcodec/dovi_rpudec.c
+++ b/libavcodec/dovi_rpudec.c
@@ -142,7 +142,7 @@  static void parse_ext_v1(DOVIContext *s, GetBitContext *gb, AVDOVIDmData *dm)
         dm->l2.trim_power = get_bits(gb, 12);
         dm->l2.trim_chroma_weight = get_bits(gb, 12);
         dm->l2.trim_saturation_gain = get_bits(gb, 12);
-        dm->l2.ms_weight = get_bits(gb, 13) - 8192;
+        dm->l2.ms_weight = get_sbits(gb, 13);
         break;
     case 4:
         dm->l4.anchor_pq = get_bits(gb, 12);
@@ -197,7 +197,7 @@  static void parse_ext_v2(DOVIContext *s, GetBitContext *gb, AVDOVIDmData *dm,
         dm->l8.trim_power = get_bits(gb, 12);
         dm->l8.trim_chroma_weight = get_bits(gb, 12);
         dm->l8.trim_saturation_gain = get_bits(gb, 12);
-        dm->l8.ms_weight = get_bits(gb, 12) - 8192;
+        dm->l8.ms_weight = get_bits(gb, 12);
         if (ext_block_length < 12)
             break;
         dm->l8.target_mid_contrast = get_bits(gb, 12);
diff --git a/libavcodec/dovi_rpuenc.c b/libavcodec/dovi_rpuenc.c
index 3c3e0f84c0..dacb8b54e7 100644
--- a/libavcodec/dovi_rpuenc.c
+++ b/libavcodec/dovi_rpuenc.c
@@ -276,7 +276,7 @@  static void generate_ext_v1(PutBitContext *pb, const AVDOVIDmData *dm)
         put_bits(pb, 12, dm->l2.trim_power);
         put_bits(pb, 12, dm->l2.trim_chroma_weight);
         put_bits(pb, 12, dm->l2.trim_saturation_gain);
-        put_bits(pb, 13, dm->l2.ms_weight + 8192);
+        put_sbits(pb, 13, dm->l2.ms_weight);
         break;
     case 4:
         put_bits(pb, 12, dm->l4.anchor_pq);
@@ -374,7 +374,7 @@  static void generate_ext_v2(PutBitContext *pb, const AVDOVIDmData *dm)
         put_bits(pb, 12, dm->l8.trim_power);
         put_bits(pb, 12, dm->l8.trim_chroma_weight);
         put_bits(pb, 12, dm->l8.trim_saturation_gain);
-        put_bits(pb, 12, dm->l8.ms_weight + 8192);
+        put_bits(pb, 12, dm->l8.ms_weight);
         if (ext_block_length < 12)
             break;
         put_bits(pb, 12, dm->l8.target_mid_contrast);