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

[Jan Ekstrom]

On Wed, Oct 25, 2017 at 11:22 AM, Nicolas George <george at 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 at 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