diff mbox series

[FFmpeg-devel,v3] avformat/mxfdec: Read video range from PictureDescriptor

Message ID E97D56BD-9E0C-45A1-A69C-EF16C5185117@codex.online
State Superseded
Headers show
Series [FFmpeg-devel,v3] avformat/mxfdec: Read video range from PictureDescriptor | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Harry Mallon Aug. 12, 2020, 12:43 p.m. UTC 
I'm very sorry for the noise, I just thought of a way that the last patch could be troublesome. Fixed here.

From 31ce817887ec84907b3aadb5dc1657b01b8d0dbd Mon Sep 17 00:00:00 2001

From: Harry Mallon <harry.mallon@codex.online>
Date: Wed, 12 Aug 2020 10:26:23 +0100
Subject: [PATCH v3] avformat/mxfdec: Read video range from PictureDescriptor

* Capture black_ref, white_ref and color_range and recognise
  full and narrow range.

Signed-off-by: Harry Mallon <harry.mallon@codex.online>
---
 libavformat/mxfdec.c           | 24 ++++++++++++++++++++++++
 tests/ref/fate/mxf-probe-dnxhd |  2 +-
 tests/ref/fate/mxf-probe-dv25  |  2 +-
 3 files changed, 26 insertions(+), 2 deletions(-)

Comments

Tomas Härdin Aug. 12, 2020, 1:59 p.m. UTC | #1 
ons 2020-08-12 klockan 13:43 +0100 skrev Harry Mallon:
> @@ -2492,6 +2504,18 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>              }
>              if (descriptor->aspect_ratio.num && descriptor->aspect_ratio.den)
>                  st->display_aspect_ratio = descriptor->aspect_ratio;
> +            if (descriptor->component_depth &&
> +                descriptor->black_ref_level == 0 &&
> +                descriptor->white_ref_level == ((1<<descriptor->component_depth) - 1) &&
> +                descriptor->color_range     == ((1<<descriptor->component_depth) - 1)) {
> +                st->codecpar->color_range = AVCOL_RANGE_JPEG;
> +            }
> +            else if (descriptor->component_depth >= 8 &&
> +                     descriptor->black_ref_level == (1  <<(descriptor->component_depth - 4)) &&
> +                     descriptor->white_ref_level == (235<<(descriptor->component_depth - 8)) &&
> +                     descriptor->color_range     == ((14<<(descriptor->component_depth - 4)) + 1)) {
> +                st->codecpar->color_range = AVCOL_RANGE_MPEG;
> +            }

Put a warning here in case levels are set but neither of these two ifs
are true, perhaps using avpriv_request_sample(). I can imagine there's
encoders that put off-by-one values in here. I'd like to see such files
first though before we widen these if clauses, so we can put the
samples in FATE.

I'm testing the previous patch. Will push that one once FATE passes.

/Tomas
Harry Mallon Aug. 13, 2020, 9:25 a.m. UTC | #2 
> On 12 Aug 2020, at 14:59, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> 
> ons 2020-08-12 klockan 13:43 +0100 skrev Harry Mallon:
>> @@ -2492,6 +2504,18 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>>             }
>>             if (descriptor->aspect_ratio.num && descriptor->aspect_ratio.den)
>>                 st->display_aspect_ratio = descriptor->aspect_ratio;
>> +            if (descriptor->component_depth &&
>> +                descriptor->black_ref_level == 0 &&
>> +                descriptor->white_ref_level == ((1<<descriptor->component_depth) - 1) &&
>> +                descriptor->color_range     == ((1<<descriptor->component_depth) - 1)) {
>> +                st->codecpar->color_range = AVCOL_RANGE_JPEG;
>> +            }
>> +            else if (descriptor->component_depth >= 8 &&
>> +                     descriptor->black_ref_level == (1  <<(descriptor->component_depth - 4)) &&
>> +                     descriptor->white_ref_level == (235<<(descriptor->component_depth - 8)) &&
>> +                     descriptor->color_range     == ((14<<(descriptor->component_depth - 4)) + 1)) {
>> +                st->codecpar->color_range = AVCOL_RANGE_MPEG;
>> +            }
> 
> Put a warning here in case levels are set but neither of these two ifs
> are true, perhaps using avpriv_request_sample(). I can imagine there's
> encoders that put off-by-one values in here. I'd like to see such files
> first though before we widen these if clauses, so we can put the
> samples in FATE.
> 
> I'm testing the previous patch. Will push that one once FATE passes.
> 
> /Tomas
> 

Thanks Tomas, here is an updated V4

From 5866d0dc5536a6ea3f6a899c3d09f19df083c16a Mon Sep 17 00:00:00 2001
From: Harry Mallon <harry.mallon@codex.online>
Date: Wed, 12 Aug 2020 10:26:23 +0100
Subject: [PATCH v4] avformat/mxfdec: Read video range from PictureDescriptor

* Capture black_ref, white_ref and color_range and recognise
  full and narrow range.

Signed-off-by: Harry Mallon <harry.mallon@codex.online>
---
 libavformat/mxfdec.c           | 29 +++++++++++++++++++++++++++++
 tests/ref/fate/mxf-probe-dnxhd |  2 +-
 tests/ref/fate/mxf-probe-dv25  |  2 +-
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 4b56984b77..0831ad5768 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -199,6 +199,9 @@ typedef struct MXFDescriptor {
     int bits_per_sample;
     int64_t duration; /* ContainerDuration optional property */
     unsigned int component_depth;
+    unsigned int black_ref_level;
+    unsigned int white_ref_level;
+    unsigned int color_range;
     unsigned int horiz_subsampling;
     unsigned int vert_subsampling;
     UID *sub_descriptors_refs;
@@ -1222,6 +1225,15 @@ static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
     case 0x3302:
         descriptor->horiz_subsampling = avio_rb32(pb);
         break;
+    case 0x3304:
+        descriptor->black_ref_level = avio_rb32(pb);
+        break;
+    case 0x3305:
+        descriptor->white_ref_level = avio_rb32(pb);
+        break;
+    case 0x3306:
+        descriptor->color_range = avio_rb32(pb);
+        break;
     case 0x3308:
         descriptor->vert_subsampling = avio_rb32(pb);
         break;
@@ -2492,6 +2504,23 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
             }
             if (descriptor->aspect_ratio.num && descriptor->aspect_ratio.den)
                 st->display_aspect_ratio = descriptor->aspect_ratio;
+            if (descriptor->component_depth &&
+                descriptor->black_ref_level == 0 &&
+                descriptor->white_ref_level == ((1<<descriptor->component_depth) - 1) &&
+                descriptor->color_range     == ((1<<descriptor->component_depth) - 1)) {
+                st->codecpar->color_range = AVCOL_RANGE_JPEG;
+            }
+            else if (descriptor->component_depth >= 8 &&
+                     descriptor->black_ref_level == (1  <<(descriptor->component_depth - 4)) &&
+                     descriptor->white_ref_level == (235<<(descriptor->component_depth - 8)) &&
+                     descriptor->color_range     == ((14<<(descriptor->component_depth - 4)) + 1)) {
+                st->codecpar->color_range = AVCOL_RANGE_MPEG;
+            }
+            else if (descriptor->black_ref_level || descriptor->white_ref_level || descriptor->color_range) {
+                avpriv_request_sample(mxf->fc, "Unrecognized color range (range %d, b %d, w %d, depth %d)",
+                                      descriptor->color_range, descriptor->black_ref_level,
+                                      descriptor->white_ref_level, descriptor->component_depth);
+            }
             st->codecpar->color_primaries = mxf_get_codec_ul(ff_mxf_color_primaries_uls, &descriptor->color_primaries_ul)->id;
             st->codecpar->color_trc       = mxf_get_codec_ul(ff_mxf_color_trc_uls, &descriptor->color_trc_ul)->id;
             st->codecpar->color_space     = mxf_get_codec_ul(ff_mxf_color_space_uls, &descriptor->color_space_ul)->id;
diff --git a/tests/ref/fate/mxf-probe-dnxhd b/tests/ref/fate/mxf-probe-dnxhd
index 012d3ea1d9..5a6221b1d5 100644
--- a/tests/ref/fate/mxf-probe-dnxhd
+++ b/tests/ref/fate/mxf-probe-dnxhd
@@ -124,7 +124,7 @@ sample_aspect_ratio=1:1
 display_aspect_ratio=4:3
 pix_fmt=yuv422p
 level=-99
-color_range=unknown
+color_range=tv
 color_space=bt709
 color_transfer=bt709
 color_primaries=bt709
diff --git a/tests/ref/fate/mxf-probe-dv25 b/tests/ref/fate/mxf-probe-dv25
index 810f145f41..ffd26c4dfd 100644
--- a/tests/ref/fate/mxf-probe-dv25
+++ b/tests/ref/fate/mxf-probe-dv25
@@ -16,7 +16,7 @@ sample_aspect_ratio=16:15
 display_aspect_ratio=4:3
 pix_fmt=yuv420p
 level=-99
-color_range=unknown
+color_range=tv
 color_space=unknown
 color_transfer=bt470m
 color_primaries=unknown
diff mbox series

Patch

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 4b56984b77..172210acfe 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -199,6 +199,9 @@  typedef struct MXFDescriptor {
     int bits_per_sample;
     int64_t duration; /* ContainerDuration optional property */
     unsigned int component_depth;
+    unsigned int black_ref_level;
+    unsigned int white_ref_level;
+    unsigned int color_range;
     unsigned int horiz_subsampling;
     unsigned int vert_subsampling;
     UID *sub_descriptors_refs;
@@ -1222,6 +1225,15 @@  static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
     case 0x3302:
         descriptor->horiz_subsampling = avio_rb32(pb);
         break;
+    case 0x3304:
+        descriptor->black_ref_level = avio_rb32(pb);
+        break;
+    case 0x3305:
+        descriptor->white_ref_level = avio_rb32(pb);
+        break;
+    case 0x3306:
+        descriptor->color_range = avio_rb32(pb);
+        break;
     case 0x3308:
         descriptor->vert_subsampling = avio_rb32(pb);
         break;
@@ -2492,6 +2504,18 @@  static int mxf_parse_structural_metadata(MXFContext *mxf)
             }
             if (descriptor->aspect_ratio.num && descriptor->aspect_ratio.den)
                 st->display_aspect_ratio = descriptor->aspect_ratio;
+            if (descriptor->component_depth &&
+                descriptor->black_ref_level == 0 &&
+                descriptor->white_ref_level == ((1<<descriptor->component_depth) - 1) &&
+                descriptor->color_range     == ((1<<descriptor->component_depth) - 1)) {
+                st->codecpar->color_range = AVCOL_RANGE_JPEG;
+            }
+            else if (descriptor->component_depth >= 8 &&
+                     descriptor->black_ref_level == (1  <<(descriptor->component_depth - 4)) &&
+                     descriptor->white_ref_level == (235<<(descriptor->component_depth - 8)) &&
+                     descriptor->color_range     == ((14<<(descriptor->component_depth - 4)) + 1)) {
+                st->codecpar->color_range = AVCOL_RANGE_MPEG;
+            }
             st->codecpar->color_primaries = mxf_get_codec_ul(ff_mxf_color_primaries_uls, &descriptor->color_primaries_ul)->id;
             st->codecpar->color_trc       = mxf_get_codec_ul(ff_mxf_color_trc_uls, &descriptor->color_trc_ul)->id;
             st->codecpar->color_space     = mxf_get_codec_ul(ff_mxf_color_space_uls, &descriptor->color_space_ul)->id;
diff --git a/tests/ref/fate/mxf-probe-dnxhd b/tests/ref/fate/mxf-probe-dnxhd
index 012d3ea1d9..5a6221b1d5 100644
--- a/tests/ref/fate/mxf-probe-dnxhd
+++ b/tests/ref/fate/mxf-probe-dnxhd
@@ -124,7 +124,7 @@  sample_aspect_ratio=1:1
 display_aspect_ratio=4:3
 pix_fmt=yuv422p
 level=-99
-color_range=unknown
+color_range=tv
 color_space=bt709
 color_transfer=bt709
 color_primaries=bt709
diff --git a/tests/ref/fate/mxf-probe-dv25 b/tests/ref/fate/mxf-probe-dv25
index 810f145f41..ffd26c4dfd 100644
--- a/tests/ref/fate/mxf-probe-dv25
+++ b/tests/ref/fate/mxf-probe-dv25
@@ -16,7 +16,7 @@  sample_aspect_ratio=16:15
 display_aspect_ratio=4:3
 pix_fmt=yuv420p
 level=-99
-color_range=unknown
+color_range=tv
 color_space=unknown
 color_transfer=bt470m
 color_primaries=unknown