[FFmpeg-devel] [Patch/RFC] Improved PixelLayout parsing in mxfdec.c
Tomas Härdin
tomas.hardin
Mon Jun 7 11:36:48 CEST 2010
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>
More information about the ffmpeg-devel
mailing list