diff mbox series

[FFmpeg-devel,4/5] avformat: make AVFormatContext io_close return an int

Message ID 20211130004950.30697-4-cus@passwd.hu
State New
Headers show
Series [FFmpeg-devel,1/5] avformat/file: use proper return value in file_close
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Marton Balint Nov. 30, 2021, 12:49 a.m. UTC
Otherwise there is no way to detect any error during avio_close().

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 doc/APIchanges         | 3 +++
 libavformat/avformat.h | 6 +++++-
 libavformat/internal.h | 6 +++++-
 libavformat/options.c  | 4 ++--
 libavformat/utils.c    | 6 ++++--
 libavformat/version.h  | 4 ++--
 6 files changed, 21 insertions(+), 8 deletions(-)

Comments

Andreas Rheinhardt Nov. 30, 2021, 8:07 a.m. UTC | #1
Marton Balint:
> Otherwise there is no way to detect any error during avio_close().
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  doc/APIchanges         | 3 +++
>  libavformat/avformat.h | 6 +++++-
>  libavformat/internal.h | 6 +++++-
>  libavformat/options.c  | 4 ++--
>  libavformat/utils.c    | 6 ++++--
>  libavformat/version.h  | 4 ++--
>  6 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index bc9f4e38da..090263aedf 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>  
>  API changes, most recent first:
>  
> +2021-12-xx - xxxxxxxxxx - lavf 59.10.100 - avformat.h
> +  AVFormatContext io_open now returns an int.
> +
>  2021-11-22 - xxxxxxxxxx - lavu 57.9.100 - pixfmt.h
>    Add AV_PIX_FMT_P210, AV_PIX_FMT_P410, AV_PIX_FMT_P216, and AV_PIX_FMT_P416.
>  
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 75699f3a32..eec1f6b20c 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1756,8 +1756,12 @@ typedef struct AVFormatContext {
>  
>      /**
>       * A callback for closing the streams opened with AVFormatContext.io_open().
> +     *
> +     * @param s the format context
> +     * @param pb IO context to be closed and freed
> +     * @return 0 on success, a negative AVERROR code on failure
>       */
> -    void (*io_close)(struct AVFormatContext *s, AVIOContext *pb);
> +    int (*io_close)(struct AVFormatContext *s, AVIOContext *pb);
>  
>      /**
>       * ',' separated list of disallowed protocols.
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 20e93d9267..4f28885f3c 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -905,8 +905,12 @@ int ff_format_output_open(AVFormatContext *s, const char *url, AVDictionary **op
>  /*
>   * A wrapper around AVFormatContext.io_close that should be used
>   * instead of calling the pointer directly.
> + *
> + * @param s AVFormatContext
> + * @param *pb the AVIOContext to be closed and freed. Can be NULL.
> + * @return >=0 on success, negative AVERROR in case of failure
>   */
> -void ff_format_io_close(AVFormatContext *s, AVIOContext **pb);
> +int ff_format_io_close(AVFormatContext *s, AVIOContext **pb);
>  
>  /**
>   * Utility function to check if the file uses http or https protocol
> diff --git a/libavformat/options.c b/libavformat/options.c
> index 72c9bdcefe..15f0c9e90c 100644
> --- a/libavformat/options.c
> +++ b/libavformat/options.c
> @@ -146,9 +146,9 @@ static int io_open_default(AVFormatContext *s, AVIOContext **pb,
>      return ffio_open_whitelist(pb, url, flags, &s->interrupt_callback, options, s->protocol_whitelist, s->protocol_blacklist);
>  }
>  
> -static void io_close_default(AVFormatContext *s, AVIOContext *pb)
> +static int io_close_default(AVFormatContext *s, AVIOContext *pb)
>  {
> -    avio_close(pb);
> +    return avio_close(pb);
>  }
>  
>  AVFormatContext *avformat_alloc_context(void)
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 7840e8717c..db5ef46dc2 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -1829,11 +1829,13 @@ int ff_format_output_open(AVFormatContext *s, const char *url, AVDictionary **op
>      return 0;
>  }
>  
> -void ff_format_io_close(AVFormatContext *s, AVIOContext **pb)
> +int ff_format_io_close(AVFormatContext *s, AVIOContext **pb)
>  {
> +    int ret = 0;
>      if (*pb)
> -        s->io_close(s, *pb);
> +        ret = s->io_close(s, *pb);
>      *pb = NULL;
> +    return ret;
>  }
>  
>  int ff_is_http_proto(const char *filename) {
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 0705ee4112..1623835a78 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -32,8 +32,8 @@
>  // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
>  // Also please add any ticket numbers that you believe might be affected here
>  #define LIBAVFORMAT_VERSION_MAJOR  59
> -#define LIBAVFORMAT_VERSION_MINOR   9
> -#define LIBAVFORMAT_VERSION_MICRO 102
> +#define LIBAVFORMAT_VERSION_MINOR  10
> +#define LIBAVFORMAT_VERSION_MICRO 100
>  
>  #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
>                                                 LIBAVFORMAT_VERSION_MINOR, \
> 

This is an API break.

- Andreas
Marton Balint Nov. 30, 2021, 9:07 a.m. UTC | #2
On Tue, 30 Nov 2021, Andreas Rheinhardt wrote:

> Marton Balint:
>> Otherwise there is no way to detect any error during avio_close().
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  doc/APIchanges         | 3 +++
>>  libavformat/avformat.h | 6 +++++-
>>  libavformat/internal.h | 6 +++++-
>>  libavformat/options.c  | 4 ++--
>>  libavformat/utils.c    | 6 ++++--
>>  libavformat/version.h  | 4 ++--
>>  6 files changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index bc9f4e38da..090263aedf 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>>
>>  API changes, most recent first:
>>
>> +2021-12-xx - xxxxxxxxxx - lavf 59.10.100 - avformat.h
>> +  AVFormatContext io_open now returns an int.
>> +
>>  2021-11-22 - xxxxxxxxxx - lavu 57.9.100 - pixfmt.h
>>    Add AV_PIX_FMT_P210, AV_PIX_FMT_P410, AV_PIX_FMT_P216, and AV_PIX_FMT_P416.
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 75699f3a32..eec1f6b20c 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -1756,8 +1756,12 @@ typedef struct AVFormatContext {
>>
>>      /**
>>       * A callback for closing the streams opened with AVFormatContext.io_open().
>> +     *
>> +     * @param s the format context
>> +     * @param pb IO context to be closed and freed
>> +     * @return 0 on success, a negative AVERROR code on failure
>>       */
>> -    void (*io_close)(struct AVFormatContext *s, AVIOContext *pb);
>> +    int (*io_close)(struct AVFormatContext *s, AVIOContext *pb);
>>
>>      /**
>>       * ',' separated list of disallowed protocols.
>
> This is an API break.

To be honest I wanted to avoid the io_close2() dance. This change has 
limited impact and we still did not have a release since the bump.

I can rework if somebody feels that introducing io_close2() is the better 
way. Please let me know.

Thanks,
Marton
Anton Khirnov Dec. 1, 2021, 11:35 a.m. UTC | #3
Quoting Marton Balint (2021-11-30 10:07:21)
> 
> 
> On Tue, 30 Nov 2021, Andreas Rheinhardt wrote:
> 
> > Marton Balint:
> >> Otherwise there is no way to detect any error during avio_close().
> >>
> >> Signed-off-by: Marton Balint <cus@passwd.hu>
> >> ---
> >>  doc/APIchanges         | 3 +++
> >>  libavformat/avformat.h | 6 +++++-
> >>  libavformat/internal.h | 6 +++++-
> >>  libavformat/options.c  | 4 ++--
> >>  libavformat/utils.c    | 6 ++++--
> >>  libavformat/version.h  | 4 ++--
> >>  6 files changed, 21 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/doc/APIchanges b/doc/APIchanges
> >> index bc9f4e38da..090263aedf 100644
> >> --- a/doc/APIchanges
> >> +++ b/doc/APIchanges
> >> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
> >>
> >>  API changes, most recent first:
> >>
> >> +2021-12-xx - xxxxxxxxxx - lavf 59.10.100 - avformat.h
> >> +  AVFormatContext io_open now returns an int.
> >> +
> >>  2021-11-22 - xxxxxxxxxx - lavu 57.9.100 - pixfmt.h
> >>    Add AV_PIX_FMT_P210, AV_PIX_FMT_P410, AV_PIX_FMT_P216, and AV_PIX_FMT_P416.
> >>
> >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> >> index 75699f3a32..eec1f6b20c 100644
> >> --- a/libavformat/avformat.h
> >> +++ b/libavformat/avformat.h
> >> @@ -1756,8 +1756,12 @@ typedef struct AVFormatContext {
> >>
> >>      /**
> >>       * A callback for closing the streams opened with AVFormatContext.io_open().
> >> +     *
> >> +     * @param s the format context
> >> +     * @param pb IO context to be closed and freed
> >> +     * @return 0 on success, a negative AVERROR code on failure
> >>       */
> >> -    void (*io_close)(struct AVFormatContext *s, AVIOContext *pb);
> >> +    int (*io_close)(struct AVFormatContext *s, AVIOContext *pb);
> >>
> >>      /**
> >>       * ',' separated list of disallowed protocols.
> >
> > This is an API break.
> 
> To be honest I wanted to avoid the io_close2() dance. This change has 
> limited impact and we still did not have a release since the bump.
> 
> I can rework if somebody feels that introducing io_close2() is the better 
> way. Please let me know.

I do. I also feel very uncomfortable with arguments along the lines of
"breaking things outside of releases is ok". We've always considered git
master to be stable (except for a _short_ period after a major bump).
Hendrik Leppkes Dec. 4, 2021, 8:04 p.m. UTC | #4
On Sat, Dec 4, 2021 at 9:00 PM Marton Balint <cus@passwd.hu> wrote:
>
> Otherwise there is no way to detect any error during avio_close().
>

Returning errors is nice and all, but whats the expected reaction to
an error here, since we're already trying to close? Can't exactly
close and go home (since thats what you just tried) like you would on
an error to open.

- Hendrik
Marton Balint Dec. 4, 2021, 8:12 p.m. UTC | #5
On Sat, 4 Dec 2021, Hendrik Leppkes wrote:

> On Sat, Dec 4, 2021 at 9:00 PM Marton Balint <cus@passwd.hu> wrote:
>>
>> Otherwise there is no way to detect any error during avio_close().
>>
>
> Returning errors is nice and all, but whats the expected reaction to
> an error here, since we're already trying to close? Can't exactly
> close and go home (since thats what you just tried) like you would on
> an error to open.

It is probably not very useful for the read case, it is paramount however 
for the write case. To be sure you successfully wrote your data you need 
to check the return status.

Regards,
Marton
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index bc9f4e38da..090263aedf 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,9 @@  libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2021-12-xx - xxxxxxxxxx - lavf 59.10.100 - avformat.h
+  AVFormatContext io_open now returns an int.
+
 2021-11-22 - xxxxxxxxxx - lavu 57.9.100 - pixfmt.h
   Add AV_PIX_FMT_P210, AV_PIX_FMT_P410, AV_PIX_FMT_P216, and AV_PIX_FMT_P416.
 
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 75699f3a32..eec1f6b20c 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -1756,8 +1756,12 @@  typedef struct AVFormatContext {
 
     /**
      * A callback for closing the streams opened with AVFormatContext.io_open().
+     *
+     * @param s the format context
+     * @param pb IO context to be closed and freed
+     * @return 0 on success, a negative AVERROR code on failure
      */
-    void (*io_close)(struct AVFormatContext *s, AVIOContext *pb);
+    int (*io_close)(struct AVFormatContext *s, AVIOContext *pb);
 
     /**
      * ',' separated list of disallowed protocols.
diff --git a/libavformat/internal.h b/libavformat/internal.h
index 20e93d9267..4f28885f3c 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -905,8 +905,12 @@  int ff_format_output_open(AVFormatContext *s, const char *url, AVDictionary **op
 /*
  * A wrapper around AVFormatContext.io_close that should be used
  * instead of calling the pointer directly.
+ *
+ * @param s AVFormatContext
+ * @param *pb the AVIOContext to be closed and freed. Can be NULL.
+ * @return >=0 on success, negative AVERROR in case of failure
  */
-void ff_format_io_close(AVFormatContext *s, AVIOContext **pb);
+int ff_format_io_close(AVFormatContext *s, AVIOContext **pb);
 
 /**
  * Utility function to check if the file uses http or https protocol
diff --git a/libavformat/options.c b/libavformat/options.c
index 72c9bdcefe..15f0c9e90c 100644
--- a/libavformat/options.c
+++ b/libavformat/options.c
@@ -146,9 +146,9 @@  static int io_open_default(AVFormatContext *s, AVIOContext **pb,
     return ffio_open_whitelist(pb, url, flags, &s->interrupt_callback, options, s->protocol_whitelist, s->protocol_blacklist);
 }
 
-static void io_close_default(AVFormatContext *s, AVIOContext *pb)
+static int io_close_default(AVFormatContext *s, AVIOContext *pb)
 {
-    avio_close(pb);
+    return avio_close(pb);
 }
 
 AVFormatContext *avformat_alloc_context(void)
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 7840e8717c..db5ef46dc2 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -1829,11 +1829,13 @@  int ff_format_output_open(AVFormatContext *s, const char *url, AVDictionary **op
     return 0;
 }
 
-void ff_format_io_close(AVFormatContext *s, AVIOContext **pb)
+int ff_format_io_close(AVFormatContext *s, AVIOContext **pb)
 {
+    int ret = 0;
     if (*pb)
-        s->io_close(s, *pb);
+        ret = s->io_close(s, *pb);
     *pb = NULL;
+    return ret;
 }
 
 int ff_is_http_proto(const char *filename) {
diff --git a/libavformat/version.h b/libavformat/version.h
index 0705ee4112..1623835a78 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -32,8 +32,8 @@ 
 // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
 // Also please add any ticket numbers that you believe might be affected here
 #define LIBAVFORMAT_VERSION_MAJOR  59
-#define LIBAVFORMAT_VERSION_MINOR   9
-#define LIBAVFORMAT_VERSION_MICRO 102
+#define LIBAVFORMAT_VERSION_MINOR  10
+#define LIBAVFORMAT_VERSION_MICRO 100
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
                                                LIBAVFORMAT_VERSION_MINOR, \