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

[Ronald S. Bultje]

Hi,

On Sun, May 16, 2010 at 11:19 PM, Sebastian Vater
<cdgs.basty at googlemail.com> wrote:
> +++ b/libavcodec/iff.c
[..]
> +// TODO: masking bits
> +#define MASK_NONE                  0
> +#define MASK_HAS_MASK              1
> +#define MASK_HAS_TRANSPARENT_COLOR 2
> +#define MASK_LASSO                 3

There aren't used, only in error handling. As said before, I'd
recommend adding this when you're adding the functionality. Right now,
an if (masking) { av_log_missing_feature(); return error; } is all you
need.

Separately, this should be an enum, and might require some
documentation, e.g. what is a lasso? (Note how this totally gets into
the actual technical details of how you'd implement this patch, which
is why I'd add this only when you add that, not before.)

> +    unsigned  bpp;          ///< bits per plane to decode

I'd add some documentation indicating that this is NOT always the same
as avctx->bits_per_coded_sample, because I was confused about that,
and then indicating how the two differ.

> +#define GET_EXTRA_DATA(x) ((uint8_t *) (x)->extradata + AV_RB16((x)->extradata))
[..]
> +#define GET_EXTRA_SIZE(x) ((x)->extradata_size - AV_RB16((x)->extradata))
[..]
> +#define GET_PACKET_DATA(x) ((uint8_t *) (x)->data + AV_RB16((x)->data))
[..]
> +#define GET_PACKET_SIZE(x) ((x)->size - AV_RB16((x)->data))

I'm OK with this, but would like to mention that this borderlines
overengineering ..

If you intend to keep them, please rename the EXTRA_DATA/SIZE to
PALETTE_DATA/SIZE (it's not "extra" data, it's a palette).

>      if (count) {
>          for (i=0; i < count; i++) {
> -            pal[i] = 0xFF000000 | AV_RB24( avctx->extradata + i*3 );
> +            pal[i] = 0xFF000000 | AV_RB24(GET_EXTRA_DATA(avctx) + i*3);
>          }
>      } else { // Create gray-scale color palette for bps < 8

.. and this is why. You're calculating the actual palette location in
every run in the loop, which isn't optimal. You could add a uint8_t
*ptr = GET_EXTRA_DATA(avctx) at the beginning of the loop, so you
don't recalculate it inside the loop.

Now, I want to reiterate, again, that this sort of poor design is
common when things are overengineered. This is why I'm personally not
in favor of overengineering, it leads to poorer performance and you
generally don't even notice because you're buried in macros, wrappers
and conveniences to work around their general and implicit
awkwardness.

if (extradata_size < 2) return error;
uint8_t *palette_data = avctx->extradata      + AV_RB16(avctx->extradata);
int palette_size      = avctx->extradata_size - AV_RB16(avctx->extradata);

That isn't so bad, is it? It's also more descriptive.

[..]

If you insist, you can keep the macros, but at least move the
GET_EXTRA_DATA() call outside the loop.

> +        if (buf_size <= 1 || (int) GET_PACKET_SIZE(avpkt) <= 1) {

The cast is unneeded, extradata_size is signed by itself. (Note again
how the wrapper macro hides that fact, adding to the total awkwardness
of the whole?)

> +            av_log(avctx, AV_LOG_ERROR,
> +                   "Invalid packet size received: %u -> video data offset: %d\n",
> +                   (unsigned) buf_size, (int) GET_PACKET_SIZE(avpkt));

Both casts are unneeded, buf_size is already unsigned and
extradata_size is already signed, and their signedness is already
indicated by the printf() string.

> +        buf = avctx->extradata;
> +        buf_size = bytestream_get_be16(&buf);
> +        if (buf_size <= 1 || (int) GET_EXTRA_SIZE(avctx) < 0) {
> +            av_log(avctx, AV_LOG_ERROR,
> +                   "Invalid extradata size received: %u -> palette data offset: %d\n",
> +                   (unsigned) buf_size, (int) GET_EXTRA_SIZE(avctx));
> +            return AVERROR_INVALIDDATA;

Duplicate. It's probably easier if you call this function with
avpkt->data and avpkt->size or extradata and size, the caller knows
which of the two is intended and then this duplicate code can be
merged into one.

> +    if (buf_size > 8) {

If you use if (buf_size < 0) return 0;, you gain 4 spaces along the
whole rest of the function.

> +            int i;
> +            int count = FFMIN(GET_EXTRA_SIZE(avctx) / 3, 1 << s->ham);

You can merge these two lines.

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

Vertical align.

> +    const uint8_t *buf = avpkt->size >= 2 ? GET_PACKET_DATA(avpkt) : NULL;
> +    const int buf_size = avpkt->size >= 2 ? GET_PACKET_SIZE(avpkt) : 0;

Both a >= b ? c : d are unneeded, since you're checking each touch of
buf against buf_end, so if it's out of bounds (regardless of whether
buf == buf_end of buf > buf_end), you'll never read *buf, and buf_size
is unused otherwise.

> +    const uint8_t *buf = avpkt->size >= 2 ? GET_PACKET_DATA(avpkt) : NULL;
> +    const int buf_size = avpkt->size >= 2 ? GET_PACKET_SIZE(avpkt) : 0;

Same.

Rest of patch looks good to me, keep it up!

Ronald