[FFmpeg-devel] [PATCH] decode at least 4 H.264 frames in av_find_stream_info

[Baptiste Coudurier]

On 7/2/10 7:01 AM, Michael Niedermayer wrote:
> On Thu, Jul 01, 2010 at 06:18:38PM -0700, Baptiste Coudurier wrote:
>> On 07/01/2010 05:03 PM, Michael Niedermayer wrote:
>>> On Mon, Jun 28, 2010 at 07:34:58PM -0700, Baptiste Coudurier wrote:
>>>> On 06/27/2010 07:03 PM, Michael Niedermayer wrote:
>>>>> On Sun, Jun 27, 2010 at 05:44:46PM -0700, Baptiste Coudurier wrote:
>>>>>> On 6/27/10 3:34 PM, Michael Niedermayer wrote:
>>>>>>> On Sun, Jun 27, 2010 at 02:18:18PM -0700, Baptiste Coudurier wrote:
>>>>>>>> On 6/27/10 7:09 AM, Michael Niedermayer wrote:
>>>>>>>>> On Sun, Jun 27, 2010 at 12:39:13AM -0700, Baptiste Coudurier wrote:
>>>>>>>>>> On 6/26/10 7:43 PM, Michael Niedermayer wrote:
>>>>>>>>>>> On Sun, Jun 27, 2010 at 04:31:41AM +0200, Michael Niedermayer
>>>>>>>>>>> wrote:
>>>>>>>>>>>> On Thu, Jun 24, 2010 at 05:44:20PM -0700, Baptiste Coudurier
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> Hi guys,
>>>>>>>>>>>>>
>>>>>>>>>>>>> $subject.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This will permit the decoder to compute has_b_frames correctly
>>>>>>>>>>>>> when
>>>>>>>>>>>>> b-pyramid is present. 4 is typically the minimum needed, though
>>>>>>>>>>>>> maybe
>>>>>>>>>>>>> we
>>>>>>>>>>>>> should decode more based on has_b_frames. Any opinion ?
>>>>>>>>>>>>
>>>>>>>>>>>> Finally found an old patch by myself doing this
>>>>>>>>>>>> I think its a bit more readable
>>>>>>>>>>>> if it works feel free to apply mine (assuming that thing still
>>>>>>>>>>>> applies
>>>>>>>>>>>> and
>>>>>>>>>>>> works)
>>>>>>>>>>>
>>>>>>>>>>> ehm, the patch:
>>>>>>>>>>>
>>>>>>>>>>> Index: libavformat/utils.c
>>>>>>>>>>> ===================================================================
>>>>>>>>>>> --- libavformat/utils.c	(revision 18646)
>>>>>>>>>>> +++ libavformat/utils.c	(working copy)
>>>>>>>>>>> @@ -1820,6 +1820,12 @@
>>>>>>>>>>>        #endif
>>>>>>>>>>>        }
>>>>>>>>>>>
>>>>>>>>>>> +/*we need to find has_b_frames approximatly, the parser is not
>>>>>>>>>>> yet
>>>>>>>>>>> able
>>>>>>>>>>> to do it*/
>>>>>>>>>>> +static int has_decode_delay_been_guessed(AVCodecContext *enc)
>>>>>>>>>>> +{
>>>>>>>>>>> +    return enc->codec_id != CODEC_ID_H264 || enc->frame_number>=
>>>>>>>>>>> 4
>>>>>>>>>>> +
>>>>>>>>>>> enc->has_b_frames;
>>>>>>>>>>> +}
>>>>>>>>>>
>>>>>>>>>> I'm not sure about enc->frame_number, now that
>>>>>>>>>> st->codec_info_nb_frames
>>>>>>>>>> is
>>>>>>>>>> used in av_find_stream_info, it seems the best field to use.
>>>>>>>>>
>>>>>>>>> hmm, i dont think theres a difference currently between using these
>>>>>>>>> 2
>>>>>>>>> though they are semantically a bit different. So use what you prefer
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Your patch is not enough I think, you need to always call
>>>>>>>>>> try_decode_frame
>>>>>>>>>> even if has_codec_parameters is true.
>>>>>>>>>
>>>>>>>>> calling it for the first 4 frames with h264 is ok
>>>>>>>>> calling it unconditionally always would slow av_find_stream_info()
>>>>>>>>> down
>>>>>>>>> quite
>>>>>>>>> a bit i think
>>>>>>>>>
>>>>>>>>
>>>>>>>> This is code in svn:
>>>>>>>>
>>>>>>>>             if (!has_codec_parameters(st->codec))
>>>>>>>>                 try_decode_frame(st, pkt);
>>>>>>>>
>>>>>>>> I meant the check has to be removed, otherwise only one frame will be
>>>>>>>> decoded.
>>>>>>>
>>>>>>> what about:
>>>>>>>
>>>>>>> if (!has_codec_parameters(st->codec) ||
>>>>>>> !has_decode_delay_been_guessed())
>>>>>>> ?
>>>>>>
>>>>>> Why duplicating the check ? This check is also in try_decode_frame, so
>>>>>> either one can be removed.
>>>>>
>>>>> i wish it could but they arent useless
>>>>> its "if we dont have parameter yet open codec"
>>>>> and "if we still dont have parameers yet decode frame"
>>>>>
>>>>> droping the first will open codecs for all streams, droping the second
>>>>> will decode frames even if its unneeded after opening the codec
>>>>
>>>> What about that ?
>>>
>>> I think with this a codec that has its parameters filled in by its
>>> avcodec_open() will have its first frame decoded unneccesarily.
>>
>> Yes, do you see anything wrong with that ? Always decoding the first frame
>> might be a benefit, I actually have a patch in my tree for that.
>
> it takes additional time and allocates additional memory.
> with many streams (maybe 20 audio streams in different languages)
> that could on slow hardware cause a noticeable delay<  1 sec though.
> But if its needed for fixing some issue? then iam not opposed

Well there has been many cases where demuxer information contradicts 
elementary stream information. Forcing decoding of the first frame would 
make the decoder fix this, and avoid some ugly #ifdef DECODER_PRESENT.

But anyway, this patch must be applied in one way or another I don't 
like the double check, but if you want to keep it then it will be kept.

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