[vlc-devel] Dynamic rendering for Kate streams

[ogg.k.ogg.k at googlemail.com]

> Here is a first pass review.

Thanks

>  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.

OK, same thing here (one subpicture for the whole subtitles stream).

>  RGBA from libass seems really good.

Hmm, I'll check again to make sure I didn't do anything wrong.

> Btw, what is "Premultiplied alpha" ?

It's a color with its RGB components being premultiplied by the A
component. For instance, RGBA ff ff ff 80 will be 80 80 80 80 in
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...

Here, src2 would be already multiplied by a (assuming that a is
the alpha component of the src2 pixel).

>> +    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.

OK

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

OK

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

What do you mean ? I've tried keeping to the whitespace rules VLC
uses, this seems OK, what did you refer to ?

>  Using p_block will be cleaner (same variable that the one in the test.

OK

>>      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).

OK. Actually, that makes me think that changing video speed doesn't
really work with my code here, forgot to mention it. VLC doesn't keep
a virtual time that takes the speed into account, so I have no way that
I know of to compute the current position within the subtitle. I think this
is true of any subtitle too.

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

Sorry, can you explain ? Do you mean the use of the macro ? The
definition of a local variable not at function start ? Something else ?

>> +                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).

OK

>> +                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.

OK (why is float a BAD idea ? speed ?)

> Besides you MUST check for a == 0.

It is checked above.

>> +    /* 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 ?

All subtitles are added to the same renderer, and rendered only once.
This function is called for every subpicture. Only one of them will have
a region attached, and libtiger will render on this one. This test is to
ensure only one render happens, even if there overlapping events.

>> +    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 ?

No, but I don't have a big endian machine to test.

>  Is there a way to NOT have only 1 region ? It will be killing slow.

It's not too bad actually. There are plans to add a bounding box to
the rendering, so you know which area was actually rendered too.
That's not in yet though.

>> +    if( !p_sys->b_ready )
>> +    {
>> +        msg_Err( p_dec, "Cannot decode Kate packet, decoder not
>> initialized" );
>> +        return NULL;
>> +    }
>  Why is it needed ?

Safety. It could happen if seeking before the decoder had seen all
headers (fixed in another patch of mine, not applied yet I think). In
case it could happen in other cases, this code ensures an uninitialized
state will not be used.

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

All events are timed properly, so ephemer should be false.
absolute is set to true when required below.

>> +        /* 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.

OK

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

Hmm, kay.

>> +            p_dec->pf_spu_buffer_del( p_dec, p_spu );
> Please use decoder_DeleteSubpicture.

Oops, thanks.