[vlc-devel] [PATCH] Updates theora to use the new API instead of the deprecated one and fixes a crash on playback.

[Jean-Baptiste Kempf]

On Fri, Feb 10, 2012 at 12:44:15PM -0800, Theron Lewis wrote :
> The crash was caused by reading past the end of the YUV structure due
> to an incorrect line count.  This has been fixed by using the minimum
> line count common to both the source and destination of the pixel
> data.  The new ycbcr structure in the new API makes the picture copy
> much more straightforward.

Is this the infamous theora crash on win32?

> -#include <theora/theora.h>
> +/* #include <theora/theora.h> */
> +#include <theora/codec.h>
> +#include <theora/theoradec.h>
> +#include <theora/theoraenc.h>

When did that change?
You should provide ifdefs for that, IMHO

> +    p_sys->tcx = NULL;
Alignment.

> +    th_setup_info    *ts = NULL;    /* theora setup information */
idem

>     /* Sanity check that seems necessary for some corrupted files */
> -    if( p_sys->ti.width < p_sys->ti.frame_width ||
> -        p_sys->ti.height < p_sys->ti.frame_height )
> +
> +    /* Added some assertions based on the documentation.  These are
> mandatory restrictions. */
> +    assert( p_sys->ti.frame_height % 16 == 0 &&
> p_sys->ti.frame_height < 1048576 );
> +    assert( p_sys->ti.frame_width % 16 == 0 && p_sys->ti.frame_width
> < 1048576 );
> +    assert( p_sys->ti.keyframe_granule_shift >= 0 &&
> p_sys->ti.keyframe_granule_shift <= 31 );
> +
> +    assert( p_sys->ti.frame_width >= p_sys->ti.pic_width );
> +    assert( p_sys->ti.frame_height >= p_sys->ti.pic_height );
> +#define MIN(x,y) ( (x) < (y) ? (x) : (y) )
> +    assert( p_sys->ti.pic_x <= MIN( p_sys->ti.frame_width -
> p_sys->ti.pic_width, 255 ));
> +#undef MIN
> +    assert( p_sys->ti.pic_y <= p_sys->ti.frame_height - p_sys->ti.pic_height);
> +    assert( p_sys->ti.frame_height - p_sys->ti.pic_height -
> p_sys->ti.pic_y <= 255 );
> +    assert( p_sys->ti.quality >= 0 && p_sys->ti.quality <= 63 );
> +

Do we really need ALL those asserts?

Defining MIN is not ok. Use __MIN

> +    /*    if( theora_decode_header( &p_sys->ti, &p_sys->tc,
> &oggpacket ) < 0 ) */
> +    if( th_decode_headerin( &p_sys->ti, &p_sys->tc, &ts, &oggpacket ) < 0 )
Why keeping the commented code?

> +        if ( ( p_sys->tcx = th_decode_alloc( &p_sys->ti, ts ) ) == NULL )
> +        {
> +          msg_Err( p_dec, "Could not allocate Theora decoder" );
> +          goto error;
> +        }

Wrong indentation.

> +    /* Clean up the decoder setup info... we're done with it */
> +    th_setup_free( ts );
> +    ts = NULL;
>     return VLC_SUCCESS;
idem

> +    status = 1;
>     if( p_sys->b_decoded_first_keyframe )
> -        theora_decode_YUVout( &p_sys->td, &yuv );
> -    else
> +      status = th_decode_ycbcr_out( p_sys->tcx, ycbcr ); /* returns 0
> on success */
> +    if (status)
>         return NULL;
> 

Weird identation too.

Best regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device