On Wed, Dec 07, 2011 at 03:41:39PM +0100, Kostya Shishkov wrote:
>
> --- /dev/null
> +++ b/libavcodec/indeo4.c
> @@ -0,0 +1,830 @@
> + ctx->transp_status = get_bits1(&ctx->gb);
> + if (ctx->transp_status) {
> +#if IVI4_STREAM_ANALYSER
> + has_transp = 1;
> +#endif
> + }
I think you should move the if inside of the ifdef and not rely on
the compiler optimizing the check away.
> + ctx->data_size = (get_bits1(&ctx->gb)) ? get_bits_long(&ctx->gb, 24) : 0;
unneeded parentheses around get_bits1()
> + /* Check key lock status. If enabled - ignore lock word. */
> + /* Usually we have to prompt the user for the password, but */
> + /* we don't do that because indeo4 videos can be decoded anyway */
slightly strange comment syntax
> + if (get_bits1(&ctx->gb)) {
> + skip_bits_long(&ctx->gb, 32);
> + //av_log(avctx, AV_LOG_INFO, "Password-protected clip!\n");
> + }
av_dlog?
> + /* Decode chroma subsampling. We support only 4:4 aka YVU9. */
> + if (get_bits(&ctx->gb, 2)) {
> + av_log(avctx, AV_LOG_ERROR, "Only YVU9 picture format is
> supported!\n");
> + return AVERROR_INVALIDDATA;
> + }
av_log_missing_feature or av_log_ask_for_sample?
> + pic_conf.chroma_height = (pic_conf.pic_height + 3) >> 2;
> + pic_conf.chroma_width = (pic_conf.pic_width + 3) >> 2;
arguably unneeded parentheses
> + ctx->frame_num = (get_bits1(&ctx->gb)) ? get_bits_long(&ctx->gb, 20) : 0;
unneeded parentheses around get_bits1()
> + /* skip decTimeEst field if present */
> + if (get_bits1(&ctx->gb)) skip_bits(&ctx->gb, 8);
Break this line please.
> + ctx->rvmap_sel = (get_bits1(&ctx->gb)) ? get_bits(&ctx->gb, 3) : 8;
unneeded parentheses around get_bits1()
> + /* TODO: ignore this parameter if unused */
> + ctx->unknown1 = (get_bits1(&ctx->gb)) ? get_bits(&ctx->gb, 3) : 0;
> +
> + ctx->checksum = (get_bits1(&ctx->gb)) ? get_bits(&ctx->gb, 16) : 0;
more
> + /* skip picture header extension if any */
> + while (get_bits1(&ctx->gb)) {
> + //av_log(avctx, AV_LOG_DEBUG, "Pic hdr extension encountered!\n");
> + val = get_bits(&ctx->gb, 8);
> + }
av_dlog?
> + scan_indx = get_bits(&ctx->gb, 4);
> + if (scan_indx == 15) {
> + av_log(avctx, AV_LOG_ERROR, "Custom scan pattern
> encountered!\n");
> + return AVERROR_INVALIDDATA;
> + }
> + band->scan = scan_index_to_tab[scan_indx];
> +
> + band->quant_mat = get_bits(&ctx->gb, 5);
> + if (band->quant_mat == 31) {
> + av_log(avctx, AV_LOG_ERROR, "Custom quant matrix
> encountered!\n");
> + return AVERROR_INVALIDDATA;
> + }
av_log_missing_feature or av_log_ask_for_sample?
> + /* decode block huffman codebook */
> + if (ff_ivi_dec_huff_desc(&ctx->gb, get_bits1(&ctx->gb),
> IVI_BLK_HUFF, &band->blk_vlc, avctx))
> + return AVERROR_INVALIDDATA;
nit: long line
> + /* select appropriate rvmap table for this band */
> + band->rvmap_sel = (get_bits1(&ctx->gb)) ? get_bits(&ctx->gb, 3) : 8;
parentheses
> + if (!band->plane && !band->band_num && ctx->in_q) {
> + mb->q_delta = get_vlc2(&ctx->gb, ctx->mb_vlc.tab->table,
> + IVI_VLC_BITS, 1);
Indentation is off by one space.
> + /* decode motion vector deltas */
> + mv_delta = get_vlc2(&ctx->gb, ctx->mb_vlc.tab->table,
> + IVI_VLC_BITS, 1);
> + mv_y += IVI_TOSIGNED(mv_delta);
> + mv_delta = get_vlc2(&ctx->gb, ctx->mb_vlc.tab->table,
> + IVI_VLC_BITS, 1);
ditto
> + if (tile->is_empty) {
> + ff_ivi_process_empty_tile(avctx, band, tile,
> + (ctx->planes[0].bands[0].mb_size >> 3)
> - (band->mb_size >> 3));
Indentation is off by one space.
> + //av_log(avctx, AV_LOG_WARNING, "Empty tile encountered!\n");
av_dlog?
> + pos += tile->data_size << 3; // skip to next tile
> + //skip_bits_long(&ctx->gb, pos - get_bits_count(&ctx->gb));
commented-out cruft
> +#if defined(DEBUG) && IVI4_DEBUG_CHECKSUM
> + if (band->checksum_present) {
double debug check?
> +static av_cold int decode_init(AVCodecContext *avctx)
> +{
> + IVI4DecContext *ctx = avctx->priv_data;
nit: double space
> + /* Force allocation of the internal buffers */
> + /* during picture header decoding. */
> + ctx->pic_conf.pic_width = 0;
> + ctx->pic_conf.pic_height = 0;
ditto
nit: uncommon comment syntax
> + switch (ctx->prev_frame_type) {
> + case FRAMETYPE_INTRA:
> + case FRAMETYPE_INTER:
> + ctx->buf_switch ^= 1;
> + ctx->dst_buf = ctx->buf_switch;
> + ctx->ref_buf = ctx->buf_switch ^ 1;
nit: align
> + switch (ctx->frame_type) {
> + case FRAMETYPE_INTRA:
> + ctx->buf_switch = 0;
> + /* FALLTHROUGH */
> + case FRAMETYPE_INTER:
> + //ctx->inter_scal = 0;
> + ctx->dst_buf = ctx->buf_switch;
> + ctx->ref_buf = ctx->buf_switch ^ 1;
> + break;
> + //case FRAMETYPE_INTER_SCAL:
> + case FRAMETYPE_INTER_NOREF:
> + case FRAMETYPE_NULL:
> + break;
commented-out cruft
> + /* If the bidirectional mode is enabled, next I and the following P
> frame will */
> + /* be sent together. Unfortunately the approach below seems to be the
> only way */
> + /* to handle the B-frames mode. That's exactly the same Intel decoders
> do. */
nit: comment syntax
> +#if IVI4_STREAM_ANALYSER
> + if (ctx->is_scalable)
> + av_log(avctx, AV_LOG_ERROR, "This video uses scalability mode!\n");
> + if (uses_tiling)
> + av_log(avctx, AV_LOG_ERROR, "This video uses local decoding!\n");
> + if (has_b_frames)
> + av_log(avctx, AV_LOG_ERROR, "This video contains B-frames!\n");
> + if (has_transp)
> + av_log(avctx, AV_LOG_ERROR, "Transparency mode is enabled!\n");
> + if (uses_haar)
> + av_log(avctx, AV_LOG_ERROR, "This video uses Haar transform!\n");
> + if (uses_fullpel)
> + av_log(avctx, AV_LOG_ERROR, "This video uses fullpel motion
> vectors!\n");
> +#endif
av_dlog?
> --- a/libavcodec/ivi_dsp.c
> +++ b/libavcodec/ivi_dsp.c
> @@ -1,7 +1,7 @@
> @@ -178,6 +178,149 @@ void ff_ivi_recompose53(const IVIPlaneDesc *plane,
> uint8_t *dst,
>
> + /* haar wavelet recomposition */
> + p0 = (b0 + b1 + b2 + b3 + 2) >> 2;
> + p1 = (b0 + b1 - b2 - b3 + 2) >> 2;
> + p2 = (b0 - b1 + b2 - b3 + 2) >> 2;
> + p3 = (b0 - b1 - b2 + b3 + 2) >> 2;
arguably pointless parentheses
> --- a/libavcodec/ivi_dsp.h
> +++ b/libavcodec/ivi_dsp.h
> @@ -44,6 +44,42 @@ void ff_ivi_recompose53(const IVIPlaneDesc *plane, uint8_t
> *dst,
>
> +/**
> + * DC-only two-dimensional inverse Haar transform for Indeo 4.
> + * Performing the inverse transform in this case is equivalent to
> + * spreading DC_coeff >> 3 over the whole block.
> + *
> + * @param[in] in pointer to the dc coefficient
> + * @param[out] out pointer to the output buffer (frame)
> + * @param[in] pitch pitch to move to the next y line
> + * @param[in] blk_size transform block size
> + */
> +void ff_ivi_dc_haar_2d(const int32_t *in, int16_t *out, uint32_t pitch, int
> blk_size);
nit: long line
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
