[FFmpeg-devel] [PATCH] close parser in av_find_stream_info is parsing is not needed

[Baptiste Coudurier]

Hi Michael,

On 05/18/2010 11:29 AM, Michael Niedermayer wrote:
> On Tue, May 18, 2010 at 06:51:05AM -0700, Baptiste Coudurier wrote:
>> On 5/18/10 5:58 AM, Michael Niedermayer wrote:
>>> On Mon, May 17, 2010 at 01:17:42PM -0700, Baptiste Coudurier wrote:
>>>> Hey guys,
>>>>
>>>> $subject.
>>>>
>>>> The parser is opened here:
>>>>
>>>>           //only for the split stuff
>>>>           if (!st->parser&&   !(ic->flags&   AVFMT_FLAG_NOPARSE)) {
>>>>               st->parser = av_parser_init(st->codec->codec_id);
>>>>               if(st->need_parsing == AVSTREAM_PARSE_HEADERS&&
>>>> st->parser){
>>>>                   st->parser->flags |= PARSER_FLAG_COMPLETE_FRAMES;
>>>>               }
>>>>           }
>>>>
>>>> So either, it must be closed after, or it shouldn't be opened if parsing
>>>> is
>>>> not needed. I'm not sure of the side effects.
>>>
>>> Parsers that have a AVCodecParser.split() must be opened and must stay
>>> open
>>> until they have extracted the global headers into extradata
>>> after that they could be closed if they arent needed for any other reasons
>>>
>>> There are several problems with this though, for example streams that
>>> start
>>> later (after av_find_stream_info()) might not have a parser enabled even
>>> if
>>> they need one for split().
>>>
>>> also if we close them before they have set extradata that similarly would
>>> be a problem
>>
>> IMHO this is an ugly hack anyway, extradata should be extracted with a
>> bitstream filter. This has proven to produce side effects.
>
> The sideeffects where due to av_parser_change() and the muxer API namely
> AVFMT_GLOBALHEADER being not expressive enough.
> mpeg2 and mpeg4 in mov would have needed different AVFMT_GLOBALHEADER but
> as there is just one flag per muxer one had to blow up.

This was one side effect, yes, there is another for AAC with extradata, 
because the parser doesn't handle it, but hopefully it does not have 
"split". This could apply to other codecs as well, not sure.

> We could implement the extraction with bitstream filters or anything else
> but it would not affect the side effect we had unless i misremember.
> Also whichever way does the extraction, must do so before actual decoding
> that is we need to search the stream for the headers and then go back and
> only once we have them start decoding from the begin.
> This would implicate that it has to stay in av_find_stream_info() or another
> thing run prior to actual decoding/demuxing.
> The demuxing case results from us needing extradata at the begin for writing
> the header for the -codec copy case.

Well if stream header is not found before av_find_stream_info finishes, 
there is no luck, but how frequently will this happen ? Extradata is 
usually stream header and without them no decoding is possible.

Also can the bistream filter buffer data itself ?

>>
>> Anyway, in ffmpeg.c:
>>                      int ticks= ist->st->parser ?
>> ist->st->parser->repeat_pict+1 : ist->st->codec->ticks_per_frame;
>>
>> This is no good for H264 in mkv, since parser is set but does not set
>> repeat_pict. What do suggest ?
>
> definitly, fix the parser

Make the parser support the bistream in mov/mp4 ?

> even if we later decide to change or remove the code from ffmpeg.c we
> still need the parser to handle the mkv/mov case, i remember some bugreport
> related to the parser not supporting that, but doent remember what exactly
> it was about
>
>
>> Using repeat_pict like this outside libavformat looks a bit weird to me
>> anyway.
>
> true
> for the case with decoding we could use AVFrame.repeat_pict probably
> a patch changing this would be welcome if it passes fate

Ok.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org