[FFmpeg-devel] VP6 issues in Swfdec

[Michael Niedermayer]

Hi

On Wed, Sep 05, 2007 at 01:27:31PM +0200, Benjamin Otte wrote:
> Hi,
> 
> I'm one of the Swfdec[1] Flash player developers. As you may be aware,
> Flash uses VP6 as a possible video decoder.
> Someone recently grabbed all of the videos at http://pown.alluc.org
> and threw them at ffmpeg. Surprisingly, lots of them killed the vp6
> decoder and in turn my browser.
> 
> I'll attached a series of patches for issues that I could fix myself,
> and point out issues I could not fix myself.
> If you want to reproduce the issues, you'll have to get swfdec git,
> wget the files I'll point to and try to play them with player/swfplay
> from the swfdec sources.

some quick review below, bugfixes and actual patch approval are
our vp6 maintainers problem :)


[...]

> Index: libavcodec/vp6.c
> ===================================================================
> --- libavcodec/vp6.c	(revision 10398)
> +++ libavcodec/vp6.c	(working copy)
> @@ -131,6 +131,10 @@
>                 "alternative entropy decoding not supported\n");
>  
>      if (coeff_offset) {
> +	if (coeff_offset > buf_size) {
> +            av_log(s->avctx, AV_LOG_ERROR, "coeff_offset invalid\n");
> +            return 0;
> +	}

tabs are forbidden in ffmpeg svn


[...]
> Index: libavcodec/imgresample.c
> ===================================================================
> --- libavcodec/imgresample.c	(revision 10398)
> +++ libavcodec/imgresample.c	(working copy)
> @@ -647,6 +647,8 @@
>          ctx->av_class = av_mallocz(sizeof(AVClass));
>      if (!ctx || !ctx->av_class) {
>          av_log(NULL, AV_LOG_ERROR, "Cannot allocate a resampling context!\n");
> +	if (ctx)
> +	    av_free(ctx);

the if(ctx) is unneeded


[...]
> @@ -164,16 +165,21 @@
>   * vp56 specific range coder implementation
>   */
>  
> -static inline void vp56_init_range_decoder(vp56_range_coder_t *c,
> +static void vp56_init_range_decoder(vp56_range_coder_t *c,
>                                             uint8_t *buf, int buf_size)

this doesnt belong in the patch



>  {
>      c->high = 255;
>      c->bits = 8;
>      c->buffer = buf;
> -    c->code_word = bytestream_get_be16(&c->buffer);
> +    if (buf_size < 2) {
> +	c->buf_size = 0;
> +    } else {
> +	c->buf_size = buf_size - 2;
> +	c->code_word = bytestream_get_be16(&c->buffer);
> +    }
>  }

this check is silly, the code prior this function reads without
checking, also calling vp56_init_range_decoder on a non existing
buffer seems odd

and the checks could be simplified by keeping track of a pointer to
the end of the buffer instead of decreasing buf_size every time

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

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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/20070905/4bc43f1d/attachment.pgp>