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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100703/b567df56/attachment.pgp>