Hi,
On Wed, Mar 23, 2011 at 2:03 PM, Kostya <[email protected]> wrote:
> On Tue, Mar 22, 2011 at 06:54:38PM +0100, Diego Biurrun wrote:
>> On Sun, Mar 20, 2011 at 11:02:23AM +0100, Kostya wrote:
>> > [...]
>>
>> The usual nits..
>
> reordered, added whitespaces, broke some long lines (even if I don't know what
> to do with all that karma), fixed license and added missing long name
Decoder review:
> +static int which_edge(int x, int y, int edge_size)
s/int/enum GlyphEdge/
> +static int which_direction(int edge0, int edge1)
s/int/enum GlyphDir/
> +static void interp_point(int8_t *ppoint, int x0, int y0, int x1, int y1, int
> ipoint, int npoints)
> +{
> + if (npoints) {
> + ppoint[0] = (x0 * ipoint + x1 * (npoints - ipoint) + (npoints >> 1))
> / npoints;
> + ppoint[1] = (y0 * ipoint + y1 * (npoints - ipoint) + (npoints >> 1))
> / npoints;
> + } else {
I find it hard to see what values npoints typically has, but is using
FASTDIV() an option here?
> +static void init_sizes(sanm_ctx *ctx, int width, int height)
[..]
> + ctx->aligned_width = FFALIGN(width, 8);
> + ctx->aligned_height = FFALIGN(height, 8);
AVCodecContext->coded_width/height?
+static int old_codec1(sanm_ctx *ctx, const uint8_t *src, int size,
+ int top, int left, int width, int height)
That's a pretty lame function name. Same for the others.
+{
+ uint8_t *dst = ((uint8_t*)ctx->pdb0) + left + top * ctx->pitch;
Spaces (hah!).
v+static int old_codec37(sanm_ctx *ctx, const uint8_t *src, int size,
> + int top, int left, int width, int height)
[..]
> + av_log(ctx->avctx, AV_LOG_ERROR, "Codec 37 compr 1: NIY\n");
IKWTMBMPW.
> + for (j = 0; j < height; j++) {
> + memcpy(dst, src, width);
> + dst += stride;
> + src += width;
Won't this overread?
(I didn't look at this function in detail, please make sure it doesn't
overread elsewhere, me tired.)
> +static int old_codec47(sanm_ctx *ctx, const uint8_t *orig_src, int size,
> + int top, int left, int width, int height)
[..]
+ const uint8_t *src = orig_src + 26, *src_end = orig_src + size;
What is this 26?
> + if (orig_src[4] & 1)
> + src += 0x8080;
?? You're just skipping 0x8080 bytes? (Weird.)
> + switch (compr) {
> + case 0:
Some comments would help the unsuspecting reader here. E.g. case 0: //
uncompressed block. Same for the others.
> + case 5:
> + t = AV_RL32(orig_src + 14);
> + buf = av_malloc(t+16);
> + if (!buf)
> + return -1;
Some checks to make sure "t" is valid would make sense, this could be
a huge malloc bomb in a nicely crafted stream.
> +static int decode_0(sanm_ctx *ctx, const uint8_t *buf, const uint8_t
> *buf_end)
How about you name these functions more interestingly? decode_<num>
isn't very descriptive. They do distinctly different things, exposing
such information in a function name is a common way to make code
maintainable and readable.
> + if (ctx->width * ctx->height * 2 > buf_end - buf) {
> + av_log(ctx->avctx, AV_LOG_ERROR, "Insufficient data for raw
> frame\n");
> + return -1;
> + }
> + for (y = 0; y < ctx->height; y++) {
> + for (x = 0; x < ctx->width; x++)
> + pdb0[x] = bytestream_get_le16(&buf);
> + pdb0 += ctx->pitch;
> + }
> + return 0;
> +}
At the very least you can
#if LITTLE_ENDIAN
memcpy(pdb0, buf, ctx->width*2); buf += ctx->width*2; pdb0 += ctx->pitch;
#else
..
> +static void copy_block(uint16_t *pdest, uint16_t *psrc, int block_size, int
> pitch)
> +{
> + int y;
> +
> + for (y = 0; y != block_size; y++, pdest += pitch, psrc += pitch)
> + memcpy(pdest, psrc, block_size * sizeof(pdest[0]));
> +}
Isn't this dspcontext.put_pixels_16/8() for block_size=8/4? They would
be faster.
> +static void fill_block(uint16_t *pdest, uint16_t color, int block_size, int
> pitch)
> +{
> + int x, y;
> +
> + pitch -= block_size;
> + for (y = 0; y != block_size; y++, pdest += pitch)
> + for (x = 0; x != block_size; x++)
> + *pdest++ = color;
> +}
I'm actually susprised, but I'd expect one of the h264 prediction
functions to be useful here. No such thing... Ignore this. :-).
> +static int opcode_0xf7(sanm_ctx *ctx, int cx, int cy, int block_size, int
> pitch)
[..]
This function name can also be better... This is not assembly, you know.
> + indices = bytestream_get_le32(&ctx->src);
> + dst[0] = ctx->codebook[indices & 0xFF]; indices >>= 8;
> + dst[1] = ctx->codebook[indices & 0xFF]; indices >>= 8;
> + dst[pitch] = ctx->codebook[indices & 0xFF]; indices >>= 8;
> + dst[pitch + 1] = ctx->codebook[indices & 0xFF];
How is this different from:
dst[0] = ctx->codebook[*ctx->src++];
dst[1] = ctx->codebook[*ctx->src++];
dst[pitch] = ctx->codebook[*ctx->src++];
dst[pitch + 1] = ctx->codebook[*ctx->src++];
? That looks more readable to me.
> +static int good_mvec(sanm_ctx *ctx, int cx, int cy, int mx, int my, int
> block_size)
> +{
> + int start_pos = cx + mx + (cy + my) * ctx->pitch;
> + int end_pos = start_pos + (block_size - 1) * (ctx->pitch + 1);
> +
> + int good = start_pos >= 0 && end_pos < (ctx->buf_size >> 1);
> +
> + if (!good) {
> + av_log(ctx->avctx, AV_LOG_ERROR, "ignoring invalid motion vector
> (%i, %i)->(%u, %u), block size = %u.\n",
> + cx + mx, cy + my, cx, cy, block_size);
> + }
> +
> + return good;
> +}
An alternative is to clip it if it's not good. Then at least the frame
data is not uninitialized, although I agree that there's still a good
chance that you get garbage out of it...
> +static int decode_5(sanm_ctx *ctx, const uint8_t *buf, const uint8_t
> *buf_end)
[..]
> + if (HAVE_BIGENDIAN) {
> + while (npixels--) {
> + *pdb0 = av_bswap16(*pdb0);
> + pdb0++;
> + }
> + }
Use the dsputil function Mans just created for AC3.
> +static int decode_frame(AVCodecContext *avctx, void *data, int *data_size,
> AVPacket *pkt)
[..]
> + if (!ctx->version) {
Any reason these aren't two decoders? I don't see them sharing a lot
of common code, and if they do share some, it can be split over 3
files. Makes review also much simpler. :-).
> + while (src < src_end - 8) {
> + int sig, size;
uint32_t.
> + if (size < 0 || src_end - src < size) {
> + av_log(avctx, AV_LOG_ERROR, "Incorrect chunk size %d\n",
> size);
> + break;
> + }
And then size < 0 condition is not needed.
> + case MKBETAG('X', 'P', 'A', 'L'):
[..]
> + if (size == 6 || size == 4) {
[..]
> + } else {
> + const uint8_t *psrc = src + 4;
Any idea what these 4 btyes are that you're skipping here?
> + case MKBETAG('S', 'T', 'O', 'R'):
> + to_store = 1;
> + break;
> + case MKBETAG('F', 'T', 'C', 'H'):
> + memcpy(ctx->pdb0, ctx->stored_frame, ctx->buf_size);
> + break;
[..]
> + if (to_store)
> + memcpy(ctx->stored_frame, ctx->pdb0, ctx->buf_size);
> + copy_output(ctx, NULL);
> + memcpy(ctx->output->data[1], ctx->pal, 1024);
So this is very basic handling of P-frames and reference frames? Is
there some way to integrate src_pitch in the writing of pdb0 so you
can use reget_buffer and frame referencing (in AVFrame) instead of the
manual memcpy()s?
(Don't spend too much time on it, it's a game codec, I'm just
wondering how complex it is.)
Also, please set AVFrame.pict_type, that shouldn't be too hard.
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
