[vlc-devel] [PATCH] Handling prefered language in AsfSelectStream Better, bitrate sort and select algo Tested with asf containing 3, Videos and 24 Lang...
Denis Charmet
typx at dinauz.org
Thu Jul 18 02:28:00 CEST 2013
Hi,
I've lost my history of this so sorry if I repeat something from others.
Le mardi 16 juillet 2013 à 04:51:20, Alain Degreffe a écrit :
> @@ -167,6 +177,48 @@ void asf_HeaderParse ( asf_header_t *hdr,
> + i_count = var_buffer_get16( &buffer );
> +
> + for( i_current_id = 0 ; i_current_id < i_count ; i_current_id++ )
> + {
> + i_langid_len = var_buffer_get8( &buffer );
> + i_lang_str_len = i_langid_len / 2;
> +
> + psz_lang_str = malloc( i_lang_str_len );
> + pi_ws_lang_str = malloc( i_langid_len );
> +
> + if( !psz_lang_str || !pi_ws_lang_str )
> + {
> + /* Malloc problem - Zap the remaining bytes of object */
> + var_buffer_getmemory( &buffer, NULL, i_langid_len );
> + free( psz_lang_str );
> + free( pi_ws_lang_str );
> + continue ;
> + }
> +
> + var_buffer_getmemory( &buffer, pi_ws_lang_str , i_langid_len );
> + asf_wstr2str( psz_lang_str, pi_ws_lang_str,i_langid_len );
> +
> + hdr->ppsz_tablang[i_current_id] = malloc( i_lang_str_len );
> + if( hdr->ppsz_tablang[i_current_id] )
> + {
> + memcpy( hdr->ppsz_tablang[i_current_id],
> + psz_lang_str, i_lang_str_len );
> + }
What's the point of that? You already allocated and filled the buffer,
don't allocate a second one to copy and then delete the first one.
> + free( psz_lang_str );
> + free( pi_ws_lang_str );
> + }
> +
> +void asf_ClearCandidate ( asf_candidate_t *cell )
> +{
> + asf_candidate_t * tmp;
> + while( cell->next )
> + {
> + tmp = cell;
> + cell = tmp->next;
> + free(tmp);
> + }
> + free(cell);
> +}
Why not while(cell) {...}?
> +asf_candidate_t * asf_AddCandidate( asf_candidate_t *work_Candidate,
> + int stream_candidate_id )
> +{
> + work_Candidate->value = stream_candidate_id;
> + work_Candidate->next = malloc( sizeof( asf_candidate_t ) );
> + if( !work_Candidate->next ) return work_Candidate;
> + work_Candidate = work_Candidate->next;
> + work_Candidate->next = NULL;
> + return work_Candidate;
> +}
Wouldn't it be easier to just allocate the element and then make it the
first element of the list?
{
asf_candidate_t * cd = malloc( sizeof( asf_candidate_t ) );
if( !cd ) return work_Candidate;
cd->next = work_Candidate;
cd->stream_candidate_id;
return cd;
}
> +
> +void asf_StreamSelect ( asf_header_t *hdr,
> + int i_bitrate_max,
> + bool b_all,
> + bool b_audio,
> + bool b_video,
> + const char * psz_lang )
> {
[...]
> + i_audio = -1;
> + i_video = -1;
> + i_bitrate_total = 0;
> + i_bitrate_var = 0;
> +
> + asf_candidate_t * head_Candidatefavlang, * courant_Candidatefavlang;
> + asf_candidate_t * head_Candidateotherlang, * courant_Candidateotherlang;
> + asf_candidate_t * head_CandidateVideo, * courant_CandidateVideo;
> + asf_candidate_t * work_Candidate;
> +
> + head_Candidatefavlang = malloc( sizeof( asf_candidate_t ) );
> + head_Candidateotherlang = malloc( sizeof( asf_candidate_t ) );
> + head_CandidateVideo = malloc( sizeof( asf_candidate_t ) );
> +
> + if( !head_Candidatefavlang || !head_Candidatefavlang ||
> + !head_Candidatefavlang )
> + {
> + free( head_Candidatefavlang );
> + free( head_Candidatefavlang );
> + free( head_Candidatefavlang );
> + return;
> + }
Using the previous comment you could just call
asf_AddCandidate(NULL,id) instead of all this.
> +
> + head_Candidatefavlang->next = NULL;
> + courant_Candidatefavlang = head_Candidatefavlang;
> +
> + head_Candidateotherlang->next = NULL;
> + courant_Candidateotherlang = head_Candidateotherlang;
> +
> + head_CandidateVideo->next = NULL;
> + courant_CandidateVideo = head_CandidateVideo;
> +
> + for( i = 1 ; i < 128 ; i++ )
> {
> if( hdr->stream[i].i_cat == ASF_CODEC_TYPE_UNKNOWN )
> {
> continue;
> }
> - else if( hdr->stream[i].i_cat == ASF_CODEC_TYPE_AUDIO && b_audio &&
> - ( i_audio <= 0 ||
> - ( ( ( hdr->stream[i].i_bitrate > hdr->stream[i_audio].i_bitrate &&
> - ( i_bitrate_total + hdr->stream[i].i_bitrate - hdr->stream[i_audio].i_bitrate
> - < i_bitrate_max || !i_bitrate_max) ) ||
> - ( hdr->stream[i].i_bitrate < hdr->stream[i_audio].i_bitrate &&
> - i_bitrate_max != 0 && i_bitrate_total > i_bitrate_max )
> - ) ) ) )
> +
> + /* audio */
> + if( hdr->stream[i].i_cat == ASF_CODEC_TYPE_AUDIO && b_audio &&
> + ( hdr->stream[i].i_bitrate < i_bitrate_max || !i_bitrate_max ) )
> {
> - /* unselect old stream */
> - if( i_audio > 0 )
> + if( psz_lang != NULL &&
> + strcmp( hdr->ppsz_tablang[hdr->stream[i].i_lang_id], psz_lang ) == 0 )
Beware... i_lang_id is a 16 bits integer that comes from a file, what if
the file is corrupted/malicious and i_lang_id > 128?
> {
> - hdr->stream[i_audio].i_selected = 0;
> - if( hdr->stream[i_audio].i_bitrate> 0 )
> - {
> - i_bitrate_total -= hdr->stream[i_audio].i_bitrate;
> - }
> + courant_Candidatefavlang = asf_AddCandidate(courant_Candidatefavlang, i);
Franglais :)
> }
> -
> - hdr->stream[i].i_selected = 1;
> - if( hdr->stream[i].i_bitrate> 0 )
> + else
> {
> - i_bitrate_total += hdr->stream[i].i_bitrate;
> + courant_Candidateotherlang = asf_AddCandidate(courant_Candidateotherlang, i);
> }
> - i_audio = i;
> }
> + /* video */
> else if( hdr->stream[i].i_cat == ASF_CODEC_TYPE_VIDEO && b_video &&
> - ( i_video <= 0 ||
> - (
> - ( ( hdr->stream[i].i_bitrate > hdr->stream[i_video].i_bitrate &&
> - ( i_bitrate_total + hdr->stream[i].i_bitrate - hdr->stream[i_video].i_bitrate
> - < i_bitrate_max || !i_bitrate_max) ) ||
> - ( hdr->stream[i].i_bitrate < hdr->stream[i_video].i_bitrate &&
> - i_bitrate_max != 0 && i_bitrate_total > i_bitrate_max )
> - ) ) ) )
> + ( hdr->stream[i].i_bitrate < i_bitrate_max || !i_bitrate_max ) )
> {
> - /* unselect old stream */
> - if( i_video > 0 )
> - {
> - hdr->stream[i_video].i_selected = 0;
> - if( hdr->stream[i_video].i_bitrate> 0 )
> - {
> - i_bitrate_total -= hdr->stream[i_video].i_bitrate;
> - }
> - }
> + courant_CandidateVideo = asf_AddCandidate(courant_CandidateVideo, i);
> + }
> + }
> +
> + if( head_Candidatefavlang->next )
> + {
> + work_Candidate = head_Candidatefavlang;
> + }
> + else
> + {
> + work_Candidate = head_Candidateotherlang;
> + }
> +
> + courant_CandidateVideo = head_CandidateVideo;
>
> - hdr->stream[i].i_selected = 1;
> - if( hdr->stream[i].i_bitrate> 0 )
> + while( work_Candidate->next )
> + {
The following seems fishy... since you never reset
courant_CandidateVideo what's the point of putting it inside the audio
loop because in the other iteration courant_CandidateVideo->next will
end up being NULL.
> + while( courant_CandidateVideo->next )
> + {
> + i_bitrate_var = hdr->stream[work_Candidate->value].i_bitrate
> + + hdr->stream[courant_CandidateVideo->value].i_bitrate;
> + if( i_bitrate_var > i_bitrate_total &&
> + ( !i_bitrate_max || i_bitrate_var < i_bitrate_max ) )
> {
> - i_bitrate_total += hdr->stream[i].i_bitrate;
> + i_audio = work_Candidate->value;
> + i_video = courant_CandidateVideo->value;
> + i_bitrate_total = i_bitrate_var;
> }
> - i_video = i;
> + courant_CandidateVideo = courant_CandidateVideo->next;
> }
>
> + i_bitrate_var = hdr->stream[work_Candidate->value].i_bitrate;
if the bitrate audio is greater than the latest bitrate audio + video
bitrate? Don't think it's going to happen :)
While I understand what you are trying to achieve I think it's buggy.
> + if( i_bitrate_var > i_bitrate_total &&
> + ( !i_bitrate_max || i_bitrate_var < i_bitrate_max ) )
> + {
> + i_audio = work_Candidate->value;
> + i_bitrate_total = i_bitrate_var;
> + }
> + work_Candidate = work_Candidate->next;
> }
> +
> + /* select stream */
> + if( i_audio != -1 ) hdr->stream[i_audio].i_selected = 1;
> + if( i_video != -1 ) hdr->stream[i_video].i_selected = 1;
> +
> + /* clear structures */
> + asf_ClearCandidate( head_Candidatefavlang );
> + asf_ClearCandidate( head_Candidateotherlang );
> + asf_ClearCandidate( head_CandidateVideo );
> }
> diff --git a/modules/access/mms/asf.h b/modules/access/mms/asf.h
> index f15b90d..a7d0725 100644
> --- a/modules/access/mms/asf.h
> +++ b/modules/access/mms/asf.h
> @@ -42,6 +42,7 @@ typedef struct
> int i_cat; /* ASF_CODEC_TYPE_VIDEO, ASF_CODEC_TYPE_AUDIO, */
> int i_bitrate; /* -1 if unknown */
> int i_selected;
> + int i_lang_id;
> } asf_stream_t;
>
> typedef struct
> @@ -49,16 +50,31 @@ typedef struct
> int64_t i_file_size;
> int64_t i_data_packets_count;
> int32_t i_min_data_packet_size;
> + char * ppsz_tablang[128];
Where is it free'd?
>
> asf_stream_t stream[128];
>
> } asf_header_t;
>
I'd like to say check with FromWide but I fear it's WIN32 only.
> +static inline void asf_wstr2str( char * dst, uint8_t * src , int inlen )
> +{
> + for( int i = 0 ; i < inlen ; i += 2 )
> + {
> + dst[i/2] = GetWLE( &src[i] );
> + }
> +};
> #endif
> diff --git a/modules/access/mms/mmsh.c b/modules/access/mms/mmsh.c
> index 2a36670..5a0743f 100644
> --- a/modules/access/mms/mmsh.c
> +++ b/modules/access/mms/mmsh.c
> @@ -440,6 +440,7 @@ static int Reset( access_t *p_access )
> access_sys_t *p_sys = p_access->p_sys;
> asf_header_t old_asfh = p_sys->asfh;
> int i;
> + char * psz_preflang;
>
> msg_Dbg( p_access, "Reset the stream" );
> p_sys->i_start = p_access->info.i_pos;
> @@ -461,11 +462,14 @@ static int Reset( access_t *p_access )
> p_sys->asfh.i_data_packets_count,
> p_sys->asfh.i_min_data_packet_size );
>
> + psz_preflang = var_InheritString( p_access->p_parent, "audio-language" );
> asf_StreamSelect( &p_sys->asfh,
> var_InheritInteger( p_access, "mms-maxbitrate" ),
> var_InheritBool( p_access, "mms-all" ),
> var_InheritBool( p_access, "audio" ),
> - var_InheritBool( p_access, "video" ) );
> + var_InheritBool( p_access, "video" ),
> + psz_preflang );
> + free (psz_preflang);
>
> /* Check we have comptible asfh */
> for( i = 1; i < 128; i++ )
> @@ -552,6 +556,7 @@ static int Describe( access_t *p_access, char **ppsz_location )
> bool b_keepalive = false;
> char *psz;
> int i_code;
> + char *psz_preflang;
>
> /* Reinit context */
> p_sys->b_broadcast = true;
> @@ -714,11 +719,14 @@ static int Describe( access_t *p_access, char **ppsz_location )
> if( p_sys->asfh.i_min_data_packet_size <= 0 )
> goto error;
>
> + psz_preflang = var_InheritString( p_access->p_parent, "audio-language" );
> asf_StreamSelect( &p_sys->asfh,
> var_InheritInteger( p_access, "mms-maxbitrate" ),
> var_InheritBool( p_access, "mms-all" ),
> var_InheritBool( p_access, "audio" ),
> - var_InheritBool( p_access, "video" ) );
> + var_InheritBool( p_access, "video" ),
> + psz_preflang );
> + free( psz_preflang );
> return VLC_SUCCESS;
>
> error:
> diff --git a/modules/access/mms/mmstu.c b/modules/access/mms/mmstu.c
> index 932c59f..dbb3fe5 100644
> --- a/modules/access/mms/mmstu.c
> +++ b/modules/access/mms/mmstu.c
> @@ -467,6 +467,7 @@ static int MMSOpen( access_t *p_access, vlc_url_t *p_url, int i_proto )
> int i_streams;
> int i_first;
> char *mediapath;
> + char *psz_preflang;
>
>
> /* *** Open a TCP connection with server *** */
> @@ -788,11 +789,14 @@ static int MMSOpen( access_t *p_access, vlc_url_t *p_url, int i_proto )
> * and bitrate mutual exclusion(optional) */
> asf_HeaderParse ( &p_sys->asfh,
> p_sys->p_header, p_sys->i_header );
> + psz_preflang = var_InheritString( p_access->p_parent, "audio-language" );
This is a list of language separated by commas, which means if someone
has "en,de,jp" you won't select the english track but the one with the
biggest bitrate regardless of the language.
At least if you don't have that, please make sure that you keep the
"legacy" behaviour.
> asf_StreamSelect( &p_sys->asfh,
> var_InheritInteger( p_access, "mms-maxbitrate" ),
> var_InheritBool( p_access, "mms-all" ),
> var_InheritBool( p_access, "audio" ),
> - var_InheritBool( p_access, "video" ) );
> + var_InheritBool( p_access, "video" ),
> + psz_preflang );
> + free( psz_preflang );
>
> /* *** now select stream we want to receive *** */
> /* TODO take care of stream bitrate TODO */
> --
Let's be honest, I understand Rémi's reluctance to this patch. It gives
the access "powers" and responsibility it shouldn't have.
With this does the demux know about the other audio tracks? If it
doesn't, it means that the user has no way to know there are other
options.
Imho, stream selection is the responsibility of the core and the user,
That's why I think that your use case needs a core evolution instead of a workaround. I'll try to think of a longer-lasting solution.
Regards,
--
Denis Charmet - TypX
Le mauvais esprit est un art de vivre
More information about the vlc-devel
mailing list