diff mbox series

[FFmpeg-devel,27/28] avformat/matroskaenc: Remove unnecessary avio_tell(), avio_seek()

Message ID 20200122192717.11678-2-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series Matroska muxer patches
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Jan. 22, 2020, 7:27 p.m. UTC
avio_close_dyn_buf() has a bug: When the write pointer does not point to
the end of the written data when calling it (i.e. when one has performed
a seek back to update already written data), it would not add padding to
the end of the buffer, but to the current position, overwriting other
data; furthermore the reported size would be wrong (off by the amount of
data it has overwritten with padding).

In order not to run into this when updating already written elements or
elements for which size has only been reserved, the Matroska muxer would
first record the current position of the dynamic buffer, then seek to
the desired position, perform the update and seek back to the earlier
position.

But now that end_ebml_master_crc32() does not make use of
avio_close_dyn_buf() any more, this is no longer necessary.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
avio_close_dyn_buf() even has more bugs: Besides the design flaw of
freeing a resource without setting the pointer to it to NULL, it returns
a size of -AV_INPUT_BUFFER_PADDING_SIZE if a memory allocation failure
happened (but not if the arbitrary limit of INT_MAX/2 has been
surpassed); and this despite its documentation not allowing returning
negative values at all. This will definitely need to be fixed (and this
muxer will need to check for whether the allocations failed).

 libavformat/matroskaenc.c | 11 -----------
 1 file changed, 11 deletions(-)
diff mbox series

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 8bbacf5cd3..354096266d 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -2211,7 +2211,6 @@  static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt)
     case AV_CODEC_ID_AAC:
         if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) {
             int filler, output_sample_rate = 0;
-            int64_t curpos;
             ret = get_aac_sample_rates(s, side_data, side_data_size, &track->sample_rate,
                                        &output_sample_rate);
             if (ret < 0)
@@ -2222,7 +2221,6 @@  static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt)
             if (ret < 0)
                 return ret;
             memcpy(par->extradata, side_data, side_data_size);
-            curpos = avio_tell(mkv->tracks_bc);
             avio_seek(mkv->tracks_bc, track->codecpriv_offset, SEEK_SET);
             mkv_write_codecprivate(s, mkv->tracks_bc, par, 1, 0);
             filler = MAX_PCE_SIZE + 2 + 4 - (avio_tell(mkv->tracks_bc) - track->codecpriv_offset);
@@ -2231,7 +2229,6 @@  static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt)
             avio_seek(mkv->tracks_bc, track->sample_rate_offset, SEEK_SET);
             put_ebml_float(mkv->tracks_bc, MATROSKA_ID_AUDIOSAMPLINGFREQ, track->sample_rate);
             put_ebml_float(mkv->tracks_bc, MATROSKA_ID_AUDIOOUTSAMPLINGFREQ, output_sample_rate);
-            avio_seek(mkv->tracks_bc, curpos, SEEK_SET);
         } else if (!par->extradata_size && !track->sample_rate) {
             // No extradata (codecpar or packet side data).
             av_log(s, AV_LOG_ERROR, "Error parsing AAC extradata, unable to determine samplerate.\n");
@@ -2241,7 +2238,6 @@  static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt)
     case AV_CODEC_ID_FLAC:
         if (side_data_size && (s->pb->seekable & AVIO_SEEKABLE_NORMAL) && !mkv->is_live) {
             AVCodecParameters *codecpriv_par;
-            int64_t curpos;
             if (side_data_size != par->extradata_size) {
                 av_log(s, AV_LOG_ERROR, "Invalid FLAC STREAMINFO metadata for output stream %d\n",
                        pkt->stream_index);
@@ -2256,10 +2252,8 @@  static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt)
                 return ret;
             }
             memcpy(codecpriv_par->extradata, side_data, side_data_size);
-            curpos = avio_tell(mkv->tracks_bc);
             avio_seek(mkv->tracks_bc, track->codecpriv_offset, SEEK_SET);
             mkv_write_codecprivate(s, mkv->tracks_bc, codecpriv_par, 1, 0);
-            avio_seek(mkv->tracks_bc, curpos, SEEK_SET);
             avcodec_parameters_free(&codecpriv_par);
         }
         break;
@@ -2271,7 +2265,6 @@  static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt)
             AVIOContext *dyn_cp;
             uint8_t *codecpriv;
             int codecpriv_size;
-            int64_t curpos;
             ret = avio_open_dyn_buf(&dyn_cp);
             if (ret < 0)
                 return ret;
@@ -2281,12 +2274,10 @@  static int mkv_check_new_extra_data(AVFormatContext *s, AVPacket *pkt)
                 av_free(codecpriv);
                 return AVERROR_INVALIDDATA;
             }
-            curpos = avio_tell(mkv->tracks_bc);
             avio_seek(mkv->tracks_bc, track->codecpriv_offset, SEEK_SET);
             // Do not write the OBUs as we don't have space saved for them
             put_ebml_binary(mkv->tracks_bc, MATROSKA_ID_CODECPRIVATE, codecpriv, 4);
             av_free(codecpriv);
-            avio_seek(mkv->tracks_bc, curpos, SEEK_SET);
             ret = ff_alloc_extradata(par, side_data_size);
             if (ret < 0)
                 return ret;
@@ -2578,7 +2569,6 @@  static int mkv_write_trailer(AVFormatContext *s)
         // update stream durations
         if (!mkv->is_live) {
             int i;
-            int64_t curr = avio_tell(mkv->tags_bc);
             for (i = 0; i < s->nb_streams; ++i) {
                 AVStream *st = s->streams[i];
                 mkv_track *track = &mkv->tracks[i];
@@ -2599,7 +2589,6 @@  static int mkv_write_trailer(AVFormatContext *s)
                     put_ebml_binary(mkv->tags_bc, MATROSKA_ID_TAGSTRING, duration_string, 20);
                 }
             }
-            avio_seek(mkv->tags_bc, curr, SEEK_SET);
         }
         if (mkv->tags_bc && !mkv->is_live) {
             avio_seek(pb, mkv->tags_pos, SEEK_SET);