diff mbox series

[FFmpeg-devel,06/13] avformat/matroskaenc: Only write Cluster if it has in fact been opened

Message ID 20200502171700.28991-1-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/6] avformat/matroskaenc: Move adding SeekEntry into end_ebml_master_crc32() | expand

Commit Message

Andreas Rheinhardt May 2, 2020, 5:16 p.m. UTC 
Since commit 4aa0665f393847c35387a1c673e62346d0acfc95, the dynamic
buffer destined for the contents of the current Cluster is no longer
constantly allocated, reallocated and then freed after writing the
content; instead it is reset and reused when closing a Cluster.

Yet the code in mkv_write_trailer() still checked for whether a Cluster
is open by checking whether the pointer to the dynamic buffer is NULL or
not (instead of checking whether the position of the current Cluster is
-1 or not). If a Cluster was not open, an empty Cluster would be output.

One usually does not run into this issue, because unless there are
errors, there are only three possibilities to not have an opened Cluster
at the end of writing a packet:
The first is if one sent an audio packet to the muxer. It might trigger
closing and outputting the old Cluster, but because the muxer caches
audio packets internally, it would not be output immediately and
therefore no new Cluster would be opened.
The second is an audio packet that does not contain data (such packets
are sometimes sent for side-data only, e.g. by the FLAC encoder). The
only difference to the first scenario is that such packets are not
cached.
The third is if one explicitly flushes the muxer by sending a NULL
packet via av_write_frame().

If one also allows for errors, then there is also another possibility:
Caching the audio packet may fail in the first scenario.

If one calls av_write_trailer() after the first scenario, the cached
audio packet will be output when writing the trailer, for which
a Cluster is opened and everything is fine; because flushing the muxer
does currently not output the cached audio packet (if one is cached),
the issue also does not exist if an audio packet has been cached before
flushing. The issue only exists in one of the other scenarios.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Given that this is a recent regression caused by me I will push this
soon if no one objects.

That the cached packet is not output on flushing looks like a bug to me.
Will send a patch soon.

 libavformat/matroskaenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff mbox series

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 8593c3fce1..99b549ecc4 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -2464,7 +2464,7 @@  static int mkv_write_trailer(AVFormatContext *s)
         }
     }
 
-    if (mkv->cluster_bc) {
+    if (mkv->cluster_pos != -1) {
         ret = end_ebml_master_crc32(pb, &mkv->cluster_bc, mkv,
                               MATROSKA_ID_CLUSTER, 0, 0, 0);
         if (ret < 0)