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

Sat Mar 20 22:14:40 CET 2010

On 10-03-20 02:09 PM, Micah F. Galizia wrote:
> On 10-03-17 10:34 PM, Michael Niedermayer wrote:
>> On Wed, Mar 17, 2010 at 08:53:21PM -0400, Micah F. Galizia wrote:
>>> On 10-03-17 08:29 PM, Michael Niedermayer wrote:
>>>> On Wed, Mar 17, 2010 at 08:12:05PM -0400, Micah F. Galizia wrote:
>>>>> 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.
>>>>
>>>> but thats not the core bug
>>>> you never reached max_probe_size, which seems wrong somehow
>>>> either max_probe_size is limited to powers of 2 or not
>>>> but either way the code should reach it in its final pass
>>>
>>> Yes, you are correct. How about this then?
>>> --
>>> 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 | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>> f54791f1b1b7fb18c47cc51f9d2954195ebead3d probe_buffer7.diff
>>> Index: libavformat/utils.c
>>> ===================================================================
>>> --- libavformat/utils.c (revision 22585)
>>> +++ libavformat/utils.c (working copy)
>>> @@ -478,7 +478,8 @@
>>> return AVERROR(EINVAL);
>>> }
>>>
>>> - for(probe_size= PROBE_BUF_MIN; probe_size<=max_probe_size&& !*fmt&&
>>> ret>= 0; probe_size<<=1){
>>> + for(probe_size= PROBE_BUF_MIN; probe_size<=max_probe_size&& !*fmt&&
>>> ret>= 0;
>>> + probe_size = (probe_size<<1< max_probe_size) ? probe_size<<1 :
>>> max_probe_size){
>>
>> FFMIN
>>
>> and ok if tested
>
> This version uses FFMIN instead. This version also returns an error if
> we get to the max probe length and still cant figure out what the format
> is. This is to avoid getting stuck in an infinite loop.

Sorry,the last one has a memory leak -- probe_buffer9.diff corrects it.

-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: probe_buffer9.diff
Type: text/x-diff
Size: 1405 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100320/87f50603/attachment.diff>