[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