On Thu, Feb 02, 2017 at 04:25:05PM +0100, wm4 wrote:
> I can see how conversion in the decoder could speed it up somewhat
> (especially if limited by memory bandwidth, I guess), but it's not
> really how we do things in FFmpeg. Normally, conversion is done by
> libswscale (or whatever the API user uses).
I would not call "twice" for "somewhat" :)
The point is to do something that libswscale hardly can help with.
> > The format can be also chosen by setting environment variable
> > CINEPAK_OUTPUT_FORMAT_OVERRIDE, if this is enabled at build time.
>
> It's not an environment variable, but a preprocessor variable. It's
Both are (intentionally) spelled the same which misled you to judge so.
> also not defined by default, which effectively makes some of your
> additions dead code. This is also not a thing we do in FFmpeg, and it
I am always enabling this feature anyway.
Was unsure whether activate it by default would be acceptable.
> usually creates maintenance nightmares. (For you too. Who would care
> about accidentally breaking this code when making changes to live parts
> of the decoder?)
Shall I repost the patch with this part on by default?
> > -typedef struct CinepakContext {
> > +typedef struct CinepakContext CinepakContext;
> > +struct CinepakContext {
> > + const AVClass *class;
> >
> > AVCodecContext *avctx;
> > AVFrame *frame;
> > @@ -71,24 +87,111 @@ typedef struct CinepakContext {
> > int sega_film_skip_bytes;
> >
> > uint32_t pal[256];
> > -} CinepakContext;
>
> Stray change?
No.
The compiler complained about referencing the type
being typedef'ed when I introduced function pointers into this
structure, the functions take pointers to it as args:
> > + void (*decode_codebook)(CinepakContext *s,
> > + cvid_codebook *codebook, int chunk_id,
> > + int size, const uint8_t *data);
> > + int (*decode_vectors)(CinepakContext *s, cvid_strip *strip,
> > + int chunk_id, int size, const uint8_t *data);
> > +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.
> 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?
> > + char *out_fmt_override = getenv("CINEPAK_OUTPUT_FORMAT_OVERRIDE");
> Absolutely not acceptable.
1. Why?
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.
Any suggestion which can replace this approach?
> AFAIK we don't usually do INFO messages in decoders or anywhere outside
> of CLI tools. I'm probably wrong, but it should be avoided anyway.
This is very useful when you wish to check that you got it right
for a particular visual buffer / device, given that an apllication can
try to make its own (and possibly bad) choices. Not critical, I admit.
Regards,
Rune
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
