[FFmpeg-devel] [PATCH] Move av_open_input_file probe loop to its own method
Michael Niedermayer
michaelni
Thu Mar 18 03:34:30 CET 2010
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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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/b67653b6/attachment.pgp>
More information about the ffmpeg-devel
mailing list