[vlc-devel] "Re: [PATCH] Rewrites asx file support using vlc_xml API"
Jean-Baptiste Kempf
jb at videolan.org
Sun Oct 6 20:08:23 CEST 2013
On 30 Sep, Reka Inovan wrote :
> Ok, i have tried to correct the files according to your suggestions..i
> attach the corrected file.
>
> >> + free( (void*) s_init );
> >
> > Why not free directly?
>
> It will introduce warning during make process, so i cast it to suppress
> those warning. Is it a bad practice?
Yes, it is bad practice.
> #include <vlc_common.h>
> #include <vlc_demux.h>
> #include <vlc_xml.h>
> #include <vlc_strings.h>
>
> #include <ctype.h>
> #include <vlc_charset.h>
> #include "playlist.h"
> #include <vlc_meta.h>
Are all of those needed?
> struct demux_sys_t
> {
> };
Why declare it?
> /*****************************************************************************
> * Local prototypes
> *****************************************************************************/
> static int Demux( demux_t *p_demux);
>
> static mtime_t ParseTime(xml_reader_t *p_xml_reader)
> {
> char *psz_value = NULL;
> char *psz_start = NULL;
>
> const char *psz_node = NULL;
> const char *psz_txt = NULL;
>
> int i_subfractions = -1;
>
> int i_subresult = 0;
> mtime_t i_result = 0;
>
> do
> {
> psz_txt = xml_ReaderNextAttr( p_xml_reader, &psz_node );
> }
> while( strncasecmp( psz_txt, "VALUE", 5 ) );
When does it exit the loop ?
> psz_value = strdup( psz_node );
> psz_start = psz_value;
>
> while( *psz_value )
> {
> if( isdigit( *psz_value ) )
> {
> i_subresult = i_subresult * 10;
> i_subresult += *psz_value - '0';
> if( i_subfractions != -1 )
> i_subfractions++;
> }
> else if( *psz_value == ':' )
> {
> i_result += i_subresult;
> i_result = i_result * 60;
> i_subresult = 0;
> }
> else if( *psz_value == '.' )
> {
> i_subfractions = 0;
> i_result += i_subresult;
> i_subresult = 0;
> }
> psz_value++;
>
> }
> if( i_subfractions == -1)
> i_result += i_subresult;
>
> /* Convert to microseconds */
> if( i_subfractions == -1)
> i_subfractions = 0;
> while( i_subfractions < 6 )
> {
> i_subresult = i_subresult * 10;
> i_subfractions++;
> }
> i_result = i_result*1000000;
missing spaces around *
> if( i_subfractions != -1)
> i_result += i_subresult;
>
> free( (void*) psz_start );
free( psz_start) should work, really.
> return i_result;
> }
>
> static void ReadElement( xml_reader_t *p_xml_reader, const char **ppsz_txt )
> {
> const char *psz_node = NULL;
>
> /* Read the text node */
> xml_ReaderNextNode( p_xml_reader, &psz_node );
> free( (void*) *ppsz_txt );
Same here.
> *ppsz_txt = strdup( psz_node );
> resolve_xml_special_chars( (char*) *ppsz_txt );
Unneeded cast.
> /* Read the end element */
> xml_ReaderNextNode( p_xml_reader, &psz_node );
> /* TODO :
> * Currently we don't check the agreement of start and end element
> * This function is only used to read the element that cannot have child
> * according to the reference.
> */
How hard is it to fix this TODO?
> /*****************************************************************************
> * Import_ASX: main import function
> *****************************************************************************/
>
> int Import_ASX( vlc_object_t *p_this )
> {
> demux_t *p_demux = (demux_t *)p_this;
>
> if( demux_IsPathExtension( p_demux, ".asx" ) ||
> demux_IsPathExtension( p_demux, ".wax" ) ||
> demux_IsPathExtension( p_demux, ".wvx" ) ||
> demux_IsForced( p_demux, "asx-open" ) )
> {
> ;
> }
> else
> return VLC_EGENERIC;
You can invert the if and be more elegant, I think :)
> STANDARD_DEMUX_INIT_MSG( "found valid ASX playlist" );
> return VLC_SUCCESS;
> }
>
> /*****************************************************************************
> * Deactivate: frees unused data
> *****************************************************************************/
>
> void Close_ASX( vlc_object_t *p_this )
> {
> demux_t *p_demux = (demux_t *)p_this;
> demux_sys_t *p_sys = p_demux->p_sys;
>
> free( (void*) p_sys );
You don't use p_sys at all, this is not needed.
> do
> {
> i_type = xml_ReaderNextNode( p_xml_reader, &psz_node );
>
> if( i_type==XML_READER_STARTELEM )
> {
> /* Metadata Node */
> if( !strncasecmp( psz_node, "TITLE", 5 ) )
> ReadElement( p_xml_reader, &psz_title );
> if( !strncasecmp( psz_node, "AUTHOR", 6 ) )
> ReadElement( p_xml_reader, &psz_artist );
> if( !strncasecmp( psz_node, "COPYRIGHT", 9 ) )
> ReadElement( p_xml_reader, &psz_copyright );
> if( !strncasecmp( psz_node,"MOREINFO", 8 ) )
> {
> do
> {
> psz_txt = xml_ReaderNextAttr( p_xml_reader, &psz_node );
> }
> while(psz_txt && strncasecmp( psz_txt, "HREF", 4 ) );
>
> if( !psz_txt )
> ReadElement( p_xml_reader, &psz_moreinfo );
> else
> psz_moreinfo = strdup( psz_node );
> resolve_xml_special_chars( (char*) psz_node );
> }
> if( !strncasecmp( psz_node, "ABSTRACT", 8 ) )
> ReadElement( p_xml_reader, &psz_description );
> if( !strncasecmp( psz_node, "DURATION", 8 ) )
> i_duration = ParseTime( p_xml_reader );
> if( !strncasecmp( psz_node, "STARTTIME", 9 ) )
> i_start = ParseTime( p_xml_reader );
>
> /* Reference Node */
> /* All ref node will be converted into an entry */
> if( !strncasecmp( psz_node, "REF", 3 ) )
> {
> *pi_n_entry = *pi_n_entry +1;
>
> if( !psz_title )
> psz_title = input_item_GetTitle( p_current_input );
> if( !psz_artist )
> psz_artist = input_item_GetArtist( p_current_input );
> if( !psz_copyright )
> psz_copyright = input_item_GetCopyright( p_current_input );
> if( !psz_description )
> psz_description = input_item_GetDescription( p_current_input );
>
> do
> {
> psz_txt = xml_ReaderNextAttr( p_xml_reader, &psz_node );
> }
> while( strncasecmp( psz_txt, "HREF", 4) );
> psz_href = strdup( psz_node );
>
> if( asprintf( &psz_name, "%d. %s", *pi_n_entry, psz_title ) == -1)
> psz_name = strdup( psz_title );
> resolve_xml_special_chars( (char*) psz_href );
> psz_mrl = ProcessMRL( psz_href, psz_prefix );
>
> /* Add Time information */
> i_options = 0;
> if( i_start )
> {
> if( asprintf( ppsz_options, ":start-time=%d" ,(int) i_start/1000000 ) != -1)
> i_options++;
> }
> if( i_duration)
> {
> if( asprintf( ppsz_options+i_options, ":stop-time=%d",(int) (i_start+i_duration)/1000000 ) != -1)
> i_options++;
> }
>
> /* Create the input item */
> p_entry = input_item_NewExt( psz_mrl, psz_name, i_options,(const char* const*) ppsz_options, VLC_INPUT_OPTION_TRUSTED, i_duration );
> input_item_CopyOptions( p_current_input, p_entry );
>
> /* Add the metadata */
> if( psz_name )
> input_item_SetTitle( p_entry, psz_name );
> if( psz_artist )
> input_item_SetArtist( p_entry, psz_artist );
> if( psz_copyright )
> input_item_SetCopyright( p_entry, psz_copyright );
> if( psz_moreinfo )
> input_item_SetURL( p_entry, psz_moreinfo );
> if( psz_description )
> input_item_SetDescription( p_entry, psz_description );
> if( i_duration > 0)
> input_item_SetDuration( p_entry, i_duration );
>
> input_item_node_AppendItem( p_subitems, p_entry );
>
> while( i_options )
> free( ppsz_options[--i_options] );
> free( psz_name );
> free( psz_mrl );
> }
> }
> }
> while( i_type!=XML_READER_ENDELEM || strncasecmp( psz_node, "ENTRY", 5 ) );
>
> free( (void*) psz_href );
> free( (void*) psz_title );
> free( (void*) psz_artist );
> free( (void*) psz_copyright );
> free( (void*) psz_moreinfo );
> free( (void*) psz_description );
> }
>
> static int Demux( demux_t *p_demux )
> {
> const char *psz_node = NULL;
> const char *psz_txt = NULL;
> xml_reader_t *p_xml_reader = NULL;
>
> bool b_asx_valid = false;
> int i_type;
> int i_n_entry=0;
>
> const char *psz_base = FindPrefix( p_demux );
> const char *psz_title_asx = NULL;
> const char *psz_entryref = NULL;
>
> p_xml_reader = xml_ReaderCreate( p_demux, p_demux->s );
> if( !p_xml_reader )
> {
> msg_Err( p_demux, "Cannot parse ASX input file as XML");
> goto error;
> }
>
> input_item_t *p_current_input = GetCurrentItem( p_demux );
> input_item_node_t *p_subitems = input_item_node_Create( p_current_input );
>
> do
> {
> i_type = xml_ReaderNextNode( p_xml_reader, &psz_node );
> if( i_type == XML_READER_STARTELEM )
> {
> if( !b_asx_valid )
> {
> if(!strncasecmp( psz_node, "ASX", 3 ) )
> b_asx_valid = true;
> else
> {
> msg_Err( p_demux, "invalid root node" );
> goto error;
> }
> }
>
> /* Metadata Node Handler */
> if( !strncasecmp( psz_node, "TITLE", 5 ) )
> {
> ReadElement( p_xml_reader, &psz_title_asx );
> input_item_SetTitle( p_current_input, psz_title_asx );
> }
> if( !strncasecmp( psz_node, "AUTHOR", 6 ) )
> {
> ReadElement( p_xml_reader, &psz_txt );
> input_item_SetArtist( p_current_input, psz_txt );
> }
> if( !strncasecmp( psz_node, "COPYRIGHT", 9 ) )
> {
> ReadElement( p_xml_reader, &psz_txt );
> input_item_SetCopyright( p_current_input, psz_txt );
> }
> if( !strncasecmp( psz_node, "MOREINFO", 8 ) )
> {
> do
> {
> psz_txt = xml_ReaderNextAttr( p_xml_reader, &psz_node );
> }
> while(psz_txt && strncasecmp( psz_txt, "HREF", 4 ) );
>
> if( !psz_txt ) // If HREF attribute doesn't exist
> ReadElement( p_xml_reader, &psz_txt );
> else
> psz_txt = strdup( psz_node );
>
> resolve_xml_special_chars( (char*) psz_txt );
> input_item_SetURL( p_current_input, psz_txt );
> }
> if( !strncasecmp( psz_node, "ABSTRACT", 8 ) )
> {
> ReadElement( p_xml_reader, &psz_txt );
> input_item_SetDescription( p_current_input, psz_txt );
> }
>
> /* Base Node handler */
> if( !strncasecmp( psz_node, "BASE", 4 ) )
> ReadElement( p_xml_reader, &psz_base );
>
> /* Entry Ref Handler */
> if( !strncasecmp( psz_node, "ENTRYREF", 7 ) )
> {
> do
> {
> psz_txt = xml_ReaderNextAttr( p_xml_reader, &psz_node );
> }
> while( !strncasecmp( psz_txt, "HREF", 4 ) );
>
> /* Create new input item */
> input_item_t *p_input;
> resolve_xml_special_chars( (char*) psz_node );
> p_input = input_item_New( psz_node, psz_title_asx );
> input_item_CopyOptions( p_current_input, p_input );
> input_item_node_AppendItem( p_subitems, p_input );
>
> vlc_gc_decref( p_input );
> }
>
> /* Entry Handler */
> if( !strncasecmp( psz_node, "ENTRY", 5 ) )
> {
> ProcessEntry( &i_n_entry, p_xml_reader, p_subitems, p_current_input, (char*) psz_base);
> }
> /* FIXME Unsupported elements
> PARAM
> EVENT
> REPEAT
> ENDMARK
> STARTMARK
> */
> }
> }
> while( i_type!=XML_READER_ENDELEM || strncasecmp( psz_node, "ASX", 3 ) );
>
> free( (char*) psz_base );
> free( (char*) psz_title_asx );
> free( (char*) psz_entryref );
> free( (char*) psz_txt );
>
> input_item_node_PostAndDelete( p_subitems );
> vlc_gc_decref( p_current_input );
>
> error:
> if( p_xml_reader)
> xml_ReaderDelete( p_xml_reader );
>
> return 0;
> }
The rest looks good.
--
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device
More information about the vlc-devel
mailing list