diff mbox series

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

Message ID 0730D7D7-22C1-465B-8242-40C3E78659CB@codex.online
State New
Headers show
Series [FFmpeg-devel,v4] 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. 13, 2020, 10:04 a.m. UTC 
Here is an updated patch (now hopefully going with correct email headers).

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

Comments

Tomas Härdin Aug. 13, 2020, 3:28 p.m. UTC | #1 
tor 2020-08-13 klockan 11:04 +0100 skrev Harry Mallon:
> Here is an updated patch (now hopefully going with correct email headers).

It would be nice if in the future you either attach the patch or make
the entire email the patch itself. I've had to trim these first couple
of lines in each of the patches so far, after doing "git am" on the
.mbox from saving your messages

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

Looks good to me. Running FATE atm, will push in a day if there are no
objections.

/Tomas
Marton Balint Aug. 13, 2020, 8:21 p.m. UTC | #2 
On Thu, 13 Aug 2020, Tomas Härdin wrote:

> tor 2020-08-13 klockan 11:04 +0100 skrev Harry Mallon:
>> Here is an updated patch (now hopefully going with correct email headers).
>
> It would be nice if in the future you either attach the patch or make
> the entire email the patch itself. I've had to trim these first couple
> of lines in each of the patches so far, after doing "git am" on the
> .mbox from saving your messages
>
>> 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(-)
>
> Looks good to me. Running FATE atm, will push in a day if there are no
> objections.

http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket4328/01_Bad_Frame_2.24.mxf 
is not detected correctly for some reason.

The MXF specs seems ambigous:

Color Range is a Property, whose unsigned 32-bit integer value shall 
specify the number of distinct values allowed for color difference 
samples.

So probably 2^depth color range should also be accepted as full range.

Regards,
Marton
Tomas Härdin Aug. 14, 2020, 10:53 a.m. UTC | #3 
tor 2020-08-13 klockan 22:21 +0200 skrev Marton Balint:
> 
> On Thu, 13 Aug 2020, Tomas Härdin wrote:
> 
> > tor 2020-08-13 klockan 11:04 +0100 skrev Harry Mallon:
> > > Here is an updated patch (now hopefully going with correct email headers).
> > 
> > It would be nice if in the future you either attach the patch or make
> > the entire email the patch itself. I've had to trim these first couple
> > of lines in each of the patches so far, after doing "git am" on the
> > .mbox from saving your messages
> > 
> > > 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(-)
> > 
> > Looks good to me. Running FATE atm, will push in a day if there are no
> > objections.
> 
> http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket4328/01_Bad_Frame_2.24.mxf 
> is not detected correctly for some reason.
> 
> The MXF specs seems ambigous:
> 
> Color Range is a Property, whose unsigned 32-bit integer value shall 
> specify the number of distinct values allowed for color difference 
> samples.
> 
> So probably 2^depth color range should also be accepted as full range.

This sounds correct. Do we have any sample using 2^depth-1? If not then
we should just go with 2^depth until such a sample emerges.

/Tomas
Harry Mallon Aug. 14, 2020, 9:33 p.m. UTC | #4 
> On 14 Aug 2020, at 11:53, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> 
> tor 2020-08-13 klockan 22:21 +0200 skrev Marton Balint:
>> 
>> On Thu, 13 Aug 2020, Tomas Härdin wrote:
>> 
>>> tor 2020-08-13 klockan 11:04 +0100 skrev Harry Mallon:
>>>> Here is an updated patch (now hopefully going with correct email headers).
>>> 
>>> It would be nice if in the future you either attach the patch or make
>>> the entire email the patch itself. I've had to trim these first couple
>>> of lines in each of the patches so far, after doing "git am" on the
>>> .mbox from saving your messages
>>> 
>>>> 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(-)
>>> 
>>> Looks good to me. Running FATE atm, will push in a day if there are no
>>> objections.
>> 
>> http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket4328/01_Bad_Frame_2.24.mxf 
>> is not detected correctly for some reason.
>> 
>> The MXF specs seems ambigous:
>> 
>> Color Range is a Property, whose unsigned 32-bit integer value shall 
>> specify the number of distinct values allowed for color difference 
>> samples.
>> 
>> So probably 2^depth color range should also be accepted as full range.
> 
> This sounds correct. Do we have any sample using 2^depth-1? If not then
> we should just go with 2^depth until such a sample emerges.
> 
> /Tomas

I based it on what mxfenc.c already did, I can try to find some other samples.

Harry

> 
> _______________________________________________
> 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".
Harry Mallon Aug. 16, 2020, 10:43 a.m. UTC | #5 
> On 14 Aug 2020, at 22:33, Harry Mallon <harry.mallon@codex.online> wrote:
> 
> 
> 
> 
>> On 14 Aug 2020, at 11:53, Tomas Härdin <tjoppen@acc.umu.se> wrote:
>> 
>> tor 2020-08-13 klockan 22:21 +0200 skrev Marton Balint:
>>> 
>>> On Thu, 13 Aug 2020, Tomas Härdin wrote:
>>> 
>>>> tor 2020-08-13 klockan 11:04 +0100 skrev Harry Mallon:
>>>>> Here is an updated patch (now hopefully going with correct email headers).
>>>> 
>>>> It would be nice if in the future you either attach the patch or make
>>>> the entire email the patch itself. I've had to trim these first couple
>>>> of lines in each of the patches so far, after doing "git am" on the
>>>> .mbox from saving your messages
>>>> 
>>>>> 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(-)
>>>> 
>>>> Looks good to me. Running FATE atm, will push in a day if there are no
>>>> objections.
>>> 
>>> http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket4328/01_Bad_Frame_2.24.mxf 
>>> is not detected correctly for some reason.
>>> 
>>> The MXF specs seems ambigous:
>>> 
>>> Color Range is a Property, whose unsigned 32-bit integer value shall 
>>> specify the number of distinct values allowed for color difference 
>>> samples.
>>> 
>>> So probably 2^depth color range should also be accepted as full range.
>> 
>> This sounds correct. Do we have any sample using 2^depth-1? If not then
>> we should just go with 2^depth until such a sample emerges.
>> 
>> /Tomas
> 
> I based it on what mxfenc.c already did, I can try to find some other samples.
> 
> Harry
> 
>> 

OK, I have checked back with the docs. 

* I agree that 2^depth is correct for mxf color_range 
* 2^depth-1 has been used in FFMPEG since n4.1 (avformat/mxfenc: add white/black ref /color range 6d0339096e10f6753049f2a5cbfd7ba69e5f8bcd) so maybe we should keep the off-by-one case, I don't mind either way.

I was checking some other MXF files I have here and one is full-range RGB J2K, rather than YUV. There are separate range metadata in RGBAEssenceDescriptor compared to CDCIEssenceDescriptor. Is there a way to get the stream component depth from this area of code (as RGBA only has component_max and component_min, no component_depth like CDCI) or somehow to pass the min/max to the codec to parse?

e.g in my file (RGB J2K with RGBAEssenceDesc) component_max is 4095 and component_min is 0, but I don't think the pixel format has been set to 12bit yet so it would seem premature to set the range to full.

Harry
Tomas Härdin Aug. 20, 2020, 10:26 a.m. UTC | #6 
sön 2020-08-16 klockan 11:43 +0100 skrev Harry Mallon:
> > > > http://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket4328/01_Bad_Frame_2.24.mxf 
> > > > is not detected correctly for some reason.
> > > > 
> > > > The MXF specs seems ambigous:
> > > > 
> > > > Color Range is a Property, whose unsigned 32-bit integer value shall 
> > > > specify the number of distinct values allowed for color difference 
> > > > samples.
> > > > 
> > > > So probably 2^depth color range should also be accepted as full range.
> > > 
> > > This sounds correct. Do we have any sample using 2^depth-1? If not then
> > > we should just go with 2^depth until such a sample emerges.
> > > 
> > > /Tomas
> > 
> > I based it on what mxfenc.c already did, I can try to find some other samples.
> > 
> > Harry
> > 
> 
> OK, I have checked back with the docs. 
> 
> * I agree that 2^depth is correct for mxf color_range 
> * 2^depth-1 has been used in FFMPEG since n4.1 (avformat/mxfenc: add
> white/black ref /color range
> 6d0339096e10f6753049f2a5cbfd7ba69e5f8bcd) so maybe we should keep the
> off-by-one case, I don't mind either way.

Ah crap. Yeah, then we do need to accept off-by-one. We could limit
that to files that have been produced by mxfenc.c if we like. Might be
more effort than it's worth. mxfenc should be fixed either way.

> I was checking some other MXF files I have here and one is full-range 
> RGB J2K, rather than YUV. There are separate range metadata in
> RGBAEssenceDescriptor compared to CDCIEssenceDescriptor. Is there a
> way to get the stream component depth from this area of code (as RGBA
> only has component_max and component_min, no component_depth like
> CDCI) or somehow to pass the min/max to the codec to parse?
> 
> e.g in my file (RGB J2K with RGBAEssenceDesc) component_max is 4095
> and component_min is 0, but I don't think the pixel format has been
> set to 12bit yet so it would seem premature to set the range to full.

I don't know anything about J2K so I can't say

/Tomas
diff mbox series

Patch

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