[FFmpeg-devel,1/3] libavformat/avio: retry_transfer_wrapper: don't treat 0 as EOF

Submitted by Daniel Kucera on Oct. 13, 2017, 7:03 p.m.

Details

Message ID 20171013190349.24112-1-daniel.kucera@gmail.com
State New
Headers show

Commit Message

Daniel Kucera Oct. 13, 2017, 7:03 p.m.
transfer_func variable passed to retry_transfer_wrapper
are h->prot->url_read and h->prot->url_write functions.
These need to return EOF or other error properly.
In case of returning >= 0, url_read/url_write is retried
until error is returned.

Signed-off-by: Daniel Kucera <daniel.kucera@gmail.com>
---
 libavformat/avio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer Oct. 13, 2017, 11 p.m.
On Fri, Oct 13, 2017 at 09:03:47PM +0200, Daniel Kucera wrote:
> transfer_func variable passed to retry_transfer_wrapper
> are h->prot->url_read and h->prot->url_write functions.
> These need to return EOF or other error properly.
> In case of returning >= 0, url_read/url_write is retried
> until error is returned.
> 
> Signed-off-by: Daniel Kucera <daniel.kucera@gmail.com>
> ---
>  libavformat/avio.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

breaks fate-indeo3-2

[...]
Daniel Kucera Oct. 14, 2017, 5:10 a.m.
breaks fate-indeo3-2

[...]
--


You need to apply the whole set. It passed all fate tests on my machine.
Michael Niedermayer Oct. 14, 2017, 3:46 p.m.
On Sat, Oct 14, 2017 at 07:10:37AM +0200, Daniel Kučera wrote:
> breaks fate-indeo3-2
> 
> [...]
> --
> 
> 
> You need to apply the whole set. It passed all fate tests on my machine.

I applied the whole set and bisected to find which patch caused the
failure

also the i-th patch of a set must pass fate with it and the 0th to ith
patches applied, otherwise there would be checkouts that fail some tests

[...]
Daniel Kucera Oct. 14, 2017, 5:16 p.m.
2017-10-14 17:46 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
> On Sat, Oct 14, 2017 at 07:10:37AM +0200, Daniel Kučera wrote:
>> breaks fate-indeo3-2
>>
>> [...]
>> --
>>
>>
>> You need to apply the whole set. It passed all fate tests on my machine.
>
> I applied the whole set and bisected to find which patch caused the
> failure
>
> also the i-th patch of a set must pass fate with it and the 0th to ith
> patches applied, otherwise there would be checkouts that fail some tests
>

So should I reorder them and post again?
Daniel Kucera Oct. 14, 2017, 5:22 p.m.
2017-10-14 19:16 GMT+02:00 Daniel Kučera <daniel.kucera@gmail.com>:
> 2017-10-14 17:46 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
>> On Sat, Oct 14, 2017 at 07:10:37AM +0200, Daniel Kučera wrote:
>>> breaks fate-indeo3-2
>>>
>>> [...]
>>> --
>>>
>>>
>>> You need to apply the whole set. It passed all fate tests on my machine.
>>
>> I applied the whole set and bisected to find which patch caused the
>> failure
>>
>> also the i-th patch of a set must pass fate with it and the 0th to ith
>> patches applied, otherwise there would be checkouts that fail some tests
>>
>
> So should I reorder them and post again?
>
>

What if they don't pass fate in any order? should I post them as one patch?
James Almer Oct. 14, 2017, 5:34 p.m.
On 10/14/2017 2:22 PM, Daniel Kučera wrote:
> 2017-10-14 19:16 GMT+02:00 Daniel Kučera <daniel.kucera@gmail.com>:
>> 2017-10-14 17:46 GMT+02:00 Michael Niedermayer <michael@niedermayer.cc>:
>>> On Sat, Oct 14, 2017 at 07:10:37AM +0200, Daniel Kučera wrote:
>>>> breaks fate-indeo3-2
>>>>
>>>> [...]
>>>> --
>>>>
>>>>
>>>> You need to apply the whole set. It passed all fate tests on my machine.
>>>
>>> I applied the whole set and bisected to find which patch caused the
>>> failure
>>>
>>> also the i-th patch of a set must pass fate with it and the 0th to ith
>>> patches applied, otherwise there would be checkouts that fail some tests
>>>
>>
>> So should I reorder them and post again?
>>
>>
> 
> What if they don't pass fate in any order? should I post them as one patch?

If one patch introduces a problem that gets fixed by a subsequent patch
then yes, both should be squashed into one to make sure the problem is
never introduced.

The idea is that no patch/commit on its own should generate a fate
failure of any kind, as it would make bisecting unrelated future issues
harder.
Daniel Kucera Oct. 14, 2017, 5:38 p.m.
>
> If one patch introduces a problem that gets fixed by a subsequent patch
> then yes, both should be squashed into one to make sure the problem is
> never introduced.
>
> The idea is that no patch/commit on its own should generate a fate
> failure of any kind, as it would make bisecting unrelated future issues
> harder.

Thank you for explaining.
I've sent squashed patch.
Nicolas George Oct. 15, 2017, 6:26 p.m.
Le duodi 22 vendémiaire, an CCXXVI, Daniel Kucera a écrit :
> transfer_func variable passed to retry_transfer_wrapper
> are h->prot->url_read and h->prot->url_write functions.
> These need to return EOF or other error properly.
> In case of returning >= 0, url_read/url_write is retried
> until error is returned.
> 
> Signed-off-by: Daniel Kucera <daniel.kucera@gmail.com>
> ---
>  libavformat/avio.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

I think the changes in this series are mostly correct, but the fact they
cause failure if they are applied separately is a little worrying. There
is something subtly wrong going on here.

I think there is another instance of the problem hidden somewhere that
is causing more complication. I have been trying to find it, but not
yet. Still on it when I have time.

Regards,

Patch hide | download patch | download mbox

diff --git a/libavformat/avio.c b/libavformat/avio.c
index 64248e098b..d3004c007f 100644
--- a/libavformat/avio.c
+++ b/libavformat/avio.c
@@ -391,8 +391,10 @@  static inline int retry_transfer_wrapper(URLContext *h, uint8_t *buf,
                 }
                 av_usleep(1000);
             }
-        } else if (ret < 1)
-            return (ret < 0 && ret != AVERROR_EOF) ? ret : len;
+        } else if (ret == AVERROR_EOF)
+	    return (len > 0) ? len : AVERROR_EOF;
+        else if (ret < 0)
+            return ret;
         if (ret) {
             fast_retries = FFMAX(fast_retries, 2);
             wait_since = 0;