[FFmpeg-devel] [Patch/RFC] Improved PixelLayout parsing in mxfdec.c

[Tomas Härdin]

On Thu, 2010-06-03 at 15:11 -0700, Baptiste Coudurier wrote: 
> Hi,
> 
> On 06/03/2010 06:52 AM, Tomas H?rdin wrote:
> > Hi
> >
> > While poking around with transcoding DPX sequences to rawvideo in MXF I
> > discovered that the demuxer's handling of the PixelLayout item only
> > parses enough to fill in bits_per_coded_sample. The attached patch
> > improves the code so that it also figures out pix_fmt. At the moment it
> > detects RGB24, RGB48LE/BE and BGR24. More formats are easily added. I
> > use pix_fmt to fill in codec_tag using avcodec_pix_fmt_to_codec_tag(),
> > since that seemed the most correct way.
> 
> No, codec_tag for mxf is not set, or should be set to the container_ul 
> if there was enough space.

Ah, yes. That was a hack to get around the fact that rawdec.c overrides
pix_fmt otherwise. I've posted a patch for that in a separate thread.

> 
> > The code could be shared with the muxer in order to enable rawvideo
> > muxing. In that case the code would probably go in mxf.c instead.
> 
> That would be welcome.

Done. For now I chose not to implement a function for encoding
PixelLayout.

> 
> > Finally, I left the old switch code intact so that YUV formats still
> > produce the same value for bits_per_coded_sample.
> >
> > RFC part follows:
> >
> > Notice that I have several definitions for PIX_FMT_RGB48BE. This is due
> > to some confusion on my part. The reason is that E.2.46 appears to be a
> > little ambiguous regarding endianness. The standard provides these two
> > examples:
> >
> > "Example: Component 4:2:2:4
> > 8-bit components packed into a 32-bit word, in 601 sequence: Cb Y Cr,
> > with alpha in 4th byte of the stored pixel
> > PixelLayout = {'U',8,'Y',8,'V',8,'A',8,0,0}"
> >
> > and
> >
> > "Example: one of the formats supported by SMPTE 268M
> > 10-bit components filled to 32-bit word boundaries, padded in most
> > significant bits
> > PixelLayout = {'F',2,'B',10,'G',10,'R',10,0,0}"
> >
> > The first example states that the output bytes come in the same order as
> > they are defined in PixelLayout. The second example states that the
> > leftmost bytes are the most significant. My interpretation of this is
> > that rawvideo must be stored in big-endian form unless otherwise stated
> > in PixelLayout (using 'R' and 'r' etc. for MSBs and LSBs). A less
> > ambiguous definition of the second example would therefore be:
> >
> > PixelLayout = {'F',2,'B',6,'b',4,'G',4,'g',6,'R',2,'r',8,0,0}
> >
> > while the little-endian form would be
> >
> > PixelLayout = {'r',8,'g',6,'R',2,'b',4,'G',4,'F',2,'B',6,0,0}
> >
> > That should hopefully make sense. Any thoughts?
> 
> I'm not sure. Do you have any file ? No file -> don't need to have that 
> case yet.

Unfortunately I don't. I only have files I've created myself using
MXFLib, but I've had to encode the PixelLayout item myself. I also
looked at libMXF++ which has a rawvideo-like example. Unfortunately it
outputs UYVY using a CDCIDescriptor, which does not have a PixelLayout
item.

I'm looking into contacting SMPTE to get a statement from them regarding
little-endian video and the PixelLayout item in general.

> 
> >
> > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > index 168fd8d..d31c3d3 100644
> > --- a/libavformat/mxfdec.c
> > +++ b/libavformat/mxfdec.c
> > @@ -101,6 +101,7 @@ typedef struct {
> >       int linked_track_id;
> >       uint8_t *extradata;
> >       int extradata_size;
> > +    enum PixelFormat pix_fmt;
> >   } MXFDescriptor;
> >
> >   typedef struct {
> > @@ -518,27 +519,66 @@ static int mxf_read_index_table_segment(MXFIndexTableSegment *segment, ByteIOCon
> >       return 0;
> >   }
> >
> > +static const struct {
> > +    enum PixelFormat pix_fmt;
> > +    int length;
> > +    int bits_per_sample;
> > +    const char pixel_layout[16];
> 
> You don't need length, use terminating 0.

Fixed.

> 
>  > [...]
>  >
> > +const int num_pixel_layouts = sizeof(pixel_layouts) / sizeof(*pixel_layouts);
> > +
> >   static void mxf_read_pixel_layout(ByteIOContext *pb, MXFDescriptor *descriptor)
> >   {
> > -    int code;
> > +    int code, value, x, ofs = 0;
> > +    int64_t pix_fmt_mask = (1LL<<  num_pixel_layouts) - 1;
> >
> >       do {
> >           code = get_byte(pb);
> > +        value = get_byte(pb);
> >           dprintf(NULL, "pixel layout: code %#x\n", code);
> >           switch (code) {
> >           case 0x52: /* R */
> > -            descriptor->bits_per_sample += get_byte(pb);
> > +            descriptor->bits_per_sample += value;
> >               break;
> >           case 0x47: /* G */
> > -            descriptor->bits_per_sample += get_byte(pb);
> > +            descriptor->bits_per_sample += value;
> >               break;
> >           case 0x42: /* B */
> > -            descriptor->bits_per_sample += get_byte(pb);
> > +            descriptor->bits_per_sample += value;
> >               break;
> >           default:
> > -            get_byte(pb);
> > +            break;
> > +        }
> > +
> > +        if (code) {
> > +            /* untick layouts that are too short or that don't match */
> > +            for(x = 0; x<  num_pixel_layouts; x++) {
> > +                if (pixel_layouts[x].length<  ofs ||
> > +                    pixel_layouts[x].pixel_layout[ofs]   != code ||
> > +                    pixel_layouts[x].pixel_layout[ofs+1] != value)
> > +                    pix_fmt_mask&= ~(1<<  x);
> > +            }
> 
> Read the full pixel layout and compare it once.

Fixed. I just read into a pre-zeroed char[16] and memcmp(), taking care
not to read more than 16 bytes.

> 
>  > [...]
>  >
> >   static int mxf_read_generic_descriptor(MXFDescriptor *descriptor, ByteIOContext *pb, int tag, int size, UID uid)
> > @@ -802,6 +842,8 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
> >               st->codec->width = descriptor->width;
> >               st->codec->height = descriptor->height;
> >               st->codec->bits_per_coded_sample = descriptor->bits_per_sample; /* Uncompressed */
> > +            st->codec->pix_fmt = descriptor->pix_fmt;
> > +            st->codec->codec_tag = avcodec_pix_fmt_to_codec_tag(st->codec->pix_fmt);
> 
> Do not set codec tag.

Fixed.

> 
> Thanks for the patch, this is appreciated.
> 
> [...]
> 

Revised patch attached. It does not pass regtests since pix_fmt differs
(not setting it makes them pass).

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mxf_pix_fmt2.patch
Type: text/x-patch
Size: 4593 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100607/5a259265/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100607/5a259265/attachment.pgp>