[FFmpeg-devel] [PATCH] Move av_open_input_file probe loop to its own method

[Micah F. Galizia]

On 10-03-17 07:58 PM, Michael Niedermayer wrote:
> On Tue, Mar 16, 2010 at 10:41:09PM -0400, Micah F. Galizia wrote:
>> On 10-03-14 06:41 PM, Stefano Sabatini wrote:
>>> On date Friday 2010-03-12 17:53:49 -0500, Micah F. Galizia encoded:
>>>> On 10-03-10 05:59 AM, Michael Niedermayer wrote:
>>>>> On Mon, Mar 08, 2010 at 08:29:40PM -0500, Micah F. Galizia wrote:
>>>>>> On 10-03-07 07:42 PM, M?ns Rullg?rd wrote:
>>>>>>> Stefano Sabatini<stefano.sabatini-lala at poste.it>     writes:
>>>>>>>
>>>>>>>> On date Sunday 2010-03-07 12:48:45 -0500, MIcah Galizia encoded:
>>>>>>>>> On 10-03-07 05:52 AM, Stefano Sabatini wrote:
>>>>>>>>>> On date Saturday 2010-03-06 19:02:07 -0500, Micah F. Galizia
>>>>>>>>>> encoded:
>>>>>>>>>>> On 10-03-06 06:17 PM, Michael Niedermayer wrote:
>>>>>>>> [...]
>>>>>>>>>>>> should be ok if tested
>>>>>>>>>>>
>>>>>>>>>>> I tested it in plain old ffmpeg, and with my shoutcast changes.
>>>>>>>>>>
>>>>>>>>>> Please test with make test, I tried it and it failed in
>>>>>>>>>> regtest-pbmpipe.
>>>>>>>>>>
>>>>>>>>>> cat pbmpipe.lavf.err
>>>>>>>>>> /home/stefano/src/ffmpeg.git/./tests/data/lavf/pbmpipe.pbm: Error
>>>>>>>>>> while
>>>>>>>>>> opening file
>>>>>>>>>
>>>>>>>>> Thank you.  The attached patch fixes it.  The problem was that after
>>>>>>>>> the initial probe in av_open_input_file (with no actual probe data),
>>>>>>>>> I was setting the input format to NULL in ff_probe_input_buffer.
>>>>>>>>> Now all regression tests pass (make test).
>>>>>>>>>
>>>>>>>>> Also, I have improved the commenting in internal.h.
>>>>>>>>>
>>>>>>>>> Thanks again!
>>>>>>>>
>>>>>>>> Patch applied, thanks for the fish.
>>>>>>>
>>>>>>> This broke ea-cdata on FATE.
>>>>>>
>>>>>> Actually, it would have broken on any probe score lower than 25 for a
>>>>>> file
>>>>>> smaller than 1MB.  The problem was that the old probe loop would reset
>>>>>> the
>>>>>> file position each time before calling get_buffer. As a result,
>>>>>> get_buffer
>>>>>> would never return AVERROR_EOF, so this wasn't ever a situation that
>>>>>> needed
>>>>>> to be handled.
>>>>>>
>>>>>> The modified probe just reads on from where it left off in the previous
>>>>>> probe, so eventually, the end of file is reached before reaching
>>>>>> PROBE_BUF_MAX, so it would just return an error.  It has been updated
>>>>>> now
>>>>>> to lower the required probe score when the end of file is reached in
>>>>>> addition to when PROBE_BUF_MAX is reached.
>>>>>>
>>>>>> Thanks again, and sorry for breaking FATE!
>>>>>> --
>>>>>> Micah F. Galizia
>>>>>> micahgalizia at gmail.com
>>>>>>
>>>>>> "The mark of an immature man is that he wants to die nobly for a cause,
>>>>>> while the mark of the mature man is that he wants to live humbly for
>>>>>> one."
>>>>>>    --W. Stekel
>>>>>
>>>>>>    internal.h |   18 +++++++++++
>>>>>>    utils.c    |   97
>>>>>> +++++++++++++++++++++++++++++++++++++++++--------------------
>>>>>>    2 files changed, 84 insertions(+), 31 deletions(-)
>>>>>> 73e23f49aa0fbc3c6a08b506c3888a40cdddab2f  probe_buffer5.diff
>>>>>
>>>>> looks good, if tested
>>>>
>>>> Is someone going to check this in?
>>>
>>> Applied, let's see if FATE will choke again this time! ;-)
>>
>> Attached is a patch against the recently accepted patch. If the max probe
>> buffer length falls between two probe_size values (24KB for example), we
>> wont drop the required score, and as a result, probing might not succeed
>> when it should.
>>
>> The patch is patchecked and make test ran successfully. It has not been
>> tested against the whole FATE test set (it is still downloading).
>>
>> TIA
>> --
>> Micah F. Galizia
>> micahgalizia at gmail.com
>>
>> "The mark of an immature man is that he wants to die nobly for a cause,
>> while the mark of the mature man is that he wants to live humbly for one."
>>   --W. Stekel
>
>>   utils.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 62b907c075f3d7be944976c3e2bca5a935129346  probe_buffer6.diff
>> Index: libavformat/utils.c
>> ===================================================================
>> --- libavformat/utils.c	(revision 22571)
>> +++ libavformat/utils.c	(working copy)
>> @@ -479,7 +479,7 @@
>>       }
>>
>>       for(probe_size= PROBE_BUF_MIN; probe_size<=max_probe_size&&  !*fmt&&  ret>= 0; probe_size<<=1){
>> -        int ret, score = probe_size<  max_probe_size ? AVPROBE_SCORE_MAX/4 : 0;
>> +        int ret, score = probe_size<<1<= max_probe_size ? AVPROBE_SCORE_MAX/4 : 0;
>
> this looks wrong
>
> the code before looks correct,
> "if probe_size is less than the maximum probe_size .." is exactly what it is
> supposed to check for

I would say that "if the probe_size is less than the maximum probe_size" 
is not considering all situations we might encounter now that the 
maximum probe size is variable.

In the situation where probe_size is less than maximum probe size, but 
on the next pass through the loop, probe_size will be greater than the 
maximum probe size, we will not reenter the loop. As a result, we will 
have required AVPROBE_SCORE_MAX/4 on every pass, which isn't what we 
really wanted.

IMO, what we really want here is to require AVPROBE_SCORE_MAX/4 on every 
pass, except the last, which would settle for anything greater than 
zero.  This is what the new code does.

Does this make sense?
-- 
Micah F. Galizia
micahgalizia at gmail.com

"The mark of an immature man is that he wants to die nobly for a cause, 
while the mark of the mature man is that he wants to live humbly for 
one."   --W. Stekel