[FFmpeg-devel] [PATCH] Do not override pix_fmt in rawdec.c

[Tomas Härdin]

On Mon, 2010-06-07 at 17:04 +0200, Michael Niedermayer wrote:
> On Mon, Jun 07, 2010 at 02:30:34PM +0200, Tomas H?rdin wrote:
> > On Mon, 2010-06-07 at 12:54 +0200, Michael Niedermayer wrote:
> > > On Mon, Jun 07, 2010 at 11:01:23AM +0200, Tomas H?rdin wrote:
> > > > Hi
> > > > 
> > > > The attached patch adds a check for pix_fmt == PIX_FMT_NONE before
> > > > attempting to guess it near rawdec.c:75. This enables demuxers to set it
> > > > directly if known in advance (for example mxfdec.c).
> > > > 
> > > > Patch passes regtests.
> > > 
> > > violates api
> > >     /**
> > >      * Pixel format, see PIX_FMT_xxx.
> > >      * - encoding: Set by user.
> > >      * - decoding: Set by libavcodec.
> > >                 ^^^^^^^^^^^^^^^^^^^^^
> > > this doesnt say set by "user" aka libavformat/app
> > >      */
> > >     enum PixelFormat pix_fmt;
> > 
> > What would be a reasonable way to deal with it? Have a user_pix_fmt or
> > something like that?
> 
> chnage the documentation and verify that all code using pix_fmt can handle it

Fair enough.

Looking through the code it appears that a lot of demuxers already use
this field for this purpose. Searching libavformat for "pix_fmt= ",
"pix_fmt = " or "CODEC_ID_RAWVIDEO" turns up a lot of examples. All of
them seem to be demuxers outputing rawvideo, like yuv4mpeg.c or
filmstripdec.c.

I was a bit confused why they worked without this patch (I tested
filmstrip). The reason has to do with av_find_stream_info() near
utils.c:2309. What happens is this: The demuxer sets codec_id =
CODEC_ID_RAWVIDEO, codec_tag = 0, bits_per_coded_sample = 0 and pix_fmt
to whatever. This causes av_find_stream_info() to convert the pixel
format to a codec tag. Finally, depending on what codec_tag and
bits_per_coded_sample are set to the final pix_fmt is overriden by a
table lookup in rawdec.c.

This seems needlessly complicated to me. In order to conform to this
method bits_per_coded_sample would have to be left alone in mxfdec.c if
a correct pixel format was detected. That would cause it to be carried
over to the decoder via codec_tag. Wouldn't make much sense to someone
reading the code, but it'd work. Still, this seems like an opportunity
for refactoring, or at least documenting.

Attached patch changes the documentation for pix_fmt a bit. The patch
probably needs more work based on the above.

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rawdec_pix_fmt2.patch
Type: text/x-patch
Size: 1524 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100607/164ce257/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/164ce257/attachment.pgp>