[FFmpeg-devel] Request for review

[Michael Niedermayer]

On Mon, Nov 17, 2008 at 10:35:27AM -0800, Roman V. Shaposhnik wrote:
> Attach are three patches:
> 
>   * functions.patch -- is a simple cosmetic change that removes
>                        unnecessary wrapper functions
>   * tables.patch    -- this one finally does away with huge static
>                        DV tables. I had to edit it a bit to fit
>                        into this list's email size quota.
>   * iweight.patch   -- in the same line of thought makes factor
>                        tables dynamic and moves them to DVprofile
> 
> Please review and comment.
> 
> Thanks,
> Roman.

> diff --git a/libavcodec/dv.c b/libavcodec/dv.c
> index a3f0511..c8c861e 100644
> --- a/libavcodec/dv.c
> +++ b/libavcodec/dv.c
> @@ -395,8 +395,9 @@ static inline void dv_calculate_mb_xy(DVVideoContext *s, DVwork_chunk *work_chun
>  }
>  
>  /* mb_x and mb_y are in units of 8 pixels */
> -static inline void dv_decode_video_segment(DVVideoContext *s, DVwork_chunk *work_chunk)
> +static int dv_decode_video_segment(AVCodecContext *avctx, DVwork_chunk *work_chunk)
>  {

> +    DVVideoContext *s = (DVVideoContext *)avctx->priv_data;

i think the cast is unneeded

except that first patch ok


[...]

> diff --git a/libavcodec/dv.c b/libavcodec/dv.c
> index c8c861e..f798584 100644
> --- a/libavcodec/dv.c
> +++ b/libavcodec/dv.c
> @@ -90,6 +90,121 @@ static inline int dv_work_pool_size(const DVprofile *d)
>      return size;
>  }
>  
> +static inline void dv_calc_mb_coordinates(const DVprofile *d, int chan, int seq, int slot, 
> +                                          uint16_t *tbl)
> +{

> +    const uint8_t off[] = { 2, 6, 8, 0, 4 };

should be const static to make sure stupid compilers dont generate these
tables in each function call on the stack

except that, second patch ok

[...]


> -    if (d->work_chunks[dv_work_pool_size(d)-1].buf_offset)
> -        return 0;
> -
> +    if (!d->work_chunks[dv_work_pool_size(d)-1].buf_offset) {
>      p = i = 0;
>      for (c=0; c<d->n_difchan; c++) {
>          for (s=0; s<d->difseg_size; s++) {
> @@ -227,42 +225,45 @@ static int dv_init_dynamic_tables(const DVprofile *d)
>              }
>          }
>      }
> -    return 0;
>  }
>  

ok, though this seems unrelated and could be a seperate commit


[...]
> diff --git a/libavcodec/dvdata.h b/libavcodec/dvdata.h
> index d673d60..0fdba57 100644
> --- a/libavcodec/dvdata.h
> +++ b/libavcodec/dvdata.h
> @@ -53,6 +53,7 @@ typedef struct DVprofile {
>      int              width;                 /* picture width in pixels */
>      AVRational       sar[2];                /* sample aspect ratios for 4:3 and 16:9 */
>      DVwork_chunk    *work_chunks;           /* each thread gets its own chunk of frame to work on */

> +    uint32_t        *idct_factor;           /* FIXME */

why "FIXME" ?



[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081118/70a31bd2/attachment.pgp>