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