[vlc-devel] [PATCH] vobsub idx in mkv track CodecPrivate data

[John Stebbins]

On 05/12/2009 08:14 AM, Jean-Baptiste Kempf wrote:
> On Tue, May 12, 2009 at 08:02:37AM -0700, John Stebbins wrote :
>    
>> I'm adding vobsub support to handbrake and was using vlc as one means to
>> verify my work.  I noticed that vlc wasn't using the palette contained
>>      
> Great.
>
>    
>> in the mkv track CodecPrivate data.  When I looked deeper into it, I
>> discovered it was ignoring the idx data entirely.  Here's a patch to
>> modules/demux/avformat/demux.c that checks that the container is
>> matroska, and that the subtitle type is vobsub.  Then parses
>> AVCodecContext.extradata (where avformat stashes matroska track
>> CodecPrivate).
>>      
>
> I have to say that it is weird, since matroska demuxer should fire up
> before avformat...
> Do you have it (vlc --list) ?
>    
vlc --list does not show mkv.  I noticed when building that nothing in 
modules/demux/mkv was compiled. I didn't look into why that was because 
I thought it was intentional.  If there is a build option that enables 
it, then it needs to be fixed as well.  If you happen to know the build 
option, I can look into that.
>    
>> +            if (strncmp(p_sys->ic->iformat->name, "matroska", 8) == 0&&
>>      
> Coding style is not good, I would say...
>
>    
What is it about the coding style you don't like? Looking at the 
surrounding code, I would have to guess the spacing.  You prefer this?
if( strncmp( p_sys->ic->iformat->name, "matroska", 8 ) == 0 ...
>> +                cc->codec_id == CODEC_ID_DVD_SUBTITLE)
>> +            {
>> +                char *pos;
>> +
>> +                pos = strstr((char*)cc->extradata, "size:");
>>      
> Shouldn't it be (const char*)?
> Coding style too.
>
>    
strstr prototype is const char*, but it it perfectly valid to pass a 
char* to a const char*.  The const is only saying that strstr isn't 
going to modify the content.  It doesn't say that the content must be 
unmodifiable.

>> +                pos = strstr((char*)cc->extradata, "palette:");
>>      
> Same.
>
>    
>> +                    int j;
>> +                    for( j = 1; j<  17; j++ )
>>      
> VLC is C99, so you can declare j in the for(, just FYI.
>
>    
> Anyway, it seems to work.
>
>      Best Regards,
>
>    
Given the style issues, did you want me to fix it and resubmit?

John

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20090512/d0c5c3ea/attachment.html>