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

[Ronald S. Bultje]

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
fmt=rgb32).

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

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

https://www.ffmpeg.org/doxygen/trunk/structAVCodecContext.html#ae85c5a0e81e9f97c063881148edc28b7

Ronald