[FFmpeg-devel] [PATCH] H264 parser fix

[John Stebbins]

 On 05/25/2010 03:26 PM, Michael Niedermayer wrote:
> On Tue, May 25, 2010 at 09:31:51AM -0700, Howard Chu wrote:
>> Michael Niedermayer wrote:
>>> On Tue, May 25, 2010 at 07:29:11AM -0700, Howard Chu wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Mon, May 24, 2010 at 11:25:05AM -0700, Howard Chu wrote:
>>>>>> Michael Niedermayer wrote:
>>>>>>> On Wed, May 19, 2010 at 01:15:17AM -0700, Howard Chu wrote:
>>>>>>>> When decoding an H264 stream that uses global headers, you get an
>>>>>>>> endless
>>>>>>>> stream of "non-existing PPS referenced" messages. This message comes
>>>>>>>> from
>>>>>>>> h264_parser.c, and it happens because it's using an H264Context 
>>>>>>>> hanging
>>>>>>>> off
>>>>>>>> the AVCodecParserContext, which is separate from the H264Context
>>>>>>>> hanging
>>>>>>>> off the AVCodecContext, and only the AVCodecContext contains the
>>>>>>>> extradata
>>>>>>>> that provides the PPS and SPS.
>>>>>>>>
>>>>>>>> This patch is admittedly a hack, but it stops the error. It just 
>>>>>>>> checks
>>>>>>>> in
>>>>>>>> the AVCodecContext if the PPS wasn't found in the 
>>>>>>>> AVCodecParserContext.
>>>>>>> ugly ugly ugly
>>>>>>> if iam not mistaken this would be fixed by making the parser work with
>>>>>>> mov/mkv ("avc") style h264
>>>>>> I read thru mov.c and matroskadec.c and couldn't find anything 
>>>>>> relevant,
>>>>>> so
>>>>>> I don't understand what your suggestion means.
>>>>> i thought that the problem is that the parser is feeded a format it does
>>>>> not
>>>>> support. I did not confirm this but i see no reason that this is a
>>>>> different
>>>>> issue.
>>>> That is not the problem I was reporting. The parser has no trouble with 
>>>> the
>>>> data format. It is simply missing the PPS and SPS because they were not
>>>> available in the AVCodecParserContext. The extradata is only available in
>>>> the AVCodecContext.
>>>>
>>>>> The format difference should be easiest vissible by looking at code
>>>>> surrounding is_avc in h264.c
>>>>>
>>>>> if this is the issue the parser should be improved to suport the slghtly
>>>>> differnt way to pack things and also to look in extradata.
>>>> Before we can tell the parser to look in the extradata, we have to give 
>>>> the
>>>> parser a way to access it, ideally when the parser is initialized. Right
>>>> now there's no reliable way to do that.
>>> The parse function has the AVCodecContext extradata
>>>> It seems odd to me that the parser
>>>> entry points all require an AVCodecContext but av_parser_init() only 
>>>> takes
>>>> a codec_id. If av_parser_init() took an AVCodecContext instead, all of 
>>>> the
>>>> necessary information would be available.
>>> well, yes, its not given during init but just later, i dont see how this
>>> would be a problem
>> This patch works. It just means adding an if (extradata_not_parsed_yet) 
>> test in h264_parse(), instead of handling it unconditionally at init time. 
>> Probably a trivial overhead. I've just taken the extradata parsing code 
>> from ff_h264_decode_init() and broken it out into its own function so that 
>> h264_parse can use it too.
>>
>> -- 
>>   -- Howard Chu
>>   CTO, Symas Corp.           http://www.symas.com
>>   Director, Highland Sun     http://highlandsun.com/hyc/
>>   Chief Architect, OpenLDAP  http://www.openldap.org/project/
>> Index: libavcodec/h264_parser.c
>> ===================================================================
>> --- libavcodec/h264_parser.c	(revision 23295)
>> +++ libavcodec/h264_parser.c	(working copy)
>> @@ -245,6 +245,12 @@
>>      ParseContext *pc = &h->s.parse_context;
>>      int next;
>>  
>> +    /* Should a failure here cause the entire parse to fail? */
> no
>
>
>> +    if (!h->sps_buffers[0] && avctx->extradata_size) {
> that looks wrong, i dont think anything gurantees that there is a sps
> with sps_id=0
>
>
>
>> +        h->s.avctx = avctx;
>> +        ff_h264_decode_extradata(avctx, h);
>> +    }
>> +
>>      if(s->flags & PARSER_FLAG_COMPLETE_FRAMES){
>>          next= buf_size;
>>      }else{
>> Index: libavcodec/h264.c
>> ===================================================================
>> --- libavcodec/h264.c	(revision 23295)
>> +++ libavcodec/h264.c	(working copy)
>> @@ -844,41 +844,9 @@
>>      memset(h->pps.scaling_matrix8, 16, 2*64*sizeof(uint8_t));
>>  }
>>  
>> -av_cold int ff_h264_decode_init(AVCodecContext *avctx){
>> -    H264Context *h= avctx->priv_data;
>> -    MpegEncContext * const s = &h->s;
>> -
>> -    MPV_decode_defaults(s);
>> -
>> -    s->avctx = avctx;
>> -    common_init(h);
>> -
>> -    s->out_format = FMT_H264;
>> -    s->workaround_bugs= avctx->workaround_bugs;
>> -
>> -    // set defaults
>> -//    s->decode_mb= ff_h263_decode_mb;
>> -    s->quarter_sample = 1;
>> -    if(!avctx->has_b_frames)
>> -    s->low_delay= 1;
>> -
>> -    avctx->chroma_sample_location = AVCHROMA_LOC_LEFT;
>> -
>> -    ff_h264_decode_init_vlc();
>> -
>> -    h->thread_context[0] = h;
>> -    h->outputed_poc = INT_MIN;
>> -    h->prev_poc_msb= 1<<16;
>> -    h->x264_build = -1;
>> -    ff_h264_reset_sei(h);
>> -    if(avctx->codec_id == CODEC_ID_H264){
>> -        if(avctx->ticks_per_frame == 1){
>> -            s->avctx->time_base.den *=2;
>> -        }
>> -        avctx->ticks_per_frame = 2;
>> -    }
>> -
>> -    if(avctx->extradata_size > 0 && avctx->extradata && *(char *)avctx->extradata == 1){
>> +int ff_h264_decode_extradata(AVCodecContext *avctx, H264Context *h)
> no need to pass AVCodecContext if its in h->s.avctx
>
> also the factorization should be a seperate patch
>
> [...]
>
Any movement on this issue? Still seeing it in svn 25249.  Avatar BD m2ts is a good example of the problem.