[vlc-devel] [PATCH] kate decoder: track and use basic formatting

[Laurent Aimar]

On Mon, Jul 28, 2008, ogg.k.ogg.k at googlemail.com wrote:
> this essentially fixes image based subtitles when the video was encoded
> at a lower resolution as the original.
> +/*
> +  This retrieves the size of the video.
> +  The best case is when the original video size is known, as we can then
> +  scale images to match. In this case, since VLC autoscales, we want to
> +  return the original size and let VLC scale everything.
> +  if the original size is not known, then VLC can't resize, so we return
> +  the size of the incoming video. If sizes in the Kate stream are in
> +  relative units, it works fine. If they are absolute, you get what you
> +  ask for. Images aren't rescaled.
> +*/
> +static void GetVideoSize(decoder_t *p_dec, int *w, int *h)
> +{
> +    vout_thread_t *p_vout;
> +    p_vout = vlc_object_find( (vlc_object_t*)p_dec, VLC_OBJECT_VOUT, FIND_ANYWHERE );
 I strongly disagree with that:
 - you can have multiple vout.
 - if we recycle vout you may using a not used vout.
 - you are not assured to be called after the vout is created.
and we tryed to remove all vlc_object_find FIND_ANYWHERE.
 If kate decoding needs the video size, it should be stored in its metadata...

> +    if( p_vout )
> +    {
> +        decoder_sys_t *p_sys = p_dec->p_sys;
> +        if (p_sys->ki.original_canvas_width > 0) {
> +            *w = p_sys->ki.original_canvas_width;
> +        }
> +        else {
> +            *w = p_vout->fmt_in.i_width;
> +        }
> +        if (p_sys->ki.original_canvas_height > 0) {
> +            *h = p_sys->ki.original_canvas_height;
> +        }
> +        else {
> +            *h = p_vout->fmt_in.i_height;
> +        }
 You cannot access *internal* vout fields without a risk of race conditions...

> +            if (p_sys->b_formatted) {
> +                size_t len0 = ev->len0;
> +                const char *prefix = "<text>";
> +                const char *suffix = "</text>";
> +                char *copy = (char*)malloc(ev->len0+strlen(prefix)+strlen(suffix));
> +                strcpy(copy, prefix);
> +                memcpy(copy+strlen(prefix), ev->text, len0);
> +                strcat(copy, suffix);
 Using asprintf would be simpler and way easier to review (and to avoid
misallocating memory). If you don't want to use asprintf, at least add a
comment stating if the '\0' is included in len0.

 +            /* we don't know about this one, so remove markup and display as text */
> +            {
> +                size_t len0 = ev->len0;
> +                char *copy = (char*)malloc(ev->len0);
> +                memcpy(copy, ev->text, len0);
 strdup or strndup would be better.

-- 
fenrir