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

[Derk-Jan Hartman]

On 29 jul 2008, at 10:35, ogg.k.ogg.k at googlemail.com wrote:
>> I strongly disagree with that:
>> - you can have multiple vout.
>
> Oh, I didn't realize.

You need to at least use FIND_CHILD, though even then there can be  
multiple vout (audio visualization filter and the actual video window  
for instance, or two enabled video tracks [quite rare, and not allowed  
by VLC interface, but possible with some mp4 samples for instance] ).

>> - you are not assured to be called after the vout is created.
>> and we tryed to remove all vlc_object_find FIND_ANYWHERE.
>
> Fair enough. I spent a lot of time trying to work out how to get the
> size of the video, and eventually that's the only way I found. I'd
> be happy to change it to another method if there is one. That way
> to get the size was found in the CMML plugin.

We have a trick for this I believe...
In the demux (ogg in this case), you set  
fmt.subs.spu.i_original_frame_width to the width of the video stream.
Then in your codec you read this information back and you use it:

p_spu->i_original_picture_width =
         p_dec->fmt_in.subs.spu.i_original_frame_width;

This was originally intended for vobsub/DVD subtitles, but it might  
work here as well.

>> If kate decoding needs the video size, it should be stored in its
>> metadata...
>
> This is not possible: positions and sizes may be expressed in
> relative units to the displayed video (eg, in percentages). This is
> to allow placement, eg, in corners. For this, the video size must
> be known at runtime. If the video size is known beforehand and
> cannot change, then sizes may be expressed in pixels instead.

VLC has no active coupling between these after they leave the demuxer.
A decoder does not have/nor should have any active dependence on  
another codec or the displaying core while it is running. It should be  
able to function on its own.

>> You cannot access *internal* vout fields without a risk of race
>> conditions...
>
> OK, I'll have a look to see what locking I need to add (unless your
> "internal" wording means I should not access them in the first  
> place ?).

I think the above method is preferred. it does not introduce ugly  
locking requirements etc.

>
>> 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.
>
> asprintf is a GNU extension, is it OK to use here ?

Doesn't matter, VLC defines it's own asprintf if a platform does not  
support it.

>>> +                char *copy = (char*)malloc(ev->len0);
>>> +                memcpy(copy, ev->text, len0);
>> strdup or strndup would be better.
>
> It would only copy one zero - granted, that's enough as only UTF-8 is
> supported at the moment, but that's the point of len0.

I think the point is that care is required (and comments don't hurt  
here either)

DJ