[FFmpeg-devel] [PATCH] Move av_open_input_file probe loop to its own method
Michael Niedermayer
michaelni
Thu Mar 18 01:29:41 CET 2010
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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20100318/76708219/attachment.pgp>
More information about the ffmpeg-devel
mailing list