[FFmpeg-devel] [PATCH] IFF: Add the HAM stuff

Ronald S. Bultje rsbultje
Sun May 16 18:18:51 CEST 2010


Hi,

On Sat, May 15, 2010 at 6:40 PM, Sebastian Vater
<cdgs.basty at googlemail.com> wrote:
> +++ b/libavcodec/iff.c
[..]
> +    unsigned  masking;      ///< TODO: masking method used
> +    unsigned  transparency; ///< TODO: transparency color index in palette

I would simply remove these values until you use them. Right now I
can't really say much about them because they are basically unused.

> +#define DECODE_HAM_PLANE32(x)       \
> +    first       = buf[x] << 1;      \
> +    second      = buf[(x)+1] << 1;  \
> +    delta      &= pal[first++];     \
> +    delta      |= pal[first];       \
> +    dst[x]      = delta;            \
> +    delta      &= pal[second++];    \
> +    delta      |= pal[second];      \
> +    dst[(x)+1]  = delta;
> +
> +/**
> + * Converts one line of HAM6/8-encoded chunky buffer to 24bpp.
> + *
> + * @param dst the destination 24bpp buffer
> + * @param buf the source 8bpp chunky buffer
> + * @param pal the HAM decode table
> + * @param buf_size the plane size in bytes
> + */
> +static void decode_ham_plane32(uint32_t *dst, const uint8_t  *buf,
> +                               const uint32_t *const pal, unsigned buf_size)
> +{
> +    uint32_t delta = 0;
> +    do {
> +        uint32_t first, second;
> +        DECODE_HAM_PLANE32(0)
> +        DECODE_HAM_PLANE32(2)
> +        DECODE_HAM_PLANE32(4)
> +        DECODE_HAM_PLANE32(6)
> +        buf += 8;
> +        dst += 8;
> +    } while (--buf_size);
> +}

gcc does a surprisingly good job at optimizing this. The only comment
I have are thus cosmetic, I'd recommend putting a semicolon (";")
behind each macro call (so DECODE_HAM_PLANE32(..); < that one).

> +/**
> + * Extracts the IFF extra context and updates internal
> + * decoder structures.
> + *
> + * @param avctx the AVCodecContext where to extract extra context to
> + * @param avpkt the AVPacket to extract extra context from
> + *
> + * @return 0 in case of success, a negative error code otherwise
> + */
> +static int extract_header(AVCodecContext *const avctx,
> +                          const AVPacket *const avpkt) {
> +    IffContext *s = avctx->priv_data;
> +    const uint8_t *buf = avpkt->data;
> +    const unsigned buf_size = bytestream_get_be16(&buf);

This will already overrun if pkt->size < 2.

> +    if (buf_size <= 1 || (int) GET_EXTRA_SIZE(avpkt) <= 1) {
> +        av_log(avctx, AV_LOG_ERROR,
> +               "Invalid packet size received: %u -> video data offset: %d\n",
> +               (unsigned) buf_size, (int) GET_EXTRA_SIZE(avpkt));
> +        return AVERROR_INVALIDDATA;
> +    }

So this check should be done before reading buf_size.

> +    if (buf_size > 8) {
> +        s->compression  = bytestream_get_byte(&buf);
> +        s->bpp          = bytestream_get_byte(&buf);
> +        s->ham          = bytestream_get_byte(&buf);
> +        s->flags        = bytestream_get_byte(&buf);
> +        s->transparency = bytestream_get_be16(&buf);
> +        s->masking      = bytestream_get_byte(&buf);
> +    }

shouldn't it be if (buf_size < 8) return error?

> +        const unsigned mbits = 8 - s->ham;
[..]
> +            uint32_t tmp = i << mbits;

mbits appears unused otherwise, so can be removed, and also the value
for s->ham isn't checked before this, it can be anything which can
probably crash the decoder.

> +        s->ham_palbuf = av_malloc((8 * (1 << s->ham) * sizeof (uint32_t)) + FF_INPUT_BUFFER_PADDING_SIZE);

Same here, s->ham value is unchecked.

> +        if (count) { // HAM with color palette attached
> +            // prefill with black and palette and set HAM take direct value mask to zero
> +            memset(s->ham_palbuf, 0, (1 << s->ham) * 2 * sizeof (uint32_t));
> +            for (i=0; i < count; i++) {
> +                s->ham_palbuf[i*2+1] = AV_RL24( avctx->extradata + i*3 );
> +            }
> +            count = 1 << s->ham;

Use av_mallocz(), memset(x, 0) or something to zero the remaining
(unpaletted) entries, e.g. if extradata is too small.

> +                s->ham_palbuf[i*2]   = 0x000000; // take direct color value from palette

Just 0 is enough.

> +        for (i=0; i < count; i++) {
> +            tmp |= tmp >> s->ham;
> +            s->ham_palbuf[(i+count)*2]   = 0x00FFFF; // just modify blue color component
> +            s->ham_palbuf[(i+count*2)*2] = 0xFFFF00; // just modify red color component
> +            s->ham_palbuf[(i+count*3)*2] = 0xFF00FF; // just modify green color component
> +            s->ham_palbuf[(i+count)*2+1]   = tmp << 16;
> +            s->ham_palbuf[(i+count*2)*2+1] = tmp;
> +            s->ham_palbuf[(i+count*3)*2+1] = tmp << 8;
> +        }

This can be vertically aligned and otherwise be made prettier.

> @@ -256,31 +396,43 @@ static int decode_frame_ilbm(AVCodecContext *avctx,
>                              AVPacket *avpkt)
>  {
>      IffContext *s = avctx->priv_data;
> -    const uint8_t *buf = avpkt->data;
> -    int buf_size = avpkt->size;
> +    const uint8_t *buf = GET_EXTRA_DATA(avpkt);
> +    int buf_size = GET_EXTRA_SIZE(avpkt);
>      const uint8_t *buf_end = buf+buf_size;
> -    int y, plane;
> +    int y, plane, err;
>
>      if (avctx->reget_buffer(avctx, &s->frame) < 0){
>          av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>          return -1;
>      }
>
> +    if ((err = extract_header(avctx, avpkt)) < 0)
> +        return err;
[..]
> @@ -304,41 +463,59 @@ static int decode_frame_byterun1(AVCodecContext *avctx,
>                              AVPacket *avpkt)
>  {
>      IffContext *s = avctx->priv_data;
> -    const uint8_t *buf = avpkt->data;
> -    int buf_size = avpkt->size;
> +    const uint8_t *buf = GET_EXTRA_DATA(avpkt);
> +    const int buf_size = GET_EXTRA_SIZE(avpkt);
>      const uint8_t *buf_end = buf+buf_size;
> -    int y, plane;
> +    int y, plane, err;
>
>      if (avctx->reget_buffer(avctx, &s->frame) < 0){
>          av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>          return -1;
>      }
>
> +    if ((err = extract_header(avctx, avpkt)) < 0)
> +        return err;
[etc.]

There's literally a ton of code duplication between these two
functions, this surely can be factored out in some sane way? I mean,
up to this point we were talking about two functions of ~10-20 lines
each, but we're extending both functions with identical code in each
patch now, so this is a great point to hold on, merge and easily (if
possible). Can you look into that?

> +++ b/libavcodec/iff.h
[..]
> +// TODO: masking bits
> +#define MASK_NONE                  0
> +#define MASK_HAS_MASK              1
> +#define MASK_HAS_TRANSPARENT_COLOR 2
> +#define MASK_LASSO                 3

I've said this before, but you can simply init the MASK_NONE in
lavf/iff.c to 0, and then this can all go to lavc/iff.c. 1 and 3 are
not used at all, so this might not be the right patch for them. 2 can
probably be removed also since it's only use is in a FIXME av_log().

> +// screenmode bits
> +#define CAMG_EHB 0x80    // Extra HalfBrite
> +#define CAMG_HAM 0x800   // Hold And Modify

These are both used only once, you don't really need the defines,
simply use the actual hex values in the if and add a comment, like
this:

if (val & 0x80) // extra halfbrite
    bla_bla();

or

if (val & 0x80 /* extra halfbrite */)
    bla_bla();

+/**
+ * This number of bytes if added at the beginning of each AVPacket
+ * which contain additional information about video properties
+ * which has to be shared between demuxer and decoder.
+ * This number may change between frames.
+ */
+#define IFF_EXTRA_VIDEO_SIZE 9

The last line of this comment ("this number may change between
frames") is a little odd, because it does not, right?

Ronald



More information about the ffmpeg-devel mailing list