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

[Laurent Aimar]

Hi,

>          case CODEC_TYPE_SUBTITLE:
>              es_format_Init( &fmt, SPU_ES, fcc );
> +            if( strncmp( p_sys->ic->iformat->name, "matroska", 8 ) == 0 &&
> +                cc->codec_id == CODEC_ID_DVD_SUBTITLE &&
> +                cc->extradata_size > 0 )
> +            {
> +                char *p_start;
> +                char *p_buf = (char *)malloc( cc->extradata_size + 1);
 No need to cast malloc.
 In VLC we always check for malloc failure (and the extra data can be very
large with a broken file).

> +                memcpy( p_buf, cc->extradata , cc->extradata_size );
> +                p_buf[cc->extradata_size] = '\0';
> +
> +                p_start = strstr( p_buf, "size:" );
> +                if( p_start && sscanf( p_start, "size: %dx%d",
> +                    &fmt.subs.spu.i_original_frame_width, 
> +                    &fmt.subs.spu.i_original_frame_height ) == 2 )
> +                {
> +                    msg_Dbg( p_demux, "original frame size: %dx%d", 
> +                             fmt.subs.spu.i_original_frame_width, 
> +                             fmt.subs.spu.i_original_frame_height );
> +                }
> +                else
> +                {
> +                    msg_Warn( p_demux, "reading original frame size failed" );
> +                }
> +
> +                p_start = strstr( p_buf, "palette:" );
> +                if( p_start && sscanf( p_start,
> +                            "palette: %x, %x, %x, %x, %x, %x, %x, %x, "
> +                                     "%x, %x, %x, %x, %x, %x, %x, %x",
> +                        &fmt.subs.spu.palette[1], 
> +                        &fmt.subs.spu.palette[2], &fmt.subs.spu.palette[3],
> +                        &fmt.subs.spu.palette[4], &fmt.subs.spu.palette[5], 
> +                        &fmt.subs.spu.palette[6], &fmt.subs.spu.palette[7],
> +                        &fmt.subs.spu.palette[8], &fmt.subs.spu.palette[9], 
> +                        &fmt.subs.spu.palette[10], &fmt.subs.spu.palette[11],
> +                        &fmt.subs.spu.palette[12], &fmt.subs.spu.palette[13], 
> +                        &fmt.subs.spu.palette[14], &fmt.subs.spu.palette[15],
> +                        &fmt.subs.spu.palette[16] ) == 16 )
> +                {
> +                    int j;
> +                    for( j = 1; j < 17; j++ )
> +                    {
> +                        uint8_t r = 0, g = 0, b = 0;
> +                        uint8_t y = 0, u = 0, v = 0;
 Useless assignation to zero (even if it was in vobsub.c ;).
> +                        r = (fmt.subs.spu.palette[j] >> 16) & 0xff;
> +                        g = (fmt.subs.spu.palette[j] >> 8) & 0xff;
> +                        b = (fmt.subs.spu.palette[j] >> 0) & 0xff;
> +                        y = (uint8_t) __MIN(abs(r * 2104 + g * 4130 + b * 802 + 4096 + 131072) >> 13, 235);
> +                        u = (uint8_t) __MIN(abs(r * -1214 + g * -2384 + b * 3598 + 4096 + 1048576) >> 13, 240);
> +                        v = (uint8_t) __MIN(abs(r * 3598 + g * -3013 + b * -585 + 4096 + 1048576) >> 13, 240);
> +                        fmt.subs.spu.palette[j] = 0;
> +                        fmt.subs.spu.palette[j] |= (y&0xff)<<16;
> +                        fmt.subs.spu.palette[j] |= (u&0xff);
> +                        fmt.subs.spu.palette[j] |= (v&0xff)<<8;
 You could also directly compute the 24 bits value (easier to read I think).
> +
> +                    }
> +                    fmt.subs.spu.palette[0] =  0xBeef; 
> +                    msg_Dbg( p_demux, "vobsub palette read" );
> +                }
> +                else
> +                {
> +                    msg_Warn( p_demux, "reading original palette failed" );
> +                }
 I would really prefer if this code (the palette parsing) is moved to a
modules/demux/vobsub.h (an inline function is perfect) and so be reused
by avformat, matroska and vobsub.c.
 Duplicating code is always a bad idea in the long term.

 Other than that, it seems fine, thanks for your work.

-- 
fenrir