[vlc-devel] Dynamic rendering for Kate streams

[Laurent Aimar]

On Tue, Nov 18, 2008, ogg.k.ogg.k at googlemail.com wrote:
> Attached with most feedback applied. I assume the 'coding style'
> comments were about declaring locals where they are used (eg,
> C99 rather than C89).
 Nope, C99 in VLC is fine.
 I just meant that either you prefix all variables with the right prefix
(i_,b_,p_,...), or you don't do it for all (in the current case, the first
choice is required to avoid too much cosmetics). I have not added comments
for them.

>  #include <kate/kate.h>
> +#ifdef HAVE_TIGER
> +#include <tiger/tiger.h>
> +#endif
Using
#ifdef HAVE_TIGER
#   include <tiger/tiger.h>
#endif
is prefered to help readability.

> +#ifdef ENABLE_PROFILE
> +#define PROFILE_START(name) int64_t profile_start_##name = mdate()
> +#define PROFILE_STOP(name) fprintf( stderr, #name ": %f ms\n", (mdate() - profile_start_##name)/1000.0f )
> +#else
> +#define PROFILE_START(name) ((void)0)
> +#define PROFILE_STOP(name) ((void)0)
> +#endif
PROFILE_START(name) do { int64_t profile_start_##name = mdate()
and
PROFILE_STOP(name) fprintf( stderr, #name ": %f ms\n", (mdate() - profile_start_##name)/1000.0f ) } while(0)
will be safer I think (unless you mix the order of start/stop).

> +#define CHECK_TIGER_RET( statement ) \
> +    do \
> +    { \
> +        int ret = (statement); \
> +        if( ret < 0 ) \
> +        { \
> +            msg_Dbg( p_dec, "Error in " #statement ": %d", ret ); \
> +        } \
> +    } while( 0 )
 Are you sure that is a good idea ? I mean, when the function fails, you
cannot catch it anymore. Is it safe every time you use it ? (If so, it can
be kept)

> +struct subpicture_sys_t
> +{
> +    decoder_sys_t *p_dec_sys;
> +    decoder_t *p_dec;
You cannot save p_dec inside subpicture_sys_t. It's the whole point of
refcounting p_dec_sys, p_dec may be destroyed before all subpictures.
> +    mtime_t i_start;
>  };

>  vlc_module_begin ()
>      set_shortname( N_("Kate"))
> -    set_description( N_("Kate text subtitles decoder") )
> -    set_capability( "decoder", 50 )
> +    set_description( N_("Kate overlay decoder") )
> +    set_help( HELP_TEXT )
> +    set_capability( "decoder", 100 )
 Unless there is a reason, could you remove it ?

>      {
> -        if( p_sys->i_headers >= p_sys->i_num_headers )
> +        if( p_sys->i_headers >= p_sys->i_num_headers && p_sys->i_num_headers > 0)
 Does kate mandate to have at least 1 header ? If yes, then it is fine.

> +            uint8_t *pixel = line+x*4;
> +#ifdef WORDS_BIGENDIAN
> +            uint8_t a = pixel[0];
> +#else
> +            uint8_t a = pixel[3];
> +#endif
> +            if( a )
> +            {
> +#ifdef WORDS_BIGENDIAN
> +                pixel[3] = a;
> +                pixel[2] = pixel[1] * 255 / a;
> +                pixel[1] = pixel[2] * 255 / a;
> +                pixel[0] = pixel[3] * 255 / a;
That cannot work, you overwrite values...

> +/* Tiger renders can end up looking a bit crap since they get overlaid on top of
> +   a subsampled YUV image, so there can be a fair amount of chroma bleeding.
> +   Looks good with white though since it's all luma. Hopefully that will be the
> +   common case. */
> +static void TigerUpdateRegions( spu_t *p_spu, subpicture_t *p_subpic, const video_format_t *p_fmt, mtime_t ts )
> +{
> +    decoder_t *p_dec = p_subpic->p_sys->p_dec;
 You cannot do that. You MUST NOT use p_dec inside this function.

> +    /* do not render more than once per frame, libtiger renders all events at once */
> +    if (ts <= p_sys->last_render_ts)
> +    {
> +        SubpictureReleaseRegions( p_subpic );
> +        return;
> +    }
 I am not sure that will work nicely with paused video. Could you check ?

> +    PROFILE_START( UpdateTigerVariables );
> +    UpdateTigerVariables( p_dec, false ); // TODO: should detect actual changes and force a rerender - callback
> +    PROFILE_STOP( UpdateTigerVariables );
 Why is that needed here ?

> +    if( ret < 0 )
> +    {
> +        msg_Dbg( p_dec, "failed to set tiger renderer buffer: %d - overlay will not be rendered", ret );
> +        vlc_mutex_unlock( &p_sys->lock );
> +        subpicture_region_ChainDelete( r );
> +        return;
> +    }
 A goto to a label to handle the error will avoid a lot of duplicated code.
> +
> +    PROFILE_START( tiger_renderer_update );
> +    ret = tiger_renderer_update( p_sys->tr, t, 1 );
> +    if( ret < 0 )
> +    {
> +        msg_Dbg( p_dec, "failed to update tiger renderer: %d - overlay will not be rendered", ret );
> +        vlc_mutex_unlock( &p_sys->lock );
> +        subpicture_region_ChainDelete( r );
> +        return;
> +    }
 Here
> +    PROFILE_STOP( tiger_renderer_update );
> +
> +    PROFILE_START( tiger_renderer_render );
> +    ret = tiger_renderer_render( p_sys->tr );
> +    if( ret < 0 )
> +    {
> +        msg_Dbg( p_dec, "failed to render with tiger: %d - overlay will not be rendered", ret );
> +        vlc_mutex_unlock( &p_sys->lock );
> +        subpicture_region_ChainDelete( r );
> +        return;
> +    }
 And here.
> +    PROFILE_STOP( tiger_renderer_render );
> +
> +    vlc_mutex_unlock( &p_sys->lock );
> +
> +    PostprocessTigerImage( plane, fmt.i_width );
> +
> +    p_subpic->p_region = r;
> +
> +    PROFILE_STOP( TigerUpdateRegions );
> +}

> +
> +#if 0
> +/* I can't get this to get called with a pointer to the decoder, or decoder_sys_t */
> +static int TigerConfigurationCallback( vlc_object_t *p_this, const char *psz_var,
> +                                       vlc_value_t oldvar, vlc_value_t newval,
> +                                       void *p_data )
> +{
> +    decoder_t *p_dec = (decoder_t*)p_this;
> +    decoder_sys_t *p_sys = (decoder_sys_t*)p_dec->p_sys;
> +
> +    VLC_UNUSED( oldvar );
> +    VLC_UNUSED( p_data );
> +
> +    msg_Dbg( p_dec, "TigerConfigurationCallback: %s (%s)\n", psz_var, p_data );
> +
> +    if( !p_sys->b_use_tiger || !p_sys->tr )
> +    {
> +        return VLC_SUCCESS;
> +    }
> +
> +#define TEST_TIGER_VAR(name, field) \
> +    do { \
> +        if( !strcmp( psz_var, name )) \
> +        { \
> +            msg_Dbg( p_dec, "TigerConfigurationCallback: " #name " was updated\n" ); \
> +            p_sys->update_##field=1; \
> +        } \
> +    } \
> +    while( 0 )
 Missing lock.
 b_update_* are boolean value, so use true iof 1.

> +    TEST_TIGER_VAR( "kate-tiger-default-font-red", font_color );
> +    TEST_TIGER_VAR( "kate-tiger-default-font-green", font_color );
> +    TEST_TIGER_VAR( "kate-tiger-default-font-blue", font_color );

> +    TEST_TIGER_VAR( "kate-tiger-default-background-red", background_color );
> +    TEST_TIGER_VAR( "kate-tiger-default-background-green", background_color );
> +    TEST_TIGER_VAR( "kate-tiger-default-background-blue", background_color );
 A hexadecimal color may avoid a lot of variables (as done by freetype module).
Anyway it is just my opinion, and the current code is fine if you prefer it.

>  /*****************************************************************************
>   * DecodePacket: decodes a Kate packet.
>   *****************************************************************************/
> @@ -549,12 +1068,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;
> +    }
 Can it happen ? With broken stream maybe ?

> +    p_spu->i_start = p_block->i_pts;
> +    p_spu->i_stop = p_block->i_pts + INT64_C(1000000)*ev->duration*p_sys->ki.gps_denominator/p_sys->ki.gps_numerator;
> +    p_spu->b_ephemer = 0;
 false iof 0

 I don't think it is right, could you test playing faster ?
 If it is not, I think that doing it like libass might work better. That is,
using ephemer picture.
 Is p_block->i_length set or not when using kate stream ? If yes, it is better
to use it.

> +#ifdef HAVE_TIGER
> +    if( p_sys->b_use_tiger)
> +    {
> +        /* hookup render/update routines */
> +        p_spu->pf_pre_render = TigerPreRender;
> +        p_spu->pf_update_regions = TigerUpdateRegions;
> +        p_spu->pf_destroy = TigerDestroySubpicture;
> +
> +        /* setup the structure to get our decoder struct back */
> +        p_spu->p_sys = malloc( sizeof( subpicture_sys_t ));
> +        if( !p_spu->p_sys )
> +        {
> +            decoder_DeleteSubpicture( p_dec, p_spu );
 And segfault. You MUST try to set p_spu->p_sys before overriding p_spu functions
(at least before p_spu->pf_destroy ).
 
> +            return NULL;
> +        }
> +        p_spu->p_sys->p_dec_sys = p_sys;
> +        p_spu->p_sys->p_dec = p_dec;
 Forbidden.

-- 
fenrir