[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