[FFmpeg-devel] libavformat/cache: don't treat 0 as EOF libavformat/aviobuf: don't treat 0 as EOF libavformat/avio: retry_transfer_wrapper: don't treat 0 as EOF

Submitted by Daniel Kucera on Oct. 14, 2017, 5:27 p.m.

Details

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

Commit Message

Daniel Kucera Oct. 14, 2017, 5:27 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 ++++--
 libavformat/aviobuf.c | 20 ++++++++++++--------
 libavformat/cache.c   |  4 ++--
 3 files changed, 18 insertions(+), 12 deletions(-)

Comments

Moritz Barsnick Oct. 15, 2017, 10:51 p.m.
On Sat, Oct 14, 2017 at 19:27:34 +0200, Daniel Kucera wrote:
> Subject: [FFmpeg-devel] [PATCH] libavformat/cache: don't treat 0 as EOF
>         libavformat/aviobuf: don't treat 0 as EOF libavformat/avio:
>         retry_transfer_wrapper: don't treat 0 as EOF

Something went wrong with your commit message. (I think, after
squashing by cherry-picking, you just need to re-edit the commit
message explicitly.)

Moritz
Michael Niedermayer Oct. 16, 2017, 12:38 a.m.
On Sat, Oct 14, 2017 at 07:27:34PM +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 ++++--
>  libavformat/aviobuf.c | 20 ++++++++++++--------
>  libavformat/cache.c   |  4 ++--
>  3 files changed, 18 insertions(+), 12 deletions(-)

This changes:
./ffmpeg -y -i 'concat:matrixbench_mpeg2.mpg|matrixbench_mpeg2.mpg' -vsync 0 -an file.avi

[...]
James Almer Oct. 16, 2017, 1:55 a.m.
On 10/15/2017 9:38 PM, Michael Niedermayer wrote:
> On Sat, Oct 14, 2017 at 07:27:34PM +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 ++++--
>>  libavformat/aviobuf.c | 20 ++++++++++++--------
>>  libavformat/cache.c   |  4 ++--
>>  3 files changed, 18 insertions(+), 12 deletions(-)
> 
> This changes:
> ./ffmpeg -y -i 'concat:matrixbench_mpeg2.mpg|matrixbench_mpeg2.mpg' -vsync 0 -an file.avi

In case he doesn't have this sample:
http://samples.ffmpeg.org/benchmark/testsuite1/matrixbench_mpeg2.mpg

Basically, with the patch applied the above command doesn't concatenate
the input as requested.
Daniel Kucera Oct. 16, 2017, 3:10 a.m.
> This changes:
> ./ffmpeg -y -i 'concat:matrixbench_mpeg2.mpg|matrixbench_mpeg2.mpg'
-vsync 0 -an file.avi

In case he doesn't have this sample:
http://samples.ffmpeg.org/benchmark/testsuite1/matrixbench_mpeg2.mpg

Basically, with the patch applied the above command doesn't concatenate
the input as requested.


Is there a reason why this test is not included in fate?
James Almer Oct. 16, 2017, 3:51 a.m.
On 10/16/2017 12:10 AM, Daniel Kučera wrote:
>> This changes:
>> ./ffmpeg -y -i 'concat:matrixbench_mpeg2.mpg|matrixbench_mpeg2.mpg'
> -vsync 0 -an file.avi
> 
> In case he doesn't have this sample:
> http://samples.ffmpeg.org/benchmark/testsuite1/matrixbench_mpeg2.mpg
> 
> Basically, with the patch applied the above command doesn't concatenate
> the input as requested.
> 
> 
> Is there a reason why this test is not included in fate?

Tests are added as regressions are found in some cases, and as features
are added in others. There's no concat protocol test in FATE currently,
which is obviously not ideal.

I see there's a cut version of this file in the fate samples suite,
mpeg2/matrixbench_mpeg2.lq1.mpg, which is enough to reproduce this
issue. A test could be created with it, but i'm not sure how to make a
working command line with it for fate that will not trip in some cases
depending on what's the path to the samples folder (Windows paths are
especially annoying in this regard).
Daniel Kucera Oct. 16, 2017, 9:14 a.m.
>
> This changes:
> ./ffmpeg -y -i 'concat:matrixbench_mpeg2.mpg|matrixbench_mpeg2.mpg' -vsync 0 -an file.avi
>

fixed, sending new patch.

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;
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 636cb46161..0d4eb051e1 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -572,13 +572,14 @@  static void fill_buffer(AVIOContext *s)
     if (s->read_packet)
         len = s->read_packet(s->opaque, dst, len);
     else
-        len = 0;
-    if (len <= 0) {
+        len = AVERROR_EOF;
+    if (len == AVERROR_EOF) {
         /* do not modify buffer if EOF reached so that a seek back can
            be done without rereading data */
         s->eof_reached = 1;
-        if (len < 0)
-            s->error = len;
+    } else if (len < 0) {
+        s->eof_reached = 1;
+        s->error= len;
     } else {
         s->pos += len;
         s->buf_ptr = dst;
@@ -646,13 +647,16 @@  int avio_read(AVIOContext *s, unsigned char *buf, int size)
                 // bypass the buffer and read data directly into buf
                 if(s->read_packet)
                     len = s->read_packet(s->opaque, buf, size);
-
-                if (len <= 0) {
+                else
+                    len = AVERROR_EOF;
+                if (len == AVERROR_EOF) {
                     /* do not modify buffer if EOF reached so that a seek back can
                     be done without rereading data */
                     s->eof_reached = 1;
-                    if(len<0)
-                        s->error= len;
+                    break;
+                } else if (len < 0) {
+                    s->eof_reached = 1;
+                    s->error= len;
                     break;
                 } else {
                     s->pos += len;
diff --git a/libavformat/cache.c b/libavformat/cache.c
index 6aabca2e78..66bbbf54c9 100644
--- a/libavformat/cache.c
+++ b/libavformat/cache.c
@@ -201,7 +201,7 @@  static int cache_read(URLContext *h, unsigned char *buf, int size)
     }
 
     r = ffurl_read(c->inner, buf, size);
-    if (r == 0 && size>0) {
+    if (r == AVERROR_EOF && size>0) {
         c->is_true_eof = 1;
         av_assert0(c->end >= c->logical_pos);
     }
@@ -263,7 +263,7 @@  resolve_eof:
                 if (whence == SEEK_SET)
                     size = FFMIN(sizeof(tmp), pos - c->logical_pos);
                 ret = cache_read(h, tmp, size);
-                if (ret == 0 && whence == SEEK_END) {
+                if (ret == AVERROR_EOF && whence == SEEK_END) {
                     av_assert0(c->is_true_eof);
                     goto resolve_eof;
                 }