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

Sebastian Vater cdgs.basty
Sun May 16 21:28:22 CEST 2010


Ronald S. Bultje a ?crit :
> 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.
>   

The reason I put them already there is that I have to read them in a
different order than I'm evaluating them (doing otherwise will make if
statements more complicated).

This is because I use them for writing an not supported error (just look
the if statements and you'll see that I access the data in a different
order).

It will require much more code to be changed later, if I add them to
local (hey, that's just 8 bytes) storage later.

Hope it's OK therefore, to keep them right now (please also note that
masking support will be added immediately after HAM patch is in).

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

Fixed.

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

Fixed.

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

Fixed.

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

No, the demuxer can send buf_size == 2 packets to indicate that nothing
has changed.

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

Fixed.

>> +        s->ham_palbuf = av_malloc((8 * (1 << s->ham) * sizeof (uint32_t)) + FF_INPUT_BUFFER_PADDING_SIZE);
>>     
>
> Same here, s->ham value is unchecked.
>   

Fixed.

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

There's already a memset just doing exactly what you mean ;)

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

Fixed.

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

Fixed.

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

Will fix in a separate patch when HAM is in, ok?

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

That was a recommendation of Stefano's patch review. So I'll keep that
for now.

>   
>> +// 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();
>   

Fixed.

> +/**
> + * 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?
>   

Fixed.

New patch attached, please review it. Please note that it requires
latest iff-function-merge.patch which also uses error checking of
byterun1 streams.

-- 

Best regards,
                   :-) Basty/CDGS (-:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: iff-ham-support.patch
Type: text/x-patch
Size: 23758 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100516/985c10b8/attachment.bin>



More information about the ffmpeg-devel mailing list