[FFmpeg-devel] [PATCH] Efficiently support several output pixel formats in Cinepak decoder

Ronald S. Bultje rsbultje at gmail.com
Thu Feb 2 18:16:35 EET 2017

Hi Rune,

On Thu, Feb 2, 2017 at 10:59 AM, <u-9iep at aetey.se> wrote:

> On Thu, Feb 02, 2017 at 04:25:05PM +0100, wm4 wrote:
> > > +static void cinepak_decode_codebook_rgb32 (CinepakContext *s,
> > > +    cvid_codebook *codebook, int chunk_id, int size, const uint8_t
> *data)
> >
> > > +static void cinepak_decode_codebook_rgb24 (CinepakContext *s,
> > > +    cvid_codebook *codebook, int chunk_id, int size, const uint8_t
> *data)
> >
> > > +static void cinepak_decode_codebook_rgb565 (CinepakContext *s,
> > > +    cvid_codebook *codebook, int chunk_id, int size, const uint8_t
> *data)
> > So a part of the decoder is essentially duplicated for each format?
> It is the irregular differences between them which are the reason
> for splitting. I would not call this "duplication". If you feel
> it is straightforward and important to make this more compact,
> with the same performance, just go ahead.

So, typically, we wouldn't duplicate the code, we'd template it. There's
some examples in h264 how to do it. You'd have a single (av_always_inline)
decode_codebook function, which takes "format" as an argument, and then
have three av_noinline callers to it (using fmt=rgb565, fmt=rgb24 or

That way performance works as you want it, without the source code

> What we _especially_ don't do in FFmpeg is hardcoding colorspace
> > conversion coefficients in decoders, and doing colorspace conversion in
> > decoders.
> Have you got a suggestion how to do avoid this in this case,
> without sacrificing the speed?

Ah, yes, the question. So, the code change is quite big and it does various
things, and each of these might have a better alternative or be good as-is.
fundamentally, I don't really understand how _adding_ a colorspace
conversion does any good to speed. It fundamentally always makes things
slower. So can you explain why you need to _add_ a colorspace conversion?
Why not just always output the native format? (And then do conversion in
GPU if really needed, that is always near-free.)

> > +    char *out_fmt_override = getenv("CINEPAK_OUTPUT_FORMAT_OVERRIDE");
> > Absolutely not acceptable.
> 1. Why?

Libavcodec is a library. Being sensitive to environment in a library, or
worse yet, affecting the environment, is typically not what is expected.
There are almost always better ways to do the same thing.

For example, in this case:

> 2. I am open to a better solution of how to choose a format at run time
> when the application lacks the knowledge for choosing the most suitable
> format or does not even try to.

wm4 suggested earlier to implement a get_format() function. He meant this:



More information about the ffmpeg-devel mailing list