[vlc-devel] [PATCH] Use newer theora API and fix windows crash (Close #3841)

[Jean-Baptiste Kempf]

On Fri, Feb 10, 2012 at 03:22:32PM -0800, Theron Lewis wrote :
> This patch updates the theora module to use the newer API introduced
> in Theora version 1.0 and solves a crash in windows that 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.
> 
> Close #3841


> +/* Requires theora >= 1.0 */

Useless comment. Configure.ac does this check already.

> +#define __MIN(x,y) ( (x) < (y) ? (x) : (y) )
> +#undef __MIN

No. vlc_common.h already defines this.

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

Is this assignment to NULL useful?

> +    ts = NULL;

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;

Do you really need the status variable here?


> -        psz_comment = strdup( p_dec->p_sys->tc.user_comments[i] );
> +        int clen = p_dec->p_sys->tc.comment_lengths[i];
> +        if ( !clen ) { i++; continue; }
> +        psz_comment = (char *) memcpy( malloc(clen + 1), (void*) p_dec->p_sys->tc.user_comments[i], clen + 1 ); /* copy the terminating null */

What happens when malloc fails here?

> +#define __MIN(x,y) ( (x) < (y) ? (x) : (y) )

As above.


> +    /* turn on fast encoding */

Why?

> +    status = th_encode_ctl( p_sys->tcx, TH_ENCCTL_GET_SPLEVEL_MAX, &max_enc_level, sizeof(max_enc_level) );
> +    if (!status) {
> +        status = th_encode_ctl( p_sys->tcx, TH_ENCCTL_SET_SPLEVEL, &max_enc_level, sizeof(max_enc_level) );
> +    }

Do you need status here?

> +    /* Set forced distance between key frames */
> +    status = th_encode_ctl( p_sys->tcx, TH_ENCCTL_SET_KEYFRAME_FREQUENCY_FORCE, &keyframe_freq_force, sizeof(keyframe_freq_force) );

And here?


Did you check that we pass the theora test-suite with this?


Best regards,

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