diff mbox series

[FFmpeg-devel,11/17] avformat/mxfenc: Store locally whether DNXHD profile is interlaced

Message ID AM7PR03MB666068A15655590B37ACB3C28F929@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit 4586de94a70a6becc075d59ff7e6374eeabdf65c
Headers show
Series [FFmpeg-devel,01/17] avformat/mxfenc: Auto-insert h264_mp4toannexb BSF if needed
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Andreas Rheinhardt Nov. 9, 2021, 6:01 p.m. UTC
It is just a flag per supported CID. So there is no reason to use
an avpriv function for this purpose.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/mxfenc.c | 47 ++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

Comments

Tomas Härdin Nov. 9, 2021, 9:37 p.m. UTC | #1
tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> It is just a flag per supported CID. So there is no reason to use
> an avpriv function for this purpose.

This is data duplication. Honestly these ULs should probably live in
dnxhddata.c.

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavformat/mxfenc.c | 47 ++++++++++++++++++++++--------------------
> --
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index aa9857fcff..326ec6a7d6 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2036,29 +2036,30 @@ static int
> mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket *pk
>  }
>  
>  static const struct {
> -    int cid;
> +    uint16_t cid;
> +    uint8_t  interlaced;
>      UID codec_ul;
>  } mxf_dnxhd_codec_uls[] = {

Not sure if the narrowing of types here does any good. You might need
to put cid and interlaced after codec_ul. On the other hand UID is
uint8_t[] so perhaps it works out.

/Tomas
Andreas Rheinhardt Nov. 9, 2021, 9:48 p.m. UTC | #2
Tomas Härdin:
> tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
>> It is just a flag per supported CID. So there is no reason to use
>> an avpriv function for this purpose.
> 
> This is data duplication. Honestly these ULs should probably live in
> dnxhddata.c.
> 

But aren't these ULs mxf-specific? So the right place for them is here
in libavformat.
And the amount of data duplicated is trivial; furthermore adding a new
DNXHD profile then and now requires modifications in two tables, so I
don't see a maintenance burden either.

>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavformat/mxfenc.c | 47 ++++++++++++++++++++++--------------------
>> --
>>  1 file changed, 23 insertions(+), 24 deletions(-)
>>
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> index aa9857fcff..326ec6a7d6 100644
>> --- a/libavformat/mxfenc.c
>> +++ b/libavformat/mxfenc.c
>> @@ -2036,29 +2036,30 @@ static int
>> mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket *pk
>>  }
>>  
>>  static const struct {
>> -    int cid;
>> +    uint16_t cid;
>> +    uint8_t  interlaced;
>>      UID codec_ul;
>>  } mxf_dnxhd_codec_uls[] = {
> 
> Not sure if the narrowing of types here does any good. You might need
> to put cid and interlaced after codec_ul. On the other hand UID is
> uint8_t[] so perhaps it works out.
> 
The narrowing is irrelevant, as all cid values in use fit into an uint16_t.
Why would I need to put it at the end? Do you worry about padding
between interlaced and codec_ul? In this case it doesn't matter: It
would just be a matter of whether the padding is in the middle or at the
end of the structure.

- Andreas
Tomas Härdin Nov. 9, 2021, 10:34 p.m. UTC | #3
tis 2021-11-09 klockan 22:48 +0100 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> > > It is just a flag per supported CID. So there is no reason to use
> > > an avpriv function for this purpose.
> > 
> > This is data duplication. Honestly these ULs should probably live
> > in
> > dnxhddata.c.
> > 
> 
> But aren't these ULs mxf-specific? So the right place for them is
> here
> in libavformat.
> And the amount of data duplicated is trivial; furthermore adding a
> new
> DNXHD profile then and now requires modifications in two tables, so I
> don't see a maintenance burden either.

Right, this improves lavc/lavf separation. I suppose that's OK. Still
not the biggest fan of having this kind of data in more than one place,
but on the other hand it's constants..

> > > 
> > > Signed-off-by: Andreas Rheinhardt
> > > <andreas.rheinhardt@outlook.com>
> > > ---
> > >  libavformat/mxfenc.c | 47 ++++++++++++++++++++++----------------
> > > ----
> > > --
> > >  1 file changed, 23 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > index aa9857fcff..326ec6a7d6 100644
> > > --- a/libavformat/mxfenc.c
> > > +++ b/libavformat/mxfenc.c
> > > @@ -2036,29 +2036,30 @@ static int
> > > mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket
> > > *pk
> > >  }
> > >  
> > >  static const struct {
> > > -    int cid;
> > > +    uint16_t cid;
> > > +    uint8_t  interlaced;
> > >      UID codec_ul;
> > >  } mxf_dnxhd_codec_uls[] = {
> > 
> > Not sure if the narrowing of types here does any good. You might
> > need
> > to put cid and interlaced after codec_ul. On the other hand UID is
> > uint8_t[] so perhaps it works out.
> > 
> The narrowing is irrelevant, as all cid values in use fit into an
> uint16_t.
> Why would I need to put it at the end? Do you worry about padding
> between interlaced and codec_ul? In this case it doesn't matter: It
> would just be a matter of whether the padding is in the middle or at
> the
> end of the structure.

I don't actually care. I just thought it was curious given patch 04

/Tomas
Andreas Rheinhardt Nov. 18, 2021, 8:42 p.m. UTC | #4
Tomas Härdin:
> tis 2021-11-09 klockan 22:48 +0100 skrev Andreas Rheinhardt:
>> Tomas Härdin:
>>> tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
>>>> It is just a flag per supported CID. So there is no reason to use
>>>> an avpriv function for this purpose.
>>>
>>> This is data duplication. Honestly these ULs should probably live
>>> in
>>> dnxhddata.c.
>>>
>>
>> But aren't these ULs mxf-specific? So the right place for them is
>> here
>> in libavformat.
>> And the amount of data duplicated is trivial; furthermore adding a
>> new
>> DNXHD profile then and now requires modifications in two tables, so I
>> don't see a maintenance burden either.
> 
> Right, this improves lavc/lavf separation. I suppose that's OK. Still
> not the biggest fan of having this kind of data in more than one place,
> but on the other hand it's constants..
> 

Ok, then I'll apply this.

>>>>
>>>> Signed-off-by: Andreas Rheinhardt
>>>> <andreas.rheinhardt@outlook.com>
>>>> ---
>>>>  libavformat/mxfenc.c | 47 ++++++++++++++++++++++----------------
>>>> ----
>>>> --
>>>>  1 file changed, 23 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>>>> index aa9857fcff..326ec6a7d6 100644
>>>> --- a/libavformat/mxfenc.c
>>>> +++ b/libavformat/mxfenc.c
>>>> @@ -2036,29 +2036,30 @@ static int
>>>> mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket
>>>> *pk
>>>>  }
>>>>  
>>>>  static const struct {
>>>> -    int cid;
>>>> +    uint16_t cid;
>>>> +    uint8_t  interlaced;
>>>>      UID codec_ul;
>>>>  } mxf_dnxhd_codec_uls[] = {
>>>
>>> Not sure if the narrowing of types here does any good. You might
>>> need
>>> to put cid and interlaced after codec_ul. On the other hand UID is
>>> uint8_t[] so perhaps it works out.
>>>
>> The narrowing is irrelevant, as all cid values in use fit into an
>> uint16_t.
>> Why would I need to put it at the end? Do you worry about padding
>> between interlaced and codec_ul? In this case it doesn't matter: It
>> would just be a matter of whether the padding is in the middle or at
>> the
>> end of the structure.
> 
> I don't actually care. I just thought it was curious given patch 04
> 
> /Tomas
>
diff mbox series

Patch

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index aa9857fcff..326ec6a7d6 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2036,29 +2036,30 @@  static int mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket *pk
 }
 
 static const struct {
-    int cid;
+    uint16_t cid;
+    uint8_t  interlaced;
     UID codec_ul;
 } mxf_dnxhd_codec_uls[] = {
-    { 1235, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x01,0x00,0x00 } }, // 1080p 10bit HIGH
-    { 1237, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x03,0x00,0x00 } }, // 1080p 8bit MED
-    { 1238, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x04,0x00,0x00 } }, // 1080p 8bit HIGH
-    { 1241, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x07,0x00,0x00 } }, // 1080i 10bit HIGH
-    { 1242, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x08,0x00,0x00 } }, // 1080i 8bit MED
-    { 1243, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x09,0x00,0x00 } }, // 1080i 8bit HIGH
-    { 1244, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x0a,0x00,0x00 } }, // 1080i 8bit TR
-    { 1250, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x10,0x00,0x00 } }, // 720p 10bit
-    { 1251, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x11,0x00,0x00 } }, // 720p 8bit HIGH
-    { 1252, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x12,0x00,0x00 } }, // 720p 8bit MED
-    { 1253, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x13,0x00,0x00 } }, // 720p 8bit LOW
-    { 1256, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x16,0x00,0x00 } }, // 1080p 10bit 444
-    { 1258, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x18,0x00,0x00 } }, // 720p 8bit TR
-    { 1259, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x19,0x00,0x00 } }, // 1080p 8bit TR
-    { 1260, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x1a,0x00,0x00 } }, // 1080i 8bit TR MBAFF
-    { 1270, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x24,0x00,0x00 } }, // DNXHR 444
-    { 1271, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x25,0x00,0x00 } }, // DNXHR HQX
-    { 1272, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x26,0x00,0x00 } }, // DNXHR HQ
-    { 1273, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x27,0x00,0x00 } }, // DNXHR SQ
-    { 1274, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x28,0x00,0x00 } }, // DNXHR LB
+    { 1235, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x01,0x00,0x00 } }, // 1080p 10bit HIGH
+    { 1237, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x03,0x00,0x00 } }, // 1080p 8bit MED
+    { 1238, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x04,0x00,0x00 } }, // 1080p 8bit HIGH
+    { 1241, 1, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x07,0x00,0x00 } }, // 1080i 10bit HIGH
+    { 1242, 1, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x08,0x00,0x00 } }, // 1080i 8bit MED
+    { 1243, 1, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x09,0x00,0x00 } }, // 1080i 8bit HIGH
+    { 1244, 1, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x0a,0x00,0x00 } }, // 1080i 8bit TR
+    { 1250, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x10,0x00,0x00 } }, // 720p 10bit
+    { 1251, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x11,0x00,0x00 } }, // 720p 8bit HIGH
+    { 1252, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x12,0x00,0x00 } }, // 720p 8bit MED
+    { 1253, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x13,0x00,0x00 } }, // 720p 8bit LOW
+    { 1256, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x16,0x00,0x00 } }, // 1080p 10bit 444
+    { 1258, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x18,0x00,0x00 } }, // 720p 8bit TR
+    { 1259, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x19,0x00,0x00 } }, // 1080p 8bit TR
+    { 1260, 1, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x1a,0x00,0x00 } }, // 1080i 8bit TR MBAFF
+    { 1270, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x24,0x00,0x00 } }, // DNXHR 444
+    { 1271, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x25,0x00,0x00 } }, // DNXHR HQX
+    { 1272, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x26,0x00,0x00 } }, // DNXHR HQ
+    { 1273, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x27,0x00,0x00 } }, // DNXHR SQ
+    { 1274, 0, { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x04,0x01,0x02,0x02,0x71,0x28,0x00,0x00 } }, // DNXHR LB
 };
 
 static int mxf_parse_dnxhd_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
@@ -2077,6 +2078,7 @@  static int mxf_parse_dnxhd_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt
     for (i = 0; i < FF_ARRAY_ELEMS(mxf_dnxhd_codec_uls); i++) {
         if (cid == mxf_dnxhd_codec_uls[i].cid) {
             sc->codec_ul = &mxf_dnxhd_codec_uls[i].codec_ul;
+            sc->interlaced = mxf_dnxhd_codec_uls[i].interlaced;
             break;
         }
     }
@@ -2098,9 +2100,6 @@  static int mxf_parse_dnxhd_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt
     if (frame_size < 0)
         return 0;
 
-    if ((sc->interlaced = avpriv_dnxhd_get_interlaced(cid)) < 0)
-        return 0;
-
     if (cid >= 1270) { // RI raster
         av_reduce(&sc->aspect_ratio.num, &sc->aspect_ratio.den,
                   st->codecpar->width, st->codecpar->height,