[vlc-devel] Dynamic rendering for Kate streams

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

>  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

Ah, right, will do.


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

Since I stop profilers in error paths before returning, this would
not work, though I agree with your underlying point.

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

I use it for functions where I do not need to act on the return code,
but I will double check to make sure.

>> +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;
>>  };

Hmm, bad problem :/
I'll rework.

>> +    set_capability( "decoder", 100 )
>  Unless there is a reason, could you remove it ?

Just the priority ? OK.

>  Does kate mandate to have at least 1 header ? If yes, then it is fine.

Yes, the BOS header.

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

Doh. /me feels sheepish.

>> +    decoder_t *p_dec = p_subpic->p_sys->p_dec;
>  You cannot do that. You MUST NOT use p_dec inside this function.

OK

>  I am not sure that will work nicely with paused video. Could you check ?

I'd checked, it's fine. It doesn't work with a speed of more or less than
1 though, but I don't think it does for other subtitle types either.

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

It's to reconfigure when variables in the prefs have been changed.
It does not work at the moment as I can't get notifications as I need,
but I'd rather leave this in for when it works, unless this is a problem.

>  A goto to a label to handle the error will avoid a lot of duplicated code.

Yes. I can certainly change it so, but lots of people cringe at gotos :)

>  A hexadecimal color may avoid a lot of variables (as done by freetype
> module).

Will check this one.

> Anyway it is just my opinion, and the current code is fine if you prefer it.

More friendly to non coders I think.

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

It did happen when seeking before all headers were read (fixed in
another patch). In case it may happen again, this is for safety. It
*should* not be able to happen.

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

Ah, maybe I misunderstood what this was: I thought this meant that
this picture should go when another one arrives (which is wrong for
Kate streams, which can have overlapping events).

>  Is p_block->i_length set or not when using kate stream ? If yes, it is
> better
> to use it.

It was not when I looked IIRC.

>  And segfault. You MUST try to set p_spu->p_sys before overriding p_spu
> functions
> (at least before p_spu->pf_destroy ).

Will rework.

>> +        p_spu->p_sys->p_dec = p_dec;
>  Forbidden.

OK, thanks for the review.