[FFmpeg-devel] [PATCH] Move av_open_input_file probe loop to its own method
Micah F. Galizia
micahgalizia
Sat Mar 20 19:09:55 CET 2010
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.
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: probe_buffer8.diff
Type: text/x-diff
Size: 1378 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100320/c6f0534b/attachment.diff>
More information about the ffmpeg-devel
mailing list