[FFmpeg-devel] [PATCH] Move av_open_input_file probe loop to its own method
Micah F. Galizia
micahgalizia
Thu Mar 18 01:53:21 CET 2010
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: probe_buffer7.diff
Type: text/x-diff
Size: 674 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100317/be53be7b/attachment.diff>
More information about the ffmpeg-devel
mailing list