[vlc-devel] [PATCH] Handling prefered language in AsfSelectStream Better, bitrate sort and select algo Tested with asf containing 3, Videos and 24 Lang...
Alain Degreffe
eczema at ecze.com
Thu Jul 18 07:00:13 CEST 2013
Le 18/07/13 02:28, Denis Charmet a écrit :
Thanks a lot for your review.
> 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.
I'll replace the malloc/strcpy by just:
hdr->ppsz_tablang[i_current_id] = strndup ( psz_lang_str, i_lang_str_len );
but I will try also to modify the static inline to directly use hdr->ppsz_tablang
Excellebt idea..
>> + 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;
> }
Ooh yes in the reverse order no need to keep a head, thx for this
sample... :-)
I suppose the line cd->stream_candidate_id will be in fact:
cd->value = stream_candidate_id;
>
>> +
>> +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.
Yes nice
>> +
>> + 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?
Yep already seen but the last patch is not submitted so this control is
not already here....
>> {
>> - 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 :)
:-) s/courant/current/g But with your comment, head and "courant" will
disappear....
>> }
>> -
>> - 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.
Well seen, I'll reconsider this algo....
>
>> + 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?
In the "not submitted" patch, I purpose to add at end of AsfStreamSelect:
for( int i = 0 ; i < 128 ; i++ )
free ( hdr->ppsz_tablang[i] ) ;
>>
>> asf_stream_t stream[128];
>>
>> } asf_header_t;
>>
>
> I'd like to say check with FromWide but I fear it's WIN32 only.
The asf spec say: lang is in unicode 16 bytes long character.
The only fear is the endian matter.... so GetWLE is here to swap the
bytes if necessary..
Am I right ?
>
>> +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.
Well this will add some tricky code to AsfStreamSelect but I'll take
care of multiple prefs lang
I haven't really see a policy to choose a stream in the asf specs... All
we have are objects of exclusion type and priority.... Se we must
compose and choose what we think is the best way to choose a stream....
Many use case in perspective.....
>
>> 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.
That's mandatory for bandwidth consumption... And this is under his
responsibility if I well observe how WMP works...
> 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.
Demux will continue to have all track information. Asf spec show us that
all informations are sent in the beginning. The mms server will just
remove stream object no filtered by user.
This is the design of Msoft: deliver ASF with all informations with a
filtering capability...
So I need help to understand how demux willl know that the user change
video or audio track and how to trigger the access/mms to make a new
filtered request with the new streams chosen by the user....
> 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.
Hope to see your future comment....
I will continue to work on it if we are able to find a common point of
view...
We should just keep in mind that ASF is especially done for access
filtering....
> Regards,
>
Thanks a lot.
Alain
More information about the vlc-devel
mailing list