On Sun, Oct 30, 2011 at 07:05:22PM +0100, Kostya Shishkov wrote: > Here's a new hit from Elvis Presley - You're The Devil in Disguise. > Nothing to do with Apple this time though.
Nah, Elvis is French nowadays and the author of this code is German or Russian depending on how you want to see it.. > /** > * @file > * This is a decoder for Intel Indeo Video v3. > * It's based on vector quantization, run-length coding and motion > compensation. nit: it is > * Known container formats: .avi and .mov > * Known FOURCCs: 'IV31', 'IV32' > * For documentation see: > * http://wiki.multimedia.cx/index.php?title=Indeo_3 @see > #define BS_8BIT_PEL (1<<1) ///< 8bit pixel bitdepth indicator > #define BS_KEYFRAME (1<<2) ///< intra frame indicator > #define BS_MV_Y_HALF (1<<4) ///< vertical mv halfpel resolution indicator > #define BS_MV_X_HALF (1<<5) ///< horizontal mv halfpel resolution indicator > #define BS_NONREF (1<<8) ///< nonref (discardable) frame indicator > #define BS_BUFFER 9 ///< indicates which of two frame buffers > should be used spaces around << would be nice > typedef struct Plane { > } Plane; > > typedef struct Cell { > } Cell; I find the capitalized names ugly. I'd also not even typedef the name, but that's likely just me. > if (luma_width < 16 || luma_width > 640 || > luma_height < 16 || luma_height > 480 || > luma_width & 3 || luma_height & 3) { > av_log(avctx, AV_LOG_ERROR, "Invalid picture dimensions: %d x %d!\n", > luma_width, luma_height); > return -1; AVERROR_INVALIDDATE I think. > /* allocate frame buffers */ > for (p = 0; p < 3; p++) { > ctx->planes[p].pitch = (!p ? luma_pitch : chroma_pitch); > ctx->planes[p].width = (!p ? luma_width : chroma_width); > ctx->planes[p].height = (!p ? luma_height : chroma_height); pointless () > /** > * Dispose frame buffers. > */ > static av_cold void free_frame_buffers(Indeo3DecodeContext *ctx) The comment is not terribly informative. > /** > * Copy pixels of the cell(x + mv_x, y + mv_y) from the previous frame into > * the cell(x, y) in the current frame. > * > * @param ctx [in] pointer to the decoder context > * @param plane [in] pointer to the plane descriptor > * @param cell [in] pointer to the cell descriptor > */ > static void copy_cell(Indeo3DecodeContext *ctx, Plane *plane, Cell *cell) Did you try building the Doxygen? I think it should be @param[in], what you have here is invalid syntax. > /** > * Fill n lines with 64bit pixel value pix > */ > static inline void fill_64(uint8_t *dst, const uint64_t pix, int32_t n, > int32_t row_offset) nit: break this long line > #define APPLY_DELTA_4 \ > *((uint16_t *)(dst+line_offset )) = *((uint16_t *)(ref )) + > delta_tab->deltas[dyad1];\ > *((uint16_t *)(dst+line_offset+2)) = *((uint16_t *)(ref+2)) + > delta_tab->deltas[dyad2];\ > if (mode >= 3) {\ > if (is_top_of_cell && !cell->ypos) {\ > *((uint32_t *)(dst)) = *((uint32_t *)(dst+row_offset));\ > } else \ > AVG_32(dst, ref, dst + row_offset);\ > } > > #define APPLY_DELTA_8 \ > /* apply two 32-bit VQ deltas to next even line */\ > if (is_top_of_cell) { \ > *((uint32_t *)(dst+row_offset )) = \ > replicate32(*((uint32_t *)(ref ))) + delta_tab->deltas_m10[dyad1];\ > *((uint32_t *)(dst+row_offset+4)) = \ > replicate32(*((uint32_t *)(ref+4))) + > delta_tab->deltas_m10[dyad2];\ > } else { \ > *((uint32_t *)(dst+row_offset )) = \ > *((uint32_t *)(ref )) + delta_tab->deltas_m10[dyad1];\ > *((uint32_t *)(dst+row_offset+4)) = \ > *((uint32_t *)(ref+4)) + delta_tab->deltas_m10[dyad2];\ > } \ > /* odd lines are not coded but rather interpolated/replicated */\ > /* first line of the cell on the top of image? - replicate */\ > /* otherwise - interpolate */\ > if (is_top_of_cell && !cell->ypos) {\ > *((uint64_t *)(dst)) = *((uint64_t *)(dst+row_offset));\ > } else \ > AVG_64(dst, ref, dst + row_offset); > > > #define APPLY_DELTA_1011_INTER \ > if (mode == 10) { \ > *((uint32_t *)(dst )) += delta_tab->deltas_m10[dyad1];\ > *((uint32_t *)(dst+4 )) += delta_tab->deltas_m10[dyad2];\ > *((uint32_t *)(dst+row_offset )) += delta_tab->deltas_m10[dyad1];\ > *((uint32_t *)(dst+row_offset+4)) += delta_tab->deltas_m10[dyad2];\ > } else { \ > *((uint16_t *)(dst )) += delta_tab->deltas[dyad1];\ > *((uint16_t *)(dst+2 )) += delta_tab->deltas[dyad2];\ > *((uint16_t *)(dst+row_offset )) += delta_tab->deltas[dyad1];\ > *((uint16_t *)(dst+row_offset+2)) += delta_tab->deltas[dyad2];\ spaces around '+' would be nice > /** > * Decode a vector-quantized cell. > * It consists of several routines, each of which handles one or more "modes" > * with which a cell can be encoded. > * > * @param ctx [in] pointer to the decoder context > * @param avctx [in] ptr to the AVCodecContext > * @param plane [in] pointer to the plane descriptor > * @param cell [in] pointer to the cell descriptor > * @param data_ptr [in] pointer to the compressed data > * @param last_ptr [in] pointer to the last byte to catch reads past end of > buffer > * @return number of cunsumed bytes or -1 if error > */ see above > static int decode_cell(Indeo3DecodeContext *ctx, AVCodecContext *avctx, Plane > *plane, > Cell *cell, const uint8_t *data_ptr, const uint8_t > *last_ptr) nit: long line > if (!cell->mv_ptr) { > /* use previous line as reference for INTRA cells */ > ref_block = block - plane->pitch; > } else { > if (mode >= 10) { > /* for mode 10 and 11 INTER first copy the predicted cell into > the current one */ > /* so we don't need to do data copying for each RLE code later */ > copy_cell(ctx, plane, cell); > } else { > /* set the pointer to the reference pixels for modes 0-4 INTER */ > mv_y = cell->mv_ptr[0]; > mv_x = cell->mv_ptr[1]; > offset += mv_y * plane->pitch + mv_x; > ref_block = plane->pixels[ctx->buf_sel ^ 1] + offset; > } > } I think this would be more readable with the first and second if merged. > return (data_ptr - data_start); /* report number of bytes consumed from > the input buffer */ pointless () > /** > * Parse plane's binary tree recursively. > */ > static int parse_bintree(Indeo3DecodeContext *ctx, AVCodecContext *avctx, > Plane *plane, > int code, Cell *ref_cell, const int depth, const int > strip_width) nit: long lines > if (depth <= 0) { > av_log(avctx, AV_LOG_ERROR, "Stack overflow (corrupted binary > tree)!\n"); > return -1; // unwind recursion I'm sure there's a better return value. > if (code >= 2) { > av_log(avctx, AV_LOG_ERROR, "Invalid VQ_NULL code: %d\n", > code); > return -1; same > if (code == 1) > av_log(avctx, AV_LOG_ERROR, "SkipCell procedure not > implemented yet!\n"); missing feature or so? > case INTER_DATA: > if (!curr_cell.tree) { /* MC tree INTER code */ > /* get motion vector index and setup the pointer to the mv > set */ > if (!ctx->need_resync) > ctx->next_cell_data = > &ctx->gb.buffer[(get_bits_count(&ctx->gb) + 7) >> 3]; > curr_cell.mv_ptr = &ctx->mc_vectors[*(ctx->next_cell_data++) > << 1]; > curr_cell.tree = 1; /* enter the VQ tree */ nit: align '=' > /** > * Decode a plane (Y, V or U) > */ > static int decode_plane(Indeo3DecodeContext *ctx, AVCodecContext *avctx, > Plane *plane, > const uint8_t *data, int32_t data_size, int32_t > strip_width) nit: long line uninformative comment > /** > * Parse indeo3 frame headers > */ > static int decode_frame_headers(Indeo3DecodeContext *ctx, AVCodecContext > *avctx, > const uint8_t *buf, int buf_size) same > if (bytestream_get_le16(&buf_ptr) != 32) { > av_log(avctx, AV_LOG_ERROR, "Unsupported decoder version!\n"); > return -1; codec version? AVERROR_INVALIDDATA? > ctx->frame_num = frame_num; > ctx->frame_flags = bytestream_get_le16(&buf_ptr); > ctx->data_size = (bytestream_get_le32(&buf_ptr) + 7) >> 3; pointless () - matter of taste > if (FFMAX3(y_offset, v_offset, u_offset) >= ctx->data_size - 16 || > FFMIN3(ctx->y_data_size, ctx->v_data_size, ctx->u_data_size) <= 0) { > av_log(avctx, AV_LOG_ERROR, "One of the y/u/v offsets is invalid\n"); > return -1; AVERROR_INVALIDDATA? > if (ctx->frame_flags & BS_8BIT_PEL) { > av_log(avctx, AV_LOG_ERROR, "8bit pixel format unsupported!\n"); > return -1; missing feature? > if (ctx->frame_flags & BS_MV_X_HALF || ctx->frame_flags & BS_MV_Y_HALF) { > av_log(avctx, AV_LOG_ERROR, "Halfpel motion vectors unsupported!\n"); > return -1; ditto > /** > * Indeo3 decoder initializations. > */ > static av_cold int decode_init(AVCodecContext *avctx) That comment looks rather redundant. > /** > * Main decoder function. > */ > static int decode_frame(AVCodecContext *avctx, void *data, int *data_size, > AVPacket *avpkt) same for this comment Diego _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
