[FFmpeg-devel,2/3] lavf/avio: temporarily accept 0 as EOF.

Submitted by Nicolas George on Oct. 25, 2017, 8:22 a.m.

Details

Message ID 20171025082258.32530-2-george@nsup.org
State Superseded
Headers show

Commit Message

Nicolas George Oct. 25, 2017, 8:22 a.m.
Print a warning to let applicatios fix their use.
After a deprecation period, check with a low-level assert.
Also make the constraint explicit in the doxygen comment.

Signed-off-by: Nicolas George <george@nsup.org>
---
 libavformat/avio.h    |  2 ++
 libavformat/aviobuf.c | 30 +++++++++++++++++++++---------
 libavformat/version.h |  3 +++
 3 files changed, 26 insertions(+), 9 deletions(-)

Comments

Jan Ekström Oct. 27, 2017, 4:48 p.m.
On Wed, Oct 25, 2017 at 11:22 AM, Nicolas George <george@nsup.org> wrote:
> Print a warning to let applicatios fix their use.
> After a deprecation period, check with a low-level assert.
> Also make the constraint explicit in the doxygen comment.
>
> Signed-off-by: Nicolas George <george@nsup.org>

Thanks for the patch.

> ---
>  libavformat/avio.h    |  2 ++
>  libavformat/aviobuf.c | 30 +++++++++++++++++++++---------
>  libavformat/version.h |  3 +++
>  3 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/libavformat/avio.h b/libavformat/avio.h
> index 19ecd96eb7..76ff7cd81e 100644
> --- a/libavformat/avio.h
> +++ b/libavformat/avio.h
> @@ -452,6 +452,8 @@ void avio_free_directory_entry(AVIODirEntry **entry);
>   * @param write_flag Set to 1 if the buffer should be writable, 0 otherwise.
>   * @param opaque An opaque pointer to user-specific data.
>   * @param read_packet  A function for refilling the buffer, may be NULL.
> + *                     For stream protocols, must never return 0 but rather
> + *                     a proper AVERROR code.

Otherwise OK, but since this is the first mention of "stream protocol"
in the repo and el goog gives me just "Internet Stream Protocol" so I
would like an explanation of what this means and how it connects to
"custom AVIO implementations/wrappers"?

>   * @param write_packet A function for writing the buffer contents, may be NULL.
>   *        The function may not change the input buffers content.
>   * @param seek A function for seeking to specified byte position, may be NULL.
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 3e9d774a13..bb5bcf7a14 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -524,6 +524,24 @@ void avio_write_marker(AVIOContext *s, int64_t time, enum AVIODataMarkerType typ
>      s->last_time = time;
>  }
>
> +static int read_packet_wrapper(AVIOContext *s, uint8_t *buf, int size)
> +{
> +    int ret;
> +
> +    if (!s->read_packet)
> +        return AVERROR_EOF;
> +    ret = s->read_packet(s->opaque, buf, size);
> +#if FF_API_OLD_AVIO_EOF_0
> +    if (!ret && !s->max_packet_size) {

Can you explain how you can utilize max_packet_size as a note
regarding if zero is EOF or not? Is it just a variable that happens to
be set to zero with custom AVIO contexts, or is there some other logic
behind this?

> +        av_log(s, AV_LOG_WARNING, "Invalid return value 0 for stream protocol\n");
> +        ret = AVERROR_EOF;
> +    }
> +#else
> +    av_assert2(ret || s->max_packet_size);
> +#endif
> +    return ret;
> +}
> +
>  /* Input stream */
>
>  static void fill_buffer(AVIOContext *s)
> @@ -562,10 +580,7 @@ static void fill_buffer(AVIOContext *s)
>          len = s->orig_buffer_size;
>      }
>
> -    if (s->read_packet)
> -        len = s->read_packet(s->opaque, dst, len);
> -    else
> -        len = AVERROR_EOF;
> +    len = read_packet_wrapper(s, dst, len);
>      if (len == AVERROR_EOF) {
>          /* do not modify buffer if EOF reached so that a seek back can
>             be done without rereading data */
> @@ -638,10 +653,7 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size)
>          if (len == 0 || s->write_flag) {
>              if((s->direct || size > s->buffer_size) && !s->update_checksum) {
>                  // bypass the buffer and read data directly into buf
> -                if(s->read_packet)
> -                    len = s->read_packet(s->opaque, buf, size);
> -                else
> -                    len = AVERROR_EOF;
> +                len = read_packet_wrapper(s, buf, size);
>                  if (len == AVERROR_EOF) {
>                      /* do not modify buffer if EOF reached so that a seek back can
>                      be done without rereading data */
> @@ -708,7 +720,7 @@ int avio_read_partial(AVIOContext *s, unsigned char *buf, int size)
>          return -1;
>
>      if (s->read_packet && s->write_flag) {
> -        len = s->read_packet(s->opaque, buf, size);
> +        len = read_packet_wrapper(s, buf, size);
>          if (len > 0)
>              s->pos += len;
>          return len;
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 0feb788c36..358ab91ab2 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -79,6 +79,9 @@
>  #ifndef FF_API_OLD_ROTATE_API
>  #define FF_API_OLD_ROTATE_API           (LIBAVFORMAT_VERSION_MAJOR < 59)
>  #endif
> +#ifndef FF_API_OLD_AVIO_EOF_0
> +#define FF_API_OLD_AVIO_EOF_0           (LIBAVFORMAT_VERSION_MAJOR < 59)
> +#endif
>
>
>  #ifndef FF_API_R_FRAME_RATE
> --
> 2.14.2

Otherwise LGTM. Will test in a moment.


Jan
Jan Ekström Oct. 27, 2017, 5:57 p.m.
On Wed, Oct 25, 2017 at 11:22 AM, Nicolas George <george@nsup.org> wrote:
> +static int read_packet_wrapper(AVIOContext *s, uint8_t *buf, int size)
> +{
> +    int ret;
> +
> +    if (!s->read_packet)
> +        return AVERROR_EOF;
> +    ret = s->read_packet(s->opaque, buf, size);
> +#if FF_API_OLD_AVIO_EOF_0
> +    if (!ret && !s->max_packet_size) {
> +        av_log(s, AV_LOG_WARNING, "Invalid return value 0 for stream protocol\n");
> +        ret = AVERROR_EOF;
> +    }
> +#else
> +    av_assert2(ret || s->max_packet_size);
> +#endif
> +    return ret;
> +}
> +

Built and tested locally and the effect seems to be the one wished,
although it seems like it is complaining about the AVClass being
nullptr? Is this something inherent to custom AVIO contexts and it's
the client that's supposed to fix this? Log follows:

> [ffmpeg] av_log callback called with bad parameters (NULL AVClass).
> [ffmpeg] This is a bug in one of Libav/FFmpeg libraries used.
> [ffmpeg] Invalid return value 0 for stream protocol


Jan
Nicolas George Oct. 27, 2017, 6:30 p.m.
Le sextidi 6 brumaire, an CCXXVI, Jan Ekstrom a écrit :
> Built and tested locally and the effect seems to be the one wished,
> although it seems like it is complaining about the AVClass being
> nullptr? Is this something inherent to custom AVIO contexts and it's
> the client that's supposed to fix this? Log follows:
> 
> > [ffmpeg] av_log callback called with bad parameters (NULL AVClass).
> > [ffmpeg] This is a bug in one of Libav/FFmpeg libraries used.
> > [ffmpeg] Invalid return value 0 for stream protocol

Thanks. It seems custom AVIOContext are not real AVIOContext in the full
meaning, and there are glitches in fixing that.

I have locally changed the log line:

	av_log(NULL, AV_LOG_WARNING, "Invalid
	       ^^^^

Not completely satisfactory, but enough for a library-to-application
warning.

Shall I send the whole series again?

Regards,
Nicolas George Oct. 27, 2017, 6:33 p.m.
Le sextidi 6 brumaire, an CCXXVI, Jan Ekstrom a écrit :
> Otherwise OK, but since this is the first mention of "stream protocol"
> in the repo and el goog gives me just "Internet Stream Protocol" so I
> would like an explanation of what this means and how it connects to
> "custom AVIO implementations/wrappers"?

It is standard parlance in networking: stream protocols produce a stream
of octets, without any additional structure, while packet protocols
produce packets, which are delimited at protocol level and visible by
the application, and can be empty.

FFmpeg distinguishes packet protocols with the max_packet_size: if it is
0, it is a stream protocol, if not a packet protocol.

Regards,
Jan Ekström Oct. 27, 2017, 6:40 p.m.
On Fri, Oct 27, 2017 at 9:33 PM, Nicolas George <george@nsup.org> wrote:
> It is standard parlance in networking: stream protocols produce a stream
> of octets, without any additional structure, while packet protocols
> produce packets, which are delimited at protocol level and visible by
> the application, and can be empty.
>

Gotcha. Seems like it's called "/* default: stream file */" in the
current code base, which is why I couldn't find references to stream
protocols.

> FFmpeg distinguishes packet protocols with the max_packet_size: if it is
> 0, it is a stream protocol, if not a packet protocol.

Gotcha^2.

Please send the patch-set v2 with the related fixes (the av_log in 2/3
and the correct ret in 3/3) as with that I think this all looks good
to go :) .

Jan

Patch hide | download patch | download mbox

diff --git a/libavformat/avio.h b/libavformat/avio.h
index 19ecd96eb7..76ff7cd81e 100644
--- a/libavformat/avio.h
+++ b/libavformat/avio.h
@@ -452,6 +452,8 @@  void avio_free_directory_entry(AVIODirEntry **entry);
  * @param write_flag Set to 1 if the buffer should be writable, 0 otherwise.
  * @param opaque An opaque pointer to user-specific data.
  * @param read_packet  A function for refilling the buffer, may be NULL.
+ *                     For stream protocols, must never return 0 but rather
+ *                     a proper AVERROR code.
  * @param write_packet A function for writing the buffer contents, may be NULL.
  *        The function may not change the input buffers content.
  * @param seek A function for seeking to specified byte position, may be NULL.
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 3e9d774a13..bb5bcf7a14 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -524,6 +524,24 @@  void avio_write_marker(AVIOContext *s, int64_t time, enum AVIODataMarkerType typ
     s->last_time = time;
 }
 
+static int read_packet_wrapper(AVIOContext *s, uint8_t *buf, int size)
+{
+    int ret;
+
+    if (!s->read_packet)
+        return AVERROR_EOF;
+    ret = s->read_packet(s->opaque, buf, size);
+#if FF_API_OLD_AVIO_EOF_0
+    if (!ret && !s->max_packet_size) {
+        av_log(s, AV_LOG_WARNING, "Invalid return value 0 for stream protocol\n");
+        ret = AVERROR_EOF;
+    }
+#else
+    av_assert2(ret || s->max_packet_size);
+#endif
+    return ret;
+}
+
 /* Input stream */
 
 static void fill_buffer(AVIOContext *s)
@@ -562,10 +580,7 @@  static void fill_buffer(AVIOContext *s)
         len = s->orig_buffer_size;
     }
 
-    if (s->read_packet)
-        len = s->read_packet(s->opaque, dst, len);
-    else
-        len = AVERROR_EOF;
+    len = read_packet_wrapper(s, dst, len);
     if (len == AVERROR_EOF) {
         /* do not modify buffer if EOF reached so that a seek back can
            be done without rereading data */
@@ -638,10 +653,7 @@  int avio_read(AVIOContext *s, unsigned char *buf, int size)
         if (len == 0 || s->write_flag) {
             if((s->direct || size > s->buffer_size) && !s->update_checksum) {
                 // bypass the buffer and read data directly into buf
-                if(s->read_packet)
-                    len = s->read_packet(s->opaque, buf, size);
-                else
-                    len = AVERROR_EOF;
+                len = read_packet_wrapper(s, buf, size);
                 if (len == AVERROR_EOF) {
                     /* do not modify buffer if EOF reached so that a seek back can
                     be done without rereading data */
@@ -708,7 +720,7 @@  int avio_read_partial(AVIOContext *s, unsigned char *buf, int size)
         return -1;
 
     if (s->read_packet && s->write_flag) {
-        len = s->read_packet(s->opaque, buf, size);
+        len = read_packet_wrapper(s, buf, size);
         if (len > 0)
             s->pos += len;
         return len;
diff --git a/libavformat/version.h b/libavformat/version.h
index 0feb788c36..358ab91ab2 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -79,6 +79,9 @@ 
 #ifndef FF_API_OLD_ROTATE_API
 #define FF_API_OLD_ROTATE_API           (LIBAVFORMAT_VERSION_MAJOR < 59)
 #endif
+#ifndef FF_API_OLD_AVIO_EOF_0
+#define FF_API_OLD_AVIO_EOF_0           (LIBAVFORMAT_VERSION_MAJOR < 59)
+#endif
 
 
 #ifndef FF_API_R_FRAME_RATE