[vlc-devel] [PATCH 2/9] Support Media Enhancement Data (Events and Annotations) - compilation

[Ilkka Ollakka]

On Fri, Oct 04, 2013 at 02:20:09PM -0500, Roiy Shpaner wrote:
> Here is the second part: New MED module.
 
Hi,

Few comments inline

> From 69345f4f87681151ef1444263974758dbad1ad7b Mon Sep 17 00:00:00 2001
> From: Roiy <roiy at cs.umanitoba.ca>
> Date: Fri, 4 Oct 2013 12:32:42 -0500
> Subject: [PATCH 2/9] New Media Enhancement Data module
> 
> +/*****************************************************************************
> + * med.c: Demux for Media Enhacnement Data files (.med). 
> + *****************************************************************************
> + * Copyright (C) 1999-2013 VLC authors and VideoLAN

Maybe just 2013 as module hasn't been around in 1999?

> +struct demux_sys_t
> +{
> +    int         i_type;
> +    text_t      txt;
> +    es_out_id_t *es;
> +
> +    int64_t     i_next_demux_date;  //ROIY - needed?
> +    int64_t     i_microsecperframe;
> +
> +    int         i_event;    //Current med event
> +    int         i_events;   //Number of events
> +    seeksegment_t   *med_event;     //ROIY - remove this, access should be from input_control only
> +    int64_t     i_event_end;
> +
> +    int64_t     i_length;
> +};

You should remove stuff that you have commented that 'remove this' ?

> +#define NOTICEABLE_TIME_DIFF 2000000

This is nanoseconds?

> +/*****************************************************************************
> + * Module initializer
> + *****************************************************************************/
> +static int Open ( vlc_object_t *p_this )
> +{
> +
> +    p_demux->pf_demux = Demux;
> +    p_demux->pf_control = Control;

Set these at the end of Open function when everything is ready


> +    p_sys->i_microsecperframe = 40000;

This assumes 25fps at the start?

> +    msg_Dbg( p_demux, "Checking for med format" );
> +    for( i_try = 0; i_try < 256; i_try++ )      //ROIY - NEEDED?
> +    {

Why this loop?

> +        int i_dummy;
> +        char* p_dummy;
> +
> +        if( ( s = stream_ReadLine( p_demux->s ) ) == NULL )
> +        {
> +            msg_Err( p_demux, "Problem with med file format, expecting med header" );
> +            return VLC_EGENERIC;
> +        }
> +
> +        else if( !strncasecmp( s, "med", 3  ) )
> +        {
> +            msg_Dbg( p_demux, "Detected med file start" );
> +            free( s );
> +            break;
> +
> +            /*for( ; i_try < 256; i_try++ )   //ROIY REMOVE ALL THIS, it is not needed/used
> +            {

Clean up commented out code before submitting patch.

> +    
> +        free( s );*/
> +        }
> +    }

> +    /* Parse it */
> +    for( i_max = 0;; )
> +    {
> +        if( p_sys->i_events >= i_max )
> +        {
> +            i_max += 500;

500 increment is ok instea of doubling? How ofter this realloc usually happens?

> +            if( !( p_sys->med_event = realloc_or_free( p_sys->med_event,
> +                                              sizeof(seeksegment_t) * i_max ) ) )
> +            {

Use unlikely


> + *****************************************************************************/
> +static int Demux( demux_t *p_demux )
> +{
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +    int64_t i_maxdate, i_item_time;
> +    bool bAutoJump;
> +
> +    //ROIY - Fix this, demux should start checking if we are in annotations just when asked/event ended not always.
> +    if ( input_Control( p_demux->p_input, INPUT_GET_AUTO_JUMP, &bAutoJump)
> +        != VLC_SUCCESS )

use unlikely 

> +    if ( input_Control( p_demux->p_input, INPUT_GET_TIME, &i_item_time)
> +        != VLC_SUCCESS )

same here?

> +    seeksegment_t *p_med;
> +    if ( input_Control( p_demux->p_input, INPUT_GET_ANNOTATIONS, &p_med, NULL, (int64_t)-1, -1)
> +        != VLC_SUCCESS )
> +    {

and here

> +    {
> +        //msg_Dbg( p_demux, "Not currently in selected annotation segment");  ROIY - readd this when it is not constantly running?
> +        
> +        //if ( true )     //ROIY - skip flag
> +        //{

Clean up commented out code

> +            
> +        /*} ROIY - skip flag check not needed here right?
> +        else  
> +        {   // Don't jump, don't demux again (unless forced)
> +            int64_t nextTime;
> +            if ( input_Control( p_demux->p_input, INPUT_GET_LENGTH, &nextTime)
> +                != VLC_SUCCESS )
> +            {
> +                msg_Err( p_demux, "Failed to get length");
> +                return VLC_EGENERIC;
> +            }
> +
> +            p_sys->i_next_demux_date = nextTime;
> +        }*/

Same here

> +}
> +/* ROIY REMOVE, was backup

and here

> +/*****************************************************************************
> + * Fix: fix time stamp and order of med events
> + ROIY - fix event order
> + *****************************************************************************/
> +static void Fix( demux_t *p_demux )

Maybe more suitable function name could be in order, like FixEventOrder or something like that?


-- 
Ilkka Ollakka