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

[Baptiste Coudurier]

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.

> 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.

> 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.

>
> 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.

 > [...]
 >
> +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.

 > [...]
 >
>   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.

Thanks for the patch, this is appreciated.

[...]

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org