[FFmpeg-devel] [PATCH] a couple of updates for MPEGTS

[Måns Rullgård]

Nico Sabbi <Nicola.Sabbi at poste.it> writes:

> Il Tuesday 12 February 2008 23:36:59 Nico Sabbi ha scritto:
>> 
>> M?ns Rullg?rd wrote:
>> >> In incoming there are a couple of interesting samples: one is a trailer
>> >> of resident evil that uses 0x82 as stream_type to identify DCA
>> >> (I don't know in what system this mapping is defined) ;
>> 
>> >I'd obviously like to know what system uses this value.  The sample
>> >has a Bluray registration descriptor, so it maybe that is where it
>> >comes from.  The PS3 refuses to recognise any audio in the file, but
>> >that need not mean anything.
>> >
>> >Otherwise I have only the standard objection about non-standard
>> >features.  Randomly assigning codes in the private range to codecs
>> >will sooner or later lead to a clash.  In this case, the registration
>> >descriptor could probably be used to identify the codec mapping in a
>> >safer manner.  It's hard to say for sure without access to the Bluray
>> >spec.
>> >
>> >Given the amount of work required to implement this properly, and that
>> >we so far only have this sample using stream type 0x82, I reckon we
>> >can still add this to the list.
>> 
>> I'll post tomorrow the patch to parse the program descriptors.
>> let's make it better now and don't regret in the future
>
> Index: libavformat/mpegts.c
> ===================================================================
> --- libavformat/mpegts.c	(revisione 12129)
> +++ libavformat/mpegts.c	(copia locale)
> @@ -478,6 +478,7 @@
>      int desc_list_len, desc_len, desc_tag;
>      int comp_page = 0, anc_page = 0; /* initialize to kill warnings */
>      char language[4] = {0}; /* initialize to kill warnings */
> +    int has_hdmv_descr = 0;

I have an instinctive dislike for things like this.  They tend to
accumulate and become unmanageable.

>  #ifdef DEBUG_SI
>      av_log(ts->stream, AV_LOG_DEBUG, "PMT: len %i\n", section_len);
> @@ -505,7 +506,28 @@
>      program_info_length = get16(&p, p_end) & 0xfff;
>      if (program_info_length < 0)
>          return;
> -    p += program_info_length;
> +    while(program_info_length > 0) {
> +        uint8_t tag, len;
> +        tag = get8(&p, p_end);
> +        len = get8(&p, p_end);
> +        if(len < 0) {

This can never happen.  You (correctly) declared len as unsigned.

> +            //something is broken, exit the program_descriptors_loop
> +            p += program_info_length;
> +            break;
> +        }
> +        program_info_length -= len + 2;
> +        if(tag == 0x5 && len>=4) {

I'd prefer if that 5 were #defined with a descriptive name.

> +            uint8_t bytes[4];
> +            bytes[0] = get8(&p, p_end);
> +            bytes[1] = get8(&p, p_end);
> +            bytes[2] = get8(&p, p_end);
> +            bytes[3] = get8(&p, p_end);
> +            len -= 4;
> +            if(bytes[0] == 'H' && bytes[1] == 'D' && bytes[2] == 'M' && bytes[3] == 'V')
> +                has_hdmv_descr = 1;
> +        }
> +        if(len > 0) p += len;

Useless if() since len is unsigned.

> +    }
>      if (p >= p_end)
>          return;
>      for(;;) {
> @@ -571,6 +593,9 @@
>          }
>          p = desc_list_end;
>  
> +        if(stream_type == 0x82 && has_hdmv_descr)
> +            stream_type = STREAM_TYPE_AUDIO_DTS;

While this will of course work, I don't like it.  The stream_type
value should not be altered based on the registration (or other)
descriptor; only the interpretation should change.

-- 
M?ns Rullg?rd
mans at mansr.com