[vlc-devel] Dynamic rendering for Kate streams

Laurent Aimar fenrir at via.ecp.fr
Fri Nov 14 18:47:29 CET 2008


Hi,
Here is a first pass review.

On Thu, Nov 13, 2008, ogg.k.ogg.k at googlemail.com wrote:
> Also, I've looked at the libass code Fenrir pointed to, and I don't
> see what's the
> use of the pf_re_render function: it's called just before pf_update_regions, and
> the test for the final_spu buffer puzzles me. It should not be needed,
> what is the
> purpose of this ?
 SSA decoder may create multiple subtitles per video picture (overlapping event).
But libass will create only one subtitle that will merge them.
 This callback is called before any rendering, so you can have an overview of
what will be used for one picture.
 Libass uses it to retain only one subpicture.

> Last, rendering doesn't look as good as it does in GStreamer as I have
> to draw to
> a SPU that then gets merged onto a subsampled YUV video. This means chroma
> bleeding. Pure green text looks not very nice, though white looks good. Fiddling
> with the blend.c function that gets used, I managed to get rid of that
> bleeding, but
 Our current rgba -> 420 blending is awfull and aliased. Any improvment is welcome.

> at the cost of other artifacts, and it doesn't look better all in all,
> so I'll leave it as
> is unless there's a known trick to get colored rendering looking better.
 RGBA from libass seems really good.

Btw, what is "Premultiplied alpha" ?

Our blending works like that
 dst = ( src1 * (255 - a) + src2 * a ) / 255
what outputs tiger ?
It cannot output src * a as it will overflow and src2 * a /255 is wrong...

> From 3ed8752740f99da5a02688c71c162a93c85ad8ad Mon Sep 17 00:00:00 2001
> From: Vincent Penquerc'h <ogg.k.ogg.k at googlemail.com>
> Date: Tue, 11 Nov 2008 20:45:05 +0000
> Subject: [PATCH] support for rendering Kate streams with libtiger
> 
> libtiger is a rendering library for Kate streams based on Pango
> and Cairo, and supports almost everything Kate streams can do.
> 
> There's a bit of wonkiness with rendering on regions as VLC then
> blends them onto a subsampled YUV buffer, and colors can bleed
> in a not so nice way. I've tried frobbing the blender in blend.c
> and I got rid of the chroma bleeding, but at the cost of other
> artifacts, so I'm leaving it out.
> 
> Also, I can't find a way to get the preferences dialog to call
> a callback on my variables with a pointer to the decoder, so
> changing the variables will not have effect till VLC is started
> again.
> 
> I've added libtiger to the contribs setup, but since it depends
> on Pango and Cairo, building it fully via contribs if those
> dependencies are not there already will be tricky.
> 
> Also includes the misc kate fixes from last week's patch, which
> was not applied yet.

> +    /* should (when it works) be set by a callback when the relevant variable is changed */
> +    uint32_t    update_font_desc:1;
> +    uint32_t    update_font_color:1;
> +    uint32_t    update_background_color:1;
> +    uint32_t    update_quality:1;
> +    uint32_t    update_font_effect:1;
 Use bool instead of bit field there.

> +#ifdef HAVE_TIGER
> +    bool   b_use_tiger;
>  #endif
>  };
>  
> +#ifdef HAVE_TIGER
> +struct subpicture_sys_t
> +{
> +    decoder_sys_t *p_dec_sys;
> +    decoder_t *p_dec;
> +    mtime_t i_start;
> +};
> +#endif

 I don't think you need to ifdef out everything. As long as it compils, you
 may let it in.

> +        int ret = tiger_renderer_create( &p_sys->tr );
 Please, use a consistant coding scheme in the complete file.

>      p_block = *pp_block;
> +
> +    if( p_block->i_flags & (BLOCK_FLAG_CORRUPTED) )
> +    {
> +        block_Release( *pp_block );
 Using p_block will be cleaner (same variable that the one in the test.

> +    if( p_block->i_flags & (BLOCK_FLAG_DISCONTINUITY) )
> +    {
> +#ifdef HAVE_TIGER
> +        /* Hmm, should we wait before flushing the renderer ? I think not, but not certain... */
> +        vlc_mutex_lock( &p_sys->lock );
> +        tiger_renderer_seek( p_sys->tr, 0 );
> +        vlc_mutex_unlock( &p_sys->lock );
> +#endif
> +        block_Release( *pp_block );
 Same here.
> +        return NULL;
> +    }

>      if( p_block->i_rate != 0 )
>          p_block->i_length = p_block->i_length * p_block->i_rate / INPUT_RATE_DEFAULT;
 This code should be removed (I have missed it).

> +        const char *desc = GET_TIGER_VAR( String, "kate-tiger-default-font-desc" );
 Use a consistant coding scheme (and the same for all following)..

> +static void DepremuliplyAlpha( plane_t *plane, unsigned int i_width )
> +{
> +    PROFILE_START( tiger_renderer_postprocess );
> +    int y;
> +    for( y=0; y<plane->i_lines; ++y )
> +    {
> +        uint32_t *line = (uint32_t*)(plane->p_pixels + y*plane->i_pitch);
> +        unsigned int x;
> +        for( x=0; x<i_width; ++x )
> +        {
> +            uint32_t *pixel = line+x;
> +            uint32_t a = (*pixel)&0xff000000;
> +            if( a )
> +            {
> +                uint32_t r = ((*pixel)&0x00ff0000)>>16;
> +                uint32_t g = ((*pixel)&0x0000ff00)>>8;
> +                uint32_t b = ((*pixel)&0x000000ff)>>0;
Littel/big indian issue.
RGBA is always store:
r = p[0];
g = p[1];
b = p[2];
a = p[3];

And please use the right type for pixel (uint8_t).

> +                float mult = 255.0/(a>>24);
> +                r = r * mult;
> +                g = g * mult;
> +                b = b * mult;
 Using float is a real BAD idea.
 r = r * 255 / a will be faster.
Besides you MUST check for a == 0.

> +    /* do not render more than once per frame, libtiger renders all events at once */
> +    if (ts <= p_sys->last_render_ts)
 Are you sure it is needed ? What happens when a frame is displayed for a long time.
Shouldn't the subtitle be updated anyway ?

> +    plane = &r->p_picture->p[0];
> +    ret = tiger_renderer_set_buffer( p_sys->tr, plane->p_pixels, fmt.i_width, plane->i_lines, plane->i_pitch, 1 );
 Btw, are you sure that tiger use vlc compatible RGBA layout for big/little endian ?

> +    vlc_mutex_unlock( &p_sys->lock );
> +
> +    /* We get premultiplied alpha, but VLC doesn't expect this, so we demultiply
> +       alpha to avoid double multiply (and thus thinner text than we should)).
> +       Best would be to have VLC be able to handle premultiplied alpha, as it
> +       would be faster to blend as well. */
> +    DepremuliplyAlpha( plane, fmt.i_width );
> +
> +    p_subpic->p_region = r;
 Is there a way to NOT have only 1 region ? It will be killing slow.

>  /*****************************************************************************
>   * DecodePacket: decodes a Kate packet.
>   *****************************************************************************/
> @@ -549,12 +1050,13 @@ static subpicture_t *DecodePacket( decoder_t *p_dec, kate_packet *p_kp, block_t
>      decoder_sys_t *p_sys = p_dec->p_sys;
>      const kate_event *ev = NULL;
>      subpicture_t *p_spu = NULL;
> -    subpicture_region_t *p_bitmap_region = NULL;
>      int ret;
> -    video_format_t fmt;
> -    video_format_t palette;
> -    kate_tracker kin;
> -    bool tracker_valid = false;
> +
> +    if( !p_sys->b_ready )
> +    {
> +        msg_Err( p_dec, "Cannot decode Kate packet, decoder not initialized" );
> +        return NULL;
> +    }
 Why is it needed ?

> +    p_spu->b_ephemer = 0;
> +    p_spu->b_absolute = false;
 They should probably be true (at least the absolute).

> +        /* hookup render/update routines */
> +        p_spu->pf_pre_render = &TigerPreRender;
> +        p_spu->pf_update_regions = &TigerUpdateRegions;
> +        p_spu->pf_destroy = &TigerDestroySubpicture;
 We do not use & to take function ptr in vlc.

> +        /* setup the structure to get our decoder struct back */
> +        p_spu->p_sys = (subpicture_sys_t*)malloc( sizeof( subpicture_sys_t ));
 Useless cast.

> +        if( !p_spu->p_sys )
> +        {
> +            p_dec->pf_spu_buffer_del( p_dec, p_spu );
Please use decoder_DeleteSubpicture.

-- 
fenrir




More information about the vlc-devel mailing list