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

[Baptiste Coudurier]

On 7/2/10 7:49 PM, Michael Niedermayer wrote:
> On Fri, Jul 02, 2010 at 12:02:28PM -0700, Baptiste Coudurier wrote:
>> 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.
>
> yes, i would prefer to keep the double check
>
> if you have an actual test case where this causes a problem i dont mind
> reconsidering this

Ok, applied your way.

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