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

[Måns Rullgård]

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

>> M?ns Rullg?rd ha scritto:
>
>>> @@ -505,7 +507,28 @@
>>>      program_info_length = get16(&p, p_end) & 0xfff;
>>>      if (program_info_length < 0)
>>>> -    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.
>
> fixed
>
>>> 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.
>
>>done,
>>
>>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.
>
> final version attached
>
> 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,33 @@
>      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;
> +        if(program_info_length < 2) {
> +            //something is broken, exit the program_descriptors_loop
> +            p += program_info_length;

You could move p += program_info_length statements (this one and the
one below) just after the while loop.  Adding zero will do no harm.

> +            break;
> +        }
> +        tag = get8(&p, p_end);
> +        len = get8(&p, p_end);
> +        if(len == -1 || len > program_info_length - 2) {

You need to declare len as int to compare it to -1.  Like this, a
return value of -1 will turn into a seemingly valid 255.

> +            //something else 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 +616,10 @@
>          case STREAM_TYPE_AUDIO_AAC:
>          case STREAM_TYPE_AUDIO_AC3:
>          case STREAM_TYPE_AUDIO_DTS:
> +        case STREAM_TYPE_AUDIO_HDMV_DTS:
>          case STREAM_TYPE_SUBTITLE_DVB:
> +            if(stream_type == STREAM_TYPE_AUDIO_HDMV_DTS && !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;

As I've said, I don't like the way this looks, neither before nor
after the patch.  Unfortunately, I can't think of an easy way of
cleaning it up, and I'm working on a better ts demuxer anyway.

> @@ -923,6 +954,7 @@
>          codec_id = CODEC_ID_AC3;
>          break;
>      case STREAM_TYPE_AUDIO_DTS:
> +    case STREAM_TYPE_AUDIO_HDMV_DTS:
>          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_HDMV_DTS  0x82
>  
>  #define STREAM_TYPE_SUBTITLE_DVB    0x100
>  

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