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

[Måns Rullgård]

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

> Il Sunday 17 February 2008 15:49:05 M?ns Rullg?rd ha scritto:
>> 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
>> >

[...]

>> > +        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.
>> 
>
> updated with some more security check on "len", but I didn't
> find a way to handle differently the presence of the hdmv.
> Please, explain
>
> Index: libavformat/mpegts.c
> ===================================================================
> --- libavformat/mpegts.c	(revisione 12129)
> +++ libavformat/mpegts.c	(copia locale)
> @@ -31,6 +31,7 @@
>  /* maximum size in which we look for synchronisation if
>     synchronisation is lost */
>  #define MAX_RESYNC_SIZE 4096
> +#define REGISTRATION_DESCRIPTOR 5
>  
>  typedef struct PESContext PESContext;
>  
> @@ -478,6 +479,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;
>  
>  #ifdef DEBUG_SI
>      av_log(ts->stream, AV_LOG_DEBUG, "PMT: len %i\n", section_len);
> @@ -505,7 +507,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);

This breaks if program_info_length == 1.  Yes, that is invalid, but we
must assume that it might happen.

> +        if(len == -1 || len > program_info_length-2) {
> +            //something is broken, exit the program_descriptors_loop
> +            p += program_info_length;
> +            break;
> +        }
> +        program_info_length -= len + 2;
> +        if(tag == REGISTRATION_DESCRIPTOR && len>=4) {
> +            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;
> +        }
> +        p += len;
> +    }
>      if (p >= p_end)
>          return;
>      for(;;) {
> @@ -588,7 +611,10 @@
>          case STREAM_TYPE_AUDIO_AAC:
>          case STREAM_TYPE_AUDIO_AC3:
>          case STREAM_TYPE_AUDIO_DTS:
> +        case STREAM_TYPE_AUDIO_DTS2:
>          case STREAM_TYPE_SUBTITLE_DVB:
> +            if(stream_type == STREAM_TYPE_AUDIO_DTS2 && !has_hdmv_descr)
> +                break;
>              if(ts->pids[pid] && ts->pids[pid]->type == MPEGTS_PES){
>                  pes= ts->pids[pid]->u.pes_filter.opaque;
>                  st= pes->st;

This could be done with less code by reordering the cases and using a
fall-through.  That will of course not extend very well, though.

> @@ -923,6 +949,7 @@
>          codec_id = CODEC_ID_AC3;
>          break;
>      case STREAM_TYPE_AUDIO_DTS:
> +    case STREAM_TYPE_AUDIO_DTS2:
>          codec_type = CODEC_TYPE_AUDIO;
>          codec_id = CODEC_ID_DTS;
>          break;
> Index: libavformat/mpegts.h
> ===================================================================
> --- libavformat/mpegts.h	(revisione 12129)
> +++ libavformat/mpegts.h	(copia locale)
> @@ -55,6 +55,7 @@
>  
>  #define STREAM_TYPE_AUDIO_AC3       0x81
>  #define STREAM_TYPE_AUDIO_DTS       0x8a
> +#define STREAM_TYPE_AUDIO_DTS2      0x82

I'd prefer a name such as STREAM_TYPE_AUDIO_HDMV_DTS, so it is clear
where this comes from.  The values from the private range already
there should also be renamed in a similar fashion.

Apart from the points above, I am OK with this patch, but do keep on
reading.

I don't like the way the mpegts demuxer is written.  It is difficult
to support various format extensions in a tidy manner, as we have
experienced on repeated occasions.  Because of this, I've been working
on improving the tcvp mpegts demuxer in areas it has been lacking,
with the intent of eventually having it replace the current lavf
demuxer.  It's been rather slow going, mostly because I have no direct
interest in the mpegts format as I used in my previous job.

Until I have a good replacement ready, I am willing to accept
reasonable patches to lavf, in the interest of supporting as many
files as possible.  I will not, however, accept anything that adds
significantly to the messy nature of the present lavf ts demuxer.

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