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

[u-9iep at aetey.se]

Hello Ronald,

On Thu, Feb 02, 2017 at 11:16:35AM -0500, Ronald S. Bultje wrote:
> On Thu, Feb 2, 2017 at 10:59 AM, <u-9iep at aetey.se> wrote:
> > 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.

(Thanks for the pointer. I'll look at how it is done in h264, but: )

I thought about generating the bodies of the functions from something
like a template but it did not feel like this would make the code more
understandable aka maintainable. So I wonder if there is any point in
doing this, given the same binary result.

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

It moves the conversion from after the decoder, where the data to convert
is all and whole frames, to the inside where the conversion applies
to the codebooks, by the codec design much less than the output.

> Why not just always output the native format? (And then do conversion in

The "native" Cinepak format is actually unknown to swscaler, and I
seriously doubt it would make sense to add it there, which would
just largely cut down the decoding efficiency.

> GPU if really needed, that is always near-free.)

You assume one have got a GPU.

The intention is to allow usable playback on as simple/slow devices
as possible.

> > > +    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.

I did my best to look for a better way but it does not seem to be existing.

> 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

Unfortunately this is _not_ a solution.

I was of course aware of get_format(). Its functionality seems to
be intended for use by the applications: "decoding: Set by user".
Correct me if I am wrong (even better correct the documentation which
apparently is confusing?).

In any case, the codec has no means to know which output format is
"best" in a particular case so all it can do is to list the formats it
offers. This is done and using get_format() should work as expected,
which unfortunately does not solve anything:

There is of course hardly any application which tries to tune the codecs
to the output. (Mplayer seems to try but this does not seem to help,
possibly because its codec handling is generalized beyond ffmpeg codec
interfaces?)
Codecs efficiently supporting multiple formats are definitely a tiny
minority, so no surprize applications are not prepared to deal with this.

Applications also do lack any reliable knowledge of which format from the
codec is or is not efficient. It is only the deployment context which
can tell what is best for the particular case, which is known neither
to the codec nor to a general-purpose application.

To resum, I do not expect that there is any better solution than using
an out-of-band channel like a dedicated environment variable,
but would be of course happy to see one.

Regards,
Rune