[FFmpeg-devel] [PATCH 1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM

[FFmpeg-devel] [PATCH 1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM

[Marton Balint]

This makes sure the error condition is kept in AVIOContext even if the user
does not check the return value of avio_read_to_bprint or
ff_read_line_to_bprint.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/aviobuf.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 29d4bd7510..6f8a822ee3 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -875,8 +875,10 @@ static int64_t read_string_to_bprint_overwrite(AVIOContext *s, AVBPrint *bp,
     if (ret < 0)
         return ret;
 
-    if (!av_bprint_is_complete(bp))
+    if (!av_bprint_is_complete(bp)) {
+        s->error = AVERROR(ENOMEM);
         return AVERROR(ENOMEM);
+    }
 
     return bp->len;
 }
@@ -1351,8 +1353,10 @@ int avio_read_to_bprint(AVIOContext *h, AVBPrint *pb, size_t max_size)
         if (ret <= 0)
             return ret;
         av_bprint_append_data(pb, buf, ret);
-        if (!av_bprint_is_complete(pb))
+        if (!av_bprint_is_complete(pb)) {
+            h->error = AVERROR(ENOMEM);
             return AVERROR(ENOMEM);
+        }
         max_size -= ret;
     }
     return 0;
-- 
2.31.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/5] avformat/movenc: factorize data shifting

[Marton Balint]

And move data shift function from movenc to utils.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/internal.h |  7 ++++++
 libavformat/movenc.c   | 55 ++---------------------------------------
 libavformat/utils.c    | 56 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 53 deletions(-)

diff --git a/libavformat/internal.h b/libavformat/internal.h
index 2ba795d669..63235ce5cf 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -1019,4 +1019,11 @@ void ff_format_set_url(AVFormatContext *s, char *url);
 
 void avpriv_register_devices(const AVOutputFormat * const o[], const AVInputFormat * const i[]);
 
+/**
+ * Make shift_size amount of space at read_start by shifting data in the output
+ * at read_start until the current IO position. The underlying IO context must
+ * be seekable.
+ */
+int ff_format_shift_data(AVFormatContext *s, int64_t read_start, int shift_size);
+
 #endif /* AVFORMAT_INTERNAL_H */
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 0f912dd012..40ad4f8642 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -7150,13 +7150,8 @@ static int compute_sidx_size(AVFormatContext *s)
 
 static int shift_data(AVFormatContext *s)
 {
-    int ret = 0, moov_size;
+    int moov_size;
     MOVMuxContext *mov = s->priv_data;
-    int64_t pos, pos_end;
-    uint8_t *buf, *read_buf[2];
-    int read_buf_id = 0;
-    int read_size[2];
-    AVIOContext *read_pb;
 
     if (mov->flags & FF_MOV_FLAG_FRAGMENT)
         moov_size = compute_sidx_size(s);
@@ -7165,53 +7160,7 @@ static int shift_data(AVFormatContext *s)
     if (moov_size < 0)
         return moov_size;
 
-    buf = av_malloc(moov_size * 2);
-    if (!buf)
-        return AVERROR(ENOMEM);
-    read_buf[0] = buf;
-    read_buf[1] = buf + moov_size;
-
-    /* Shift the data: the AVIO context of the output can only be used for
-     * writing, so we re-open the same output, but for reading. It also avoids
-     * a read/seek/write/seek back and forth. */
-    avio_flush(s->pb);
-    ret = s->io_open(s, &read_pb, s->url, AVIO_FLAG_READ, NULL);
-    if (ret < 0) {
-        av_log(s, AV_LOG_ERROR, "Unable to re-open %s output file for "
-               "the second pass (faststart)\n", s->url);
-        goto end;
-    }
-
-    /* mark the end of the shift to up to the last data we wrote, and get ready
-     * for writing */
-    pos_end = avio_tell(s->pb);
-    avio_seek(s->pb, mov->reserved_header_pos + moov_size, SEEK_SET);
-
-    /* start reading at where the new moov will be placed */
-    avio_seek(read_pb, mov->reserved_header_pos, SEEK_SET);
-    pos = avio_tell(read_pb);
-
-#define READ_BLOCK do {                                                             \
-    read_size[read_buf_id] = avio_read(read_pb, read_buf[read_buf_id], moov_size);  \
-    read_buf_id ^= 1;                                                               \
-} while (0)
-
-    /* shift data by chunk of at most moov_size */
-    READ_BLOCK;
-    do {
-        int n;
-        READ_BLOCK;
-        n = read_size[read_buf_id];
-        if (n <= 0)
-            break;
-        avio_write(s->pb, read_buf[read_buf_id], n);
-        pos += n;
-    } while (pos < pos_end);
-    ff_format_io_close(s, &read_pb);
-
-end:
-    av_free(buf);
-    return ret;
+    return ff_format_shift_data(s, mov->reserved_header_pos, moov_size);
 }
 
 static int mov_write_trailer(AVFormatContext *s)
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 332ba534d2..a78797ef57 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -2035,3 +2035,59 @@ const char *av_disposition_to_string(int disposition)
 
     return NULL;
 }
+
+int ff_format_shift_data(AVFormatContext *s, int64_t read_start, int shift_size)
+{
+    int ret;
+    int64_t pos, pos_end;
+    uint8_t *buf, *read_buf[2];
+    int read_buf_id = 0;
+    int read_size[2];
+    AVIOContext *read_pb;
+
+    buf = av_malloc_array(shift_size, 2);
+    if (!buf)
+        return AVERROR(ENOMEM);
+    read_buf[0] = buf;
+    read_buf[1] = buf + shift_size;
+
+    /* Shift the data: the AVIO context of the output can only be used for
+     * writing, so we re-open the same output, but for reading. It also avoids
+     * a read/seek/write/seek back and forth. */
+    avio_flush(s->pb);
+    ret = s->io_open(s, &read_pb, s->url, AVIO_FLAG_READ, NULL);
+    if (ret < 0) {
+        av_log(s, AV_LOG_ERROR, "Unable to re-open %s output file for shifting data\n", s->url);
+        goto end;
+    }
+
+    /* mark the end of the shift to up to the last data we wrote, and get ready
+     * for writing */
+    pos_end = avio_tell(s->pb);
+    avio_seek(s->pb, read_start + shift_size, SEEK_SET);
+
+    avio_seek(read_pb, read_start, SEEK_SET);
+    pos = avio_tell(read_pb);
+
+#define READ_BLOCK do {                                                             \
+    read_size[read_buf_id] = avio_read(read_pb, read_buf[read_buf_id], shift_size);  \
+    read_buf_id ^= 1;                                                               \
+} while (0)
+
+    /* shift data by chunk of at most shift_size */
+    READ_BLOCK;
+    do {
+        int n;
+        READ_BLOCK;
+        n = read_size[read_buf_id];
+        if (n <= 0)
+            break;
+        avio_write(s->pb, read_buf[read_buf_id], n);
+        pos += n;
+    } while (pos < pos_end);
+    ff_format_io_close(s, &read_pb);
+
+end:
+    av_free(buf);
+    return ret;
+}
-- 
2.31.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/5] avformat/movenc: factorize data shifting

[Andreas Rheinhardt]

Marton Balint:
> And move data shift function from movenc to utils.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/internal.h |  7 ++++++
>  libavformat/movenc.c   | 55 ++---------------------------------------
>  libavformat/utils.c    | 56 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+), 53 deletions(-)
> 
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 2ba795d669..63235ce5cf 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -1019,4 +1019,11 @@ void ff_format_set_url(AVFormatContext *s, char *url);
>  
>  void avpriv_register_devices(const AVOutputFormat * const o[], const AVInputFormat * const i[]);
>  
> +/**
> + * Make shift_size amount of space at read_start by shifting data in the output
> + * at read_start until the current IO position. The underlying IO context must
> + * be seekable.
> + */
> +int ff_format_shift_data(AVFormatContext *s, int64_t read_start, int shift_size);
> +
>  #endif /* AVFORMAT_INTERNAL_H */
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 0f912dd012..40ad4f8642 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -7150,13 +7150,8 @@ static int compute_sidx_size(AVFormatContext *s)
>  
>  static int shift_data(AVFormatContext *s)
>  {
> -    int ret = 0, moov_size;
> +    int moov_size;
>      MOVMuxContext *mov = s->priv_data;
> -    int64_t pos, pos_end;
> -    uint8_t *buf, *read_buf[2];
> -    int read_buf_id = 0;
> -    int read_size[2];
> -    AVIOContext *read_pb;
>  
>      if (mov->flags & FF_MOV_FLAG_FRAGMENT)
>          moov_size = compute_sidx_size(s);
> @@ -7165,53 +7160,7 @@ static int shift_data(AVFormatContext *s)
>      if (moov_size < 0)
>          return moov_size;
>  
> -    buf = av_malloc(moov_size * 2);
> -    if (!buf)
> -        return AVERROR(ENOMEM);
> -    read_buf[0] = buf;
> -    read_buf[1] = buf + moov_size;
> -
> -    /* Shift the data: the AVIO context of the output can only be used for
> -     * writing, so we re-open the same output, but for reading. It also avoids
> -     * a read/seek/write/seek back and forth. */
> -    avio_flush(s->pb);
> -    ret = s->io_open(s, &read_pb, s->url, AVIO_FLAG_READ, NULL);
> -    if (ret < 0) {
> -        av_log(s, AV_LOG_ERROR, "Unable to re-open %s output file for "
> -               "the second pass (faststart)\n", s->url);
> -        goto end;
> -    }
> -
> -    /* mark the end of the shift to up to the last data we wrote, and get ready
> -     * for writing */
> -    pos_end = avio_tell(s->pb);
> -    avio_seek(s->pb, mov->reserved_header_pos + moov_size, SEEK_SET);
> -
> -    /* start reading at where the new moov will be placed */
> -    avio_seek(read_pb, mov->reserved_header_pos, SEEK_SET);
> -    pos = avio_tell(read_pb);
> -
> -#define READ_BLOCK do {                                                             \
> -    read_size[read_buf_id] = avio_read(read_pb, read_buf[read_buf_id], moov_size);  \
> -    read_buf_id ^= 1;                                                               \
> -} while (0)
> -
> -    /* shift data by chunk of at most moov_size */
> -    READ_BLOCK;
> -    do {
> -        int n;
> -        READ_BLOCK;
> -        n = read_size[read_buf_id];
> -        if (n <= 0)
> -            break;
> -        avio_write(s->pb, read_buf[read_buf_id], n);
> -        pos += n;
> -    } while (pos < pos_end);
> -    ff_format_io_close(s, &read_pb);
> -
> -end:
> -    av_free(buf);
> -    return ret;
> +    return ff_format_shift_data(s, mov->reserved_header_pos, moov_size);
>  }
>  
>  static int mov_write_trailer(AVFormatContext *s)
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 332ba534d2..a78797ef57 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -2035,3 +2035,59 @@ const char *av_disposition_to_string(int disposition)
>  
>      return NULL;
>  }
> +
> +int ff_format_shift_data(AVFormatContext *s, int64_t read_start, int shift_size)
> +{
> +    int ret;
> +    int64_t pos, pos_end;
> +    uint8_t *buf, *read_buf[2];
> +    int read_buf_id = 0;
> +    int read_size[2];
> +    AVIOContext *read_pb;
> +
> +    buf = av_malloc_array(shift_size, 2);
> +    if (!buf)
> +        return AVERROR(ENOMEM);
> +    read_buf[0] = buf;
> +    read_buf[1] = buf + shift_size;
> +
> +    /* Shift the data: the AVIO context of the output can only be used for
> +     * writing, so we re-open the same output, but for reading. It also avoids
> +     * a read/seek/write/seek back and forth. */
> +    avio_flush(s->pb);
> +    ret = s->io_open(s, &read_pb, s->url, AVIO_FLAG_READ, NULL);
> +    if (ret < 0) {
> +        av_log(s, AV_LOG_ERROR, "Unable to re-open %s output file for shifting data\n", s->url);
> +        goto end;
> +    }
> +
> +    /* mark the end of the shift to up to the last data we wrote, and get ready
> +     * for writing */
> +    pos_end = avio_tell(s->pb);
> +    avio_seek(s->pb, read_start + shift_size, SEEK_SET);
> +
> +    avio_seek(read_pb, read_start, SEEK_SET);
> +    pos = avio_tell(read_pb);

This avio_tell() is redundant: avio_seek() returns the new position on
non-error and in this case it is equal to read_start.

> +
> +#define READ_BLOCK do {                                                             \
> +    read_size[read_buf_id] = avio_read(read_pb, read_buf[read_buf_id], shift_size);  \
> +    read_buf_id ^= 1;                                                               \
> +} while (0)
> +
> +    /* shift data by chunk of at most shift_size */
> +    READ_BLOCK;
> +    do {
> +        int n;
> +        READ_BLOCK;
> +        n = read_size[read_buf_id];
> +        if (n <= 0)
> +            break;
> +        avio_write(s->pb, read_buf[read_buf_id], n);
> +        pos += n;
> +    } while (pos < pos_end);
> +    ff_format_io_close(s, &read_pb);
> +
> +end:
> +    av_free(buf);
> +    return ret;
> +}
> 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/5] avformat/flvenc: use ff_format_shift_data for data shifting

[Marton Balint]

add_keyframe_index seems to generate a corrupted index even before this change.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/flvenc.c | 59 +++++---------------------------------------
 1 file changed, 6 insertions(+), 53 deletions(-)

diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
index 5130d429ad..31d1d93c23 100644
--- a/libavformat/flvenc.c
+++ b/libavformat/flvenc.c
@@ -576,15 +576,9 @@ static int flv_append_keyframe_info(AVFormatContext *s, FLVContext *flv, double
 
 static int shift_data(AVFormatContext *s)
 {
-    int ret = 0;
-    int n = 0;
+    int ret;
     int64_t metadata_size = 0;
     FLVContext *flv = s->priv_data;
-    int64_t pos, pos_end = avio_tell(s->pb); /* Save the pre-shift size. */
-    uint8_t *buf, *read_buf[2];
-    int read_buf_id = 0;
-    int read_size[2];
-    AVIOContext *read_pb;
 
     metadata_size = flv->filepositions_count * 9 * 2 + 10; /* filepositions and times value */
     metadata_size += 2 + 13; /* filepositions String */
@@ -596,58 +590,17 @@ static int shift_data(AVFormatContext *s)
     if (metadata_size < 0)
         return metadata_size;
 
-    buf = av_malloc_array(metadata_size, 2);
-    if (!buf) {
-        return AVERROR(ENOMEM);
-    }
-    read_buf[0] = buf;
-    read_buf[1] = buf + metadata_size;
+    ret = ff_format_shift_data(s, flv->keyframes_info_offset, metadata_size);
+    if (ret < 0)
+        return ret;
 
     avio_seek(s->pb, flv->metadata_size_pos, SEEK_SET);
     avio_wb24(s->pb, flv->metadata_totalsize + metadata_size);
 
-    avio_seek(s->pb, flv->metadata_totalsize_pos, SEEK_SET);
+    avio_seek(s->pb, flv->metadata_totalsize_pos + metadata_size, SEEK_SET);
     avio_wb32(s->pb, flv->metadata_totalsize + 11 + metadata_size);
 
-    /* Shift the data: the AVIO context of the output can only be used for
-     * writing, so we re-open the same output, but for reading. It also avoids
-     * a read/seek/write/seek back and forth. */
-    avio_flush(s->pb);
-    ret = s->io_open(s, &read_pb, s->url, AVIO_FLAG_READ, NULL);
-    if (ret < 0) {
-        av_log(s, AV_LOG_ERROR, "Unable to re-open %s output file for "
-               "the second pass (add_keyframe_index)\n", s->url);
-        goto end;
-    }
-
-    /* Get ready for writing. */
-    avio_seek(s->pb, flv->keyframes_info_offset + metadata_size, SEEK_SET);
-
-    /* start reading at where the keyframe index information will be placed */
-    avio_seek(read_pb, flv->keyframes_info_offset, SEEK_SET);
-    pos = avio_tell(read_pb);
-
-#define READ_BLOCK do {                                                             \
-    read_size[read_buf_id] = avio_read(read_pb, read_buf[read_buf_id], metadata_size);  \
-    read_buf_id ^= 1;                                                               \
-} while (0)
-
-    /* shift data by chunk of at most keyframe *filepositions* and *times* size */
-    READ_BLOCK;
-    do {
-        READ_BLOCK;
-        n = read_size[read_buf_id];
-        if (n < 0)
-            break;
-        avio_write(s->pb, read_buf[read_buf_id], n);
-        pos += n;
-    } while (pos <= pos_end);
-
-    ff_format_io_close(s, &read_pb);
-
-end:
-    av_free(buf);
-    return ret;
+    return 0;
 }
 
 static int flv_init(struct AVFormatContext *s)
-- 
2.31.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/5] avformat/segafilmenc: use ff_format_shift_data for shifting

[Marton Balint]

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/segafilmenc.c | 51 ++++-----------------------------------
 1 file changed, 5 insertions(+), 46 deletions(-)

diff --git a/libavformat/segafilmenc.c b/libavformat/segafilmenc.c
index ff8cb66aca..737805faa6 100644
--- a/libavformat/segafilmenc.c
+++ b/libavformat/segafilmenc.c
@@ -170,54 +170,13 @@ static int film_init(AVFormatContext *format_context)
 static int write_header(AVFormatContext *format_context, uint8_t *header,
                         unsigned header_size)
 {
-    int ret = 0;
-    int64_t pos, pos_end;
-    uint8_t *buf, *read_buf[2];
-    int read_buf_id = 0;
-    int read_size[2];
-    AVIOContext *read_pb;
-
-    buf = av_malloc(header_size);
-    if (!buf)
-        return AVERROR(ENOMEM);
-    read_buf[0] = buf;
-    read_buf[1]  = header;
-    read_size[1] = header_size;
-
-    /* Write the header at the beginning of the file, shifting all content as necessary;
-     * based on the approach used by MOV faststart. */
-    avio_flush(format_context->pb);
-    ret = format_context->io_open(format_context, &read_pb, format_context->url, AVIO_FLAG_READ, NULL);
-    if (ret < 0) {
-        av_log(format_context, AV_LOG_ERROR, "Unable to re-open %s output file to "
-               "write the header\n", format_context->url);
-        av_free(buf);
+    int ret = ff_format_shift_data(format_context, 0, header_size);
+    if (ret < 0)
         return ret;
-    }
 
-    /* Mark the end of the shift to up to the last data we are going to write,
-     * and get ready for writing */
-    pos_end = avio_tell(format_context->pb) + header_size;
-    pos = avio_seek(format_context->pb, 0, SEEK_SET);
-
-    /* start reading at where the new header will be placed */
-    avio_seek(read_pb, 0, SEEK_SET);
-
-    /* shift data by chunk of at most header_size */
-    do {
-        int n;
-        read_size[read_buf_id] = avio_read(read_pb, read_buf[read_buf_id],
-                                           header_size);
-        read_buf_id ^= 1;
-        n = read_size[read_buf_id];
-        if (n <= 0)
-            break;
-        avio_write(format_context->pb, read_buf[read_buf_id], n);
-        pos += n;
-    } while (pos < pos_end);
-    ff_format_io_close(format_context, &read_pb);
-
-    av_free(buf);
+    avio_seek(format_context->pb, 0, SEEK_SET);
+    avio_write(format_context->pb, header, header_size);
+
     return 0;
 }
 
-- 
2.31.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] avformat/segafilmenc: use ff_format_shift_data for shifting

[Andreas Rheinhardt]

Marton Balint:
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/segafilmenc.c | 51 ++++-----------------------------------
>  1 file changed, 5 insertions(+), 46 deletions(-)
> 
> diff --git a/libavformat/segafilmenc.c b/libavformat/segafilmenc.c
> index ff8cb66aca..737805faa6 100644
> --- a/libavformat/segafilmenc.c
> +++ b/libavformat/segafilmenc.c
> @@ -170,54 +170,13 @@ static int film_init(AVFormatContext *format_context)
>  static int write_header(AVFormatContext *format_context, uint8_t *header,
>                          unsigned header_size)
>  {
> -    int ret = 0;
> -    int64_t pos, pos_end;
> -    uint8_t *buf, *read_buf[2];
> -    int read_buf_id = 0;
> -    int read_size[2];
> -    AVIOContext *read_pb;
> -
> -    buf = av_malloc(header_size);
> -    if (!buf)
> -        return AVERROR(ENOMEM);
> -    read_buf[0] = buf;
> -    read_buf[1]  = header;
> -    read_size[1] = header_size;
> -
> -    /* Write the header at the beginning of the file, shifting all content as necessary;
> -     * based on the approach used by MOV faststart. */
> -    avio_flush(format_context->pb);
> -    ret = format_context->io_open(format_context, &read_pb, format_context->url, AVIO_FLAG_READ, NULL);
> -    if (ret < 0) {
> -        av_log(format_context, AV_LOG_ERROR, "Unable to re-open %s output file to "
> -               "write the header\n", format_context->url);
> -        av_free(buf);
> +    int ret = ff_format_shift_data(format_context, 0, header_size);
> +    if (ret < 0)
>          return ret;
> -    }
>  
> -    /* Mark the end of the shift to up to the last data we are going to write,
> -     * and get ready for writing */
> -    pos_end = avio_tell(format_context->pb) + header_size;
> -    pos = avio_seek(format_context->pb, 0, SEEK_SET);
> -
> -    /* start reading at where the new header will be placed */
> -    avio_seek(read_pb, 0, SEEK_SET);
> -
> -    /* shift data by chunk of at most header_size */
> -    do {
> -        int n;
> -        read_size[read_buf_id] = avio_read(read_pb, read_buf[read_buf_id],
> -                                           header_size);
> -        read_buf_id ^= 1;
> -        n = read_size[read_buf_id];
> -        if (n <= 0)
> -            break;
> -        avio_write(format_context->pb, read_buf[read_buf_id], n);
> -        pos += n;
> -    } while (pos < pos_end);
> -    ff_format_io_close(format_context, &read_pb);
> -
> -    av_free(buf);
> +    avio_seek(format_context->pb, 0, SEEK_SET);

This adds a new seek; this could be easily avoided if
ff_format_shift_data() would be changed to add a buffer parameter which
if != NULL would need to point to a buffer of size equal to shift_size
that is directly written at read_start.

> +    avio_write(format_context->pb, header, header_size);
> +
>      return 0;
>  }
>  
> 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 5/5] avformat/utils: propagate return value of ff_format_io_close in ff_format_shift_data

[Marton Balint]

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index a78797ef57..23e81bda9d 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -2085,7 +2085,7 @@ int ff_format_shift_data(AVFormatContext *s, int64_t read_start, int shift_size)
         avio_write(s->pb, read_buf[read_buf_id], n);
         pos += n;
     } while (pos < pos_end);
-    ff_format_io_close(s, &read_pb);
+    ret = ff_format_io_close(s, &read_pb);
 
 end:
     av_free(buf);
-- 
2.31.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM

[Andreas Rheinhardt]

Marton Balint:
> This makes sure the error condition is kept in AVIOContext even if the user
> does not check the return value of avio_read_to_bprint or
> ff_read_line_to_bprint.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/aviobuf.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 29d4bd7510..6f8a822ee3 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -875,8 +875,10 @@ static int64_t read_string_to_bprint_overwrite(AVIOContext *s, AVBPrint *bp,
>      if (ret < 0)
>          return ret;
>  
> -    if (!av_bprint_is_complete(bp))
> +    if (!av_bprint_is_complete(bp)) {
> +        s->error = AVERROR(ENOMEM);
>          return AVERROR(ENOMEM);
> +    }
>  
>      return bp->len;
>  }
> @@ -1351,8 +1353,10 @@ int avio_read_to_bprint(AVIOContext *h, AVBPrint *pb, size_t max_size)
>          if (ret <= 0)
>              return ret;
>          av_bprint_append_data(pb, buf, ret);
> -        if (!av_bprint_is_complete(pb))
> +        if (!av_bprint_is_complete(pb)) {
> +            h->error = AVERROR(ENOMEM);
>              return AVERROR(ENOMEM);
> +        }
>          max_size -= ret;
>      }
>      return 0;
> 

I don't really see the point of this: It is not a real read error that
should stick to the AVIOContext (which can still be used afterwards
without any issue). If the user does not check the errors, then the user
has no one to blame but himself for missing errors.

- Andreas

PS: If the AVBPrint API had a documented way of marking data as used,
one could avoid those stack buffers and use the AVBPrint buffer directly
with av_bprint_get_buffer(). (Marking data as used would be equivalent
to incrementing len and ensuring that the buffer stays zero-terminated.)
If this were done, no already read data would be lost in case of a later
allocation failure.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM

[Marton Balint]

On Fri, 31 Dec 2021, Andreas Rheinhardt wrote:

> Marton Balint:
>> This makes sure the error condition is kept in AVIOContext even if the user
>> does not check the return value of avio_read_to_bprint or
>> ff_read_line_to_bprint.
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/aviobuf.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>> index 29d4bd7510..6f8a822ee3 100644
>> --- a/libavformat/aviobuf.c
>> +++ b/libavformat/aviobuf.c
>> @@ -875,8 +875,10 @@ static int64_t read_string_to_bprint_overwrite(AVIOContext *s, AVBPrint *bp,
>>      if (ret < 0)
>>          return ret;
>>
>> -    if (!av_bprint_is_complete(bp))
>> +    if (!av_bprint_is_complete(bp)) {
>> +        s->error = AVERROR(ENOMEM);
>>          return AVERROR(ENOMEM);
>> +    }
>>
>>      return bp->len;
>>  }
>> @@ -1351,8 +1353,10 @@ int avio_read_to_bprint(AVIOContext *h, AVBPrint *pb, size_t max_size)
>>          if (ret <= 0)
>>              return ret;
>>          av_bprint_append_data(pb, buf, ret);
>> -        if (!av_bprint_is_complete(pb))
>> +        if (!av_bprint_is_complete(pb)) {
>> +            h->error = AVERROR(ENOMEM);
>>              return AVERROR(ENOMEM);
>> +        }
>>          max_size -= ret;
>>      }
>>      return 0;
>>
>
> I don't really see the point of this: It is not a real read error that
> should stick to the AVIOContext (which can still be used afterwards
> without any issue).
> If the user does not check the errors, then the user
> has no one to blame but himself for missing errors.

AVIO read/write behaviour is to store IO errors in the context so the user 
does not have to check for them in every call. It is not well documented 
which calls should be checked always, so the user might be under the 
impression that errors during read/write may be checked sometime 
later.

Admittedly, ENOMEM is not an IO error, but I think it is better to store 
that as well in the context to keep the behaviour consistent, because 
in case of ENOMEM avio_read_to_bprint reads and drops undefined amount of 
data, so the context will also be in an undefined state.

Other possibilities:
  - make avio_read_to_bprint read all the data regardless of AVBPrint 
fullness
  - mark avio_read_to_bprint av_warn_unused_result.
  - both :)

But these also forces the user to check return values... So I kind of like 
my original approach better, because it maintains avio_read/write call 
behaviour that it is safe to check errors sometime later.

Regards,
Marton


>
> - Andreas
>
> PS: If the AVBPrint API had a documented way of marking data as used,
> one could avoid those stack buffers and use the AVBPrint buffer directly
> with av_bprint_get_buffer(). (Marking data as used would be equivalent
> to incrementing len and ensuring that the buffer stays zero-terminated.)
> If this were done, no already read data would be lost in case of a later
> allocation failure.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM

[Marton Balint]

On Fri, 31 Dec 2021, Marton Balint wrote:

>
>
> On Fri, 31 Dec 2021, Andreas Rheinhardt wrote:
>
>>  Marton Balint:
>>>  This makes sure the error condition is kept in AVIOContext even if the
>>>  user
>>>  does not check the return value of avio_read_to_bprint or
>>>  ff_read_line_to_bprint.
>>>
>>>  Signed-off-by: Marton Balint <cus@passwd.hu>
>>>  ---
>>>   libavformat/aviobuf.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>>  diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>>>  index 29d4bd7510..6f8a822ee3 100644
>>>  --- a/libavformat/aviobuf.c
>>>  +++ b/libavformat/aviobuf.c
>>>  @@ -875,8 +875,10 @@ static int64_t
>>>  read_string_to_bprint_overwrite(AVIOContext *s, AVBPrint *bp,
>>>       if (ret < 0)
>>>           return ret;
>>>
>>>  -    if (!av_bprint_is_complete(bp))
>>>  +    if (!av_bprint_is_complete(bp)) {
>>>  +        s->error = AVERROR(ENOMEM);
>>>           return AVERROR(ENOMEM);
>>>  +    }
>>>
>>>       return bp->len;
>>>  }
>>>  @@ -1351,8 +1353,10 @@ int avio_read_to_bprint(AVIOContext *h, AVBPrint
>>>  *pb, size_t max_size)
>>>           if (ret <= 0)
>>>               return ret;
>>>           av_bprint_append_data(pb, buf, ret);
>>>  -        if (!av_bprint_is_complete(pb))
>>>  +        if (!av_bprint_is_complete(pb)) {
>>>  +            h->error = AVERROR(ENOMEM);
>>>               return AVERROR(ENOMEM);
>>>  +        }
>>>           max_size -= ret;
>>>       }
>>>       return 0;
>>> 
>>
>>  I don't really see the point of this: It is not a real read error that
>>  should stick to the AVIOContext (which can still be used afterwards
>>  without any issue).
>>  If the user does not check the errors, then the user
>>  has no one to blame but himself for missing errors.
>
> AVIO read/write behaviour is to store IO errors in the context so the user 
> does not have to check for them in every call. It is not well documented 
> which calls should be checked always, so the user might be under the 
> impression that errors during read/write may be checked sometime later.
>
> Admittedly, ENOMEM is not an IO error, but I think it is better to store that 
> as well in the context to keep the behaviour consistent, because in case of 
> ENOMEM avio_read_to_bprint reads and drops undefined amount of data, so the 
> context will also be in an undefined state.
>
> Other possibilities:
> - make avio_read_to_bprint read all the data regardless of AVBPrint fullness
>  - mark avio_read_to_bprint av_warn_unused_result.
>  - both :)
>
> But these also forces the user to check return values... So I kind of like my 
> original approach better, because it maintains avio_read/write call behaviour 
> that it is safe to check errors sometime later.

Any more comments about this or the rest of the series? I plan to apply it 
tomorrow.

Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Fri, 31 Dec 2021, Marton Balint wrote:
> 
>>
>>
>> On Fri, 31 Dec 2021, Andreas Rheinhardt wrote:
>>
>>>  Marton Balint:
>>>>  This makes sure the error condition is kept in AVIOContext even if the
>>>>  user
>>>>  does not check the return value of avio_read_to_bprint or
>>>>  ff_read_line_to_bprint.
>>>>
>>>>  Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>  ---
>>>>   libavformat/aviobuf.c | 8 ++++++--
>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>>  diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>>>>  index 29d4bd7510..6f8a822ee3 100644
>>>>  --- a/libavformat/aviobuf.c
>>>>  +++ b/libavformat/aviobuf.c
>>>>  @@ -875,8 +875,10 @@ static int64_t
>>>>  read_string_to_bprint_overwrite(AVIOContext *s, AVBPrint *bp,
>>>>       if (ret < 0)
>>>>           return ret;
>>>>
>>>>  -    if (!av_bprint_is_complete(bp))
>>>>  +    if (!av_bprint_is_complete(bp)) {
>>>>  +        s->error = AVERROR(ENOMEM);
>>>>           return AVERROR(ENOMEM);
>>>>  +    }
>>>>
>>>>       return bp->len;
>>>>  }
>>>>  @@ -1351,8 +1353,10 @@ int avio_read_to_bprint(AVIOContext *h,
>>>> AVBPrint
>>>>  *pb, size_t max_size)
>>>>           if (ret <= 0)
>>>>               return ret;
>>>>           av_bprint_append_data(pb, buf, ret);
>>>>  -        if (!av_bprint_is_complete(pb))
>>>>  +        if (!av_bprint_is_complete(pb)) {
>>>>  +            h->error = AVERROR(ENOMEM);
>>>>               return AVERROR(ENOMEM);
>>>>  +        }
>>>>           max_size -= ret;
>>>>       }
>>>>       return 0;
>>>>
>>>
>>>  I don't really see the point of this: It is not a real read error that
>>>  should stick to the AVIOContext (which can still be used afterwards
>>>  without any issue).
>>>  If the user does not check the errors, then the user
>>>  has no one to blame but himself for missing errors.
>>
>> AVIO read/write behaviour is to store IO errors in the context so the
>> user does not have to check for them in every call. It is not well
>> documented which calls should be checked always, so the user might be
>> under the impression that errors during read/write may be checked
>> sometime later.
>>
>> Admittedly, ENOMEM is not an IO error, but I think it is better to
>> store that as well in the context to keep the behaviour consistent,
>> because in case of ENOMEM avio_read_to_bprint reads and drops
>> undefined amount of data, so the context will also be in an undefined
>> state.
>>
>> Other possibilities:
>> - make avio_read_to_bprint read all the data regardless of AVBPrint
>> fullness
>>  - mark avio_read_to_bprint av_warn_unused_result.
>>  - both :)
>>
>> But these also forces the user to check return values... So I kind of
>> like my original approach better, because it maintains avio_read/write
>> call behaviour that it is safe to check errors sometime later.
> 
> Any more comments about this or the rest of the series? I plan to apply
> it tomorrow.
> 

I still don't like storing ENOMEM in the AVIOContext and I don't see why
having to check the error is so burdensome. But if you want it so bad,
then go ahead.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] avformat/aviobuf: set AVIOContext->error on bprint buffer ENOMEM

[Marton Balint]

On Mon, 3 Jan 2022, Andreas Rheinhardt wrote:

> Marton Balint:
>> 
>> 
>> On Fri, 31 Dec 2021, Marton Balint wrote:
>> 
>>>
>>>
>>> On Fri, 31 Dec 2021, Andreas Rheinhardt wrote:
>>>
>>>>  Marton Balint:
>>>>>  This makes sure the error condition is kept in AVIOContext even if the
>>>>>  user
>>>>>  does not check the return value of avio_read_to_bprint or
>>>>>  ff_read_line_to_bprint.
>>>>>
>>>>>  Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>>  ---
>>>>>   libavformat/aviobuf.c | 8 ++++++--
>>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>>  diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>>>>>  index 29d4bd7510..6f8a822ee3 100644
>>>>>  --- a/libavformat/aviobuf.c
>>>>>  +++ b/libavformat/aviobuf.c
>>>>>  @@ -875,8 +875,10 @@ static int64_t
>>>>>  read_string_to_bprint_overwrite(AVIOContext *s, AVBPrint *bp,
>>>>>       if (ret < 0)
>>>>>           return ret;
>>>>>
>>>>>  -    if (!av_bprint_is_complete(bp))
>>>>>  +    if (!av_bprint_is_complete(bp)) {
>>>>>  +        s->error = AVERROR(ENOMEM);
>>>>>           return AVERROR(ENOMEM);
>>>>>  +    }
>>>>>
>>>>>       return bp->len;
>>>>>  }
>>>>>  @@ -1351,8 +1353,10 @@ int avio_read_to_bprint(AVIOContext *h,
>>>>> AVBPrint
>>>>>  *pb, size_t max_size)
>>>>>           if (ret <= 0)
>>>>>               return ret;
>>>>>           av_bprint_append_data(pb, buf, ret);
>>>>>  -        if (!av_bprint_is_complete(pb))
>>>>>  +        if (!av_bprint_is_complete(pb)) {
>>>>>  +            h->error = AVERROR(ENOMEM);
>>>>>               return AVERROR(ENOMEM);
>>>>>  +        }
>>>>>           max_size -= ret;
>>>>>       }
>>>>>       return 0;
>>>>>
>>>>
>>>>  I don't really see the point of this: It is not a real read error that
>>>>  should stick to the AVIOContext (which can still be used afterwards
>>>>  without any issue).
>>>>  If the user does not check the errors, then the user
>>>>  has no one to blame but himself for missing errors.
>>>
>>> AVIO read/write behaviour is to store IO errors in the context so the
>>> user does not have to check for them in every call. It is not well
>>> documented which calls should be checked always, so the user might be
>>> under the impression that errors during read/write may be checked
>>> sometime later.
>>>
>>> Admittedly, ENOMEM is not an IO error, but I think it is better to
>>> store that as well in the context to keep the behaviour consistent,
>>> because in case of ENOMEM avio_read_to_bprint reads and drops
>>> undefined amount of data, so the context will also be in an undefined
>>> state.
>>>
>>> Other possibilities:
>>> - make avio_read_to_bprint read all the data regardless of AVBPrint
>>> fullness
>>>  - mark avio_read_to_bprint av_warn_unused_result.
>>>  - both :)
>>>
>>> But these also forces the user to check return values... So I kind of
>>> like my original approach better, because it maintains avio_read/write
>>> call behaviour that it is safe to check errors sometime later.
>> 
>> Any more comments about this or the rest of the series? I plan to apply
>> it tomorrow.
>> 
>
> I still don't like storing ENOMEM in the AVIOContext and I don't see why
> having to check the error is so burdensome. But if you want it so bad,
> then go ahead.

I don't feel strongly about it, it just looked convenient/consistent. But 
I will just skip this patch then and apply the rest.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".