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

[Michael Niedermayer]

On Mon, Jun 07, 2010 at 07:05:07PM +0200, Tomas H?rdin wrote:
> 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

>  avcodec.h |    5 ++++-
>  rawdec.c  |    2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> d44a164fed7c1a5cd69f362d521ef66aad6f42b2  rawdec_pix_fmt2.patch
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index a6f6b04..d0aa162 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1120,8 +1120,11 @@ typedef struct AVCodecContext {
>  
>      /**
>       * Pixel format, see PIX_FMT_xxx.
> +     * May be set by the demuxer, especially when demuxing rawvideo.
> +     * Some demuxers use bits_per_coded_sample or codec_tag for this purpose instead.

> +     * Some codecs may override this value. The rawvideo decoder will not.

i really would like to avoid adding special cases


>       * - encoding: Set by user.
> -     * - decoding: Set by libavcodec.
> +     * - decoding: Set by user and/or libavcodec.

i would say
Set by user if known, overridden by libavcodec if known

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100607/8d194341/attachment.pgp>