[FFmpeg-devel] [PATCH 4/7] utvideoenc: avoid writing into the input picture.

[Derek Buitenhuis]

On 22/08/2012 10:26 AM, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> ---
>  libavcodec/utvideo.h    |    2 +-
>  libavcodec/utvideoenc.c |   55 +++++++++++++++++++++++++++++++----------------
>  2 files changed, 37 insertions(+), 20 deletions(-)

[...]

>      av_freep(&avctx->coded_frame);
>      av_freep(&c->slice_bits);
> -    av_freep(&c->slice_buffer);
> +    for(i=0; i<4; i++)
> +        av_freep(&c->slice_buffer[i]);

Should cosmetically match the rest: for (i = 0; i < 4; i++)

> +    for(i=0; i<c->planes; i++) {

Ditto.

> +        c->slice_buffer[i] = av_malloc(avctx->width * (avctx->height+1) +
> +                                       FF_INPUT_BUFFER_PADDING_SIZE);

avctx->height+1 -> avctx->height + 1


> -static void mangle_rgb_planes(uint8_t *src, int step, int stride, int width,
> +static void mangle_rgb_planes(uint8_t *dst[4], uint8_t *src, int step, int stride, int width,
>                                int height)

This looks like it might go over 80 cols?

>  {
>      int i, j;
> +    int k=width;

Space around the = and add an extra newline after.

> +        if(step==3) {

Space around ==.

> +            for (i = 0; i < width*step; i += step,k++) {

Space around the * and ,.

Perhaps k++ should get its own line at the end of the loop. Ask Jan what he prefers, I guess.

> +                unsigned g = src[i + 1];

Can this be moved to the top of the function?

> +                dst[0][k] = g;
> +                g += 0x80;
> +                dst[1][k] = src[i + 2] - g;
> +                dst[2][k] = src[i + 0] - g;
> +            }
> +        } else {
> +            for (i = 0; i < width*step; i += step,k++) {

Same comment as the other loop.

> +                unsigned g = src[i + 1];

Ditto.

> @@ -526,7 +543,7 @@ static int utvideo_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>  
>      /* In case of RGB, mangle the planes to Ut Video's format */
>      if (avctx->pix_fmt == PIX_FMT_RGBA || avctx->pix_fmt == PIX_FMT_RGB24)
> -        mangle_rgb_planes(pic->data[0], c->planes, pic->linesize[0], width,
> +        mangle_rgb_planes(c->slice_buffer, pic->data[0], c->planes, pic->linesize[0], width,

Over 80 cols?

Sorry for the Deigo-style cosmetics review :)

>From a technical standpoint, it looks OK.

- Derek