[FFmpeg-devel] [PATCH] Fix empty G-VOP header decoding in MPEG-4

[Måns Rullgård]

Anatoly Nenashev <anatoly.nenashev at ovsoft.ru> writes:

> On 10.02.2011 01:32, M?ns Rullg?rd wrote:
>> Anatoly Nenashev<anatoly.nenashev at ovsoft.ru>  writes:
>>
>>    
>>> On 08.02.2011 22:00, M?ns Rullg?rd wrote:
>>>      
>>>> Anatoly Nenashev<anatoly.nenashev at ovsoft.ru>   writes:
>>>>
>>>>
>>>>        
>>>>> Hi all!
>>>>> There are some cameras which send mpeg-4 streams with empty G-VOP header.
>>>>> This part of stream looks like this:
>>>>> ... 00 00 01 B3 00 00 00 01 B6 ...
>>>>> Sample file uploaded in issue 2592.
>>>>> FFmpeg reports "header damaged" and ignores first I-frame in G-VOP.
>>>>> Attached patch fix this problem.
>>>>>
>>>>> Anatoly.
>>>>>
>>>>>          
>>>   [...]
>>>      
>>>> Looking at this, I wonder why this is there at all.  The s->time_base
>>>> field is only used to incorrectly set the pts field of the decoded
>>>> frame (now I understand why those values always are wrong).  The
>>>> actual PTS is passed from the input packet to the pkt_pts field of the
>>>> output frame.
>>>>
>>>> IMO this nonsense should be removed.  The time_code field in the
>>>> bitstream has nothing to do with PTS.
>>>>
>>>>
>>>>        
>>> Hmm... So, if I understand you clearly, we just can ignore GOP header
>>> as done in attached patch.
>>>      
>> The value seems to also be used in a convoluted calculation of direct
>> mode MVs.  This calculation doesn't actually need the time_code value,
>> only the distance (in frames) between reference frames and the predicted
>> frame.  I'd be careful messing with it.
>>
>> The part setting the PTS of the output frame should of course be
>> removed.  It is clearly wrong.
>>
>>    
> I've changed  original patch to carefully check that time code
> obtained from GOP header is monotone.
> Also PTS setting for output frame is removed. See attachment.
>
> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> index 5303da3..02e7853 100644
> --- a/libavcodec/mpeg4videodec.c
> +++ b/libavcodec/mpeg4videodec.c
> @@ -1494,16 +1494,19 @@ end:
>
>  static int mpeg4_decode_gop_header(MpegEncContext * s, GetBitContext *gb){
>      int hours, minutes, seconds;
> +    int val, time_base;
>
> -    hours= get_bits(gb, 5);
> -    minutes= get_bits(gb, 6);
> -    skip_bits1(gb);
> -    seconds= get_bits(gb, 6);
> +    val= show_bits(gb, 18);
> +    hours= (val >> 13) & 0x1F;
> +    minutes= (val >> 7) & 0x3F;
> +    seconds= val & 0x3F;
>
> -    s->time_base= seconds + 60*(minutes + 60*hours);
> -
> -    skip_bits1(gb);
> -    skip_bits1(gb);
> +    time_base= seconds + 60*(minutes + 60*hours);
> +    if (time_base < s->time_base) {
> +        av_log(s->avctx, AV_LOG_WARNING, "time_code in GOP header is non monotone, skipping\n");
> +    } else {
> +        s->time_base= time_base;
> +    }
>
>      return 0;
>  }

The time_code doesn't have to be monotonically increasing IIUC.  Better
to just check the marker bit and do nothing if it's not there.  It won't
be there in your broken streams.

> @@ -1947,12 +1950,7 @@ static int decode_vop_header(MpegEncContext *s, GetBitContext *gb){
>          }
>      }
>
> -    if(s->avctx->time_base.num)
> -        s->current_picture_ptr->pts= (s->time + s->avctx->time_base.num/2) / s->avctx->time_base.num;
> -    else
> -        s->current_picture_ptr->pts= AV_NOPTS_VALUE;
> -    if(s->avctx->debug&FF_DEBUG_PTS)
> -        av_log(s->avctx, AV_LOG_DEBUG, "MPEG4 PTS: %"PRId64"\n", s->current_picture_ptr->pts);
> +    s->current_picture_ptr->pts= AV_NOPTS_VALUE;
>
>      check_marker(gb, "before vop_coded");

This is for another patch.  I wouldn't worry about it right now.

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