[FFmpeg-devel] [PATCH] MXF - Add jpeg2000 subdescriptor - Sponsored by INA

[Tomas Härdin]

ons 2023-04-05 klockan 15:05 +0200 skrev Cédric Le Barz:
> Le 03/04/2023 à 17:14, Michael Niedermayer a écrit :
> > On Mon, Apr 03, 2023 at 10:08:25AM +0200, Cédric Le Barz wrote:
> > > Hi,
> > > 
> > > I've attached the patch to this mail, in order to solve newlines
> > insertion
> > > issue.
> > Please make sure each patch also updates the fate tests so
> > make fate
> > doesnt fail
> I've attached to this mail the new patch. Fate test issue is fixed.
> 

Please avoid top posting.

I was actually about to suggest merging these two patches but I see you
read my mind :)

> @@ -1131,9 +1164,9 @@ static const UID mxf_aes3_descriptor_key      =
> { 0x06,0x0E,0x2B,0x34,0x02,0x53,
>  static const UID mxf_cdci_descriptor_key      = {
> 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01
> ,0x28,0x00 };
>  static const UID mxf_rgba_descriptor_key      = {
> 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01
> ,0x29,0x00 };
>  static const UID mxf_generic_sound_descriptor_key = {
> 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01
> ,0x42,0x00 };
> -

Stray line deletion

> +    mxf_write_local_tag(s, 2, 0x8401);
> +    avio_wb16(pb, 0x0000);
> +    mxf_write_local_tag(s, 4, 0x8402);
> +    avio_wb32(pb, st->codecpar->width);
> +    mxf_write_local_tag(s, 4, 0x8403);
> +    avio_wb32(pb, st->codecpar->height);
> +    mxf_write_local_tag(s, 4, 0x8404);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 4, 0x8405);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 4, 0x8406);
> +    avio_wb32(pb, st->codecpar->width);
> +    mxf_write_local_tag(s, 4, 0x8407);
> +    avio_wb32(pb, st->codecpar->height);
> +    mxf_write_local_tag(s, 4, 0x8408);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 4, 0x8409);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 2, 0x840A);
> +    avio_wb16(pb, component_count);

A comment on each of these explaining what they are would be nice.

> +    {
> +        char _desc [3][3]= {  {0x09,0x01,0x01} , {0x09,0x02,0x01} ,
> {0x09,0x02,0x01} };
> +        int comp = 0;
> +        for ( comp = 0; comp< component_count ;comp++ ) {
> +            avio_write(pb, _desc[comp%3] , 3);
> +        }
> +    }

Maybe just a style nit but you could move the char desc[] into the loop
body, int comp to the start of the function and then you can remove the
extra {} around this. Also you could make desc static const.

> +    {
> +        char _layout[16] = {  'Y' , '\n', 'U' , '\n', 'V' , '\n',
> 'F' , 0x02,
> +                            0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00 };
> +        avio_write(pb, _layout , 16);
> +    }

Again there is the issue of RGB(A)

/Tomas