On Wed, 1 Feb 2017 20:37:02 +0100
u-9...@aetey.se wrote:
> From 300b8a4e712d1a404983b245aac501e09326ee72 Mon Sep 17 00:00:00 2001
> From: Rl <addr-see-the-webs...@aetey.se>
> Date: Wed, 1 Feb 2017 19:44:40 +0100
> Subject: [PATCH] Efficiently support several output pixel formats in Cinepak
> decoder
>
> Optimize decoding in general and largely improve speed,
> among others by the ability to produce device-native pixel formats.
>
> The output format can be chosen at runtime by an option.
>
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).
> 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
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
usually creates maintenance nightmares. (For you too. Who would care
about accidentally breaking this code when making changes to live parts
of the decoder?)
> [...]
> -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?
>
> -static void cinepak_decode_codebook (cvid_codebook *codebook,
> - int chunk_id, int size, const uint8_t
> *data)
> + 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);
> +/* options */
> + enum AVPixelFormat out_pixfmt;
> +};
> +
> +#define OFFSET(x) offsetof(CinepakContext, x)
> +#define VD AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_DECODING_PARAM
> +static const AVOption options[] = {
> +{"output_pixel_format", "set output pixel format:
> rgb24/rgb32/rgb565/yuv420p/pal8; yuv420p is approximate", OFFSET(out_pixfmt),
> AV_OPT_TYPE_PIXEL_FMT, {.i64=AV_PIX_FMT_RGB24}, -1, INT_MAX, VD },
> + { NULL },
> +};
This is also not how we do things in FFmpeg. Usually get_format is used
when a decoder really can output multiple formats (which is very
rarely, usually for hardware decoding).
> +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?
> +/* a simplistic version to begin with, it is also fast -- rl */
> +static void cinepak_decode_codebook_yuv420 (CinepakContext *s,
> + cvid_codebook *codebook, int chunk_id, int size, const uint8_t *data)
> +{
> + const uint8_t *eod = (data + size);
> + uint32_t flag, mask;
> + int i, n;
> + uint8_t *p;
> + int palette_video = s->palette_video;
> + int selective_update = (chunk_id & 0x01);
> +
> + /* check if this chunk contains 4- or 6-element vectors */
> + n = (chunk_id & 0x04) ? 4 : 6;
> + flag = 0;
> + mask = 0;
> +
> + p = codebook->yuv420[0];
> + for (i=0; i < 256; i++) {
> + if (selective_update && !(mask >>= 1)) {
> + if ((data + 4) > eod)
> + break;
> +
> + flag = AV_RB32 (data);
> + data += 4;
> + mask = 0x80000000;
> + }
> +
> + if (!selective_update || (flag & mask)) {
> + int k;
> +
> + if ((data + n) > eod)
> + break;
> +
> + if (n == 4)
> + if (palette_video) {
> +/* here we have kind of "more" data than the output format can express */
> + int r, g, b, u = 0, v = 0;
> + for (k = 0; k < 4; ++k) {
> + uint32_t rr = s->pal[*data++];
> + r = (rr>>16)&0xff;
> + g = (rr>>8) &0xff;
> + b = rr &0xff;
> +/* calculate the components (https://en.wikipedia.org/wiki/YUV) */
> + *p++ = ((r*66+g*129+b*25+128)>>8)+16;
> + u += (-r*38-g*74+b*112+128)>>8;
> + v += (r*112-g*94-b*18+128)>>8;
> + }
> + *p++ = (u+2)/4+128;
> + *p++ = (v+2)/4+128;
> + } else { /* grayscale, easy */
> + for (k = 0; k < 4; ++k) {
> + *p++ = *data++;
> + }
> + *p++ = 128;
> + *p++ = 128;
> + }
> + else { /* n == 6 */
> +/* here we'd have to handle double format conversion
> + * Cinepak=>rgb24 and then rgb24=>yuv420p, which can not be shortcut;
> + * for the moment just copying as-is, for simplicity and speed,
> + * color will be slightly off but not much */
> + *p++ = *data++;
> + *p++ = *data++;
> + *p++ = *data++;
> + *p++ = *data++;
> + *p++ = *data++ + 128;
> + *p++ = *data++ + 128;
> + }
> + } else {
> + p += 6;
> + }
> + }
> +}
What we _especially_ don't do in FFmpeg is hardcoding colorspace
conversion coefficients in decoders, and doing colorspace conversion in
decoders.
(OK, we do for subtitle formats - which is actually a bad thing, but
historical mistakes.)
> +#ifdef CINEPAK_OUTPUT_FORMAT_OVERRIDE
> + char *out_fmt_override = getenv("CINEPAK_OUTPUT_FORMAT_OVERRIDE");
> +#endif
Absolutely not acceptable.
> + switch (avctx->pix_fmt) {
> + case AV_PIX_FMT_RGB32:
> + av_log(avctx, AV_LOG_INFO, "Codec output pixel format is rgb32\n");
> + s->decode_codebook = cinepak_decode_codebook_rgb32;
> + s->decode_vectors = cinepak_decode_vectors_rgb32;
> + break;
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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
