Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: Regression fix for invalid MKV headers

[Soft Works]

==> Revised patch in plain text (the attachment from my previous reply wasn't 
recognized by patchwork)

The following three commits created a regression by writing initially
invalid mkv headers:

650e17d88b63b5aca6e0a43483e89e64b0f7d2dd avformat/matroskaenc: write a
CRC32 element on Tags
3bcadf822711720ff0f8d14db71ae47cdf97e652 avformat/matroskaenc: write a
CRC32 element on Info
ee888cfbe777cd2916a3548c750e433ab8f8e6a5 avformat/matroskaenc: postpone
writing the Tracks master

Symptoms:

- You can no longer playback a file that is still processed by ffmpeg,
e.g. VLC fails playback
- You can no longer stream a file to a client while if is still being
processed
- Various diagnosing tools show header errors or incomplete headers
(e.g. ffprobe, mediainfo, mkvalidator)

Note: The symptoms do not apply to completed files or ffmpeg runs that
were interrupted with 'q'

Cause:

The mentioned commits made changes in a way that some header elements
are only partially written in
mkv_write_header, leaving the header in an invalid state. Only in
mkv_write_trailer, these elements
are finished correctly, but that does only occur at the end of the
process.

Regression:

Before these commits were applied, mkv headers have always been valid,
even before completion of ffmpeg.
This has worked reliably over many versions of ffmpeg, to it was an
obvious regression.

Bugtracker:

This issue has been recorded as #5977 which is resolved by this patch

Patch:

The patch adds a new function 'end_ebml_master_crc32_preliminary' that
preliminarily finishes the ebl
element without destroying the buffer. The buffer can be used to update
the ebml element later during
mkv_write_trailer. But most important: mkv_write_header finishes with a
valid mkv header again.
---
 libavformat/avio.h| 12 
 libavformat/aviobuf.c | 17 +
 libavformat/matroskaenc.c | 28 +---
 3 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/libavformat/avio.h b/libavformat/avio.h
index b1ce1d1..f2b9a6f 100644
--- a/libavformat/avio.h
+++ b/libavformat/avio.h
@@ -704,6 +704,18 @@ int avio_closep(AVIOContext **s);
 int avio_open_dyn_buf(AVIOContext **s);
 
 /**
+ * Return the written size and a pointer to the buffer. 
+ * The AVIOContext stream is left intact.
+ * The buffer must NOT be freed. 
+ * No padding is added to the buffer.
+ *
+ * @param s IO context
+ * @param pbuffer pointer to a byte buffer
+ * @return the length of the byte buffer
+ */
+int avio_get_dyn_buf(AVIOContext *s, uint8_t **pbuffer);
+
+/**
  * Return the written size and a pointer to the buffer. The buffer
  * must be freed with av_free().
  * Padding of AV_INPUT_BUFFER_PADDING_SIZE is added to the buffer.
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 134d627..bf7e5f8 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -1277,6 +1277,23 @@ int ffio_open_dyn_packet_buf(AVIOContext **s, int 
max_packet_size)
 return url_open_dyn_buf_internal(s, max_packet_size);
 }
 
+int avio_get_dyn_buf(AVIOContext *s, uint8_t **pbuffer)
+{
+DynBuffer *d;
+
+if (!s) {
+*pbuffer = NULL;
+return 0;
+}
+
+avio_flush(s);
+
+d = s->opaque;
+*pbuffer = d->buffer;
+
+return d->size;
+}
+
 int avio_close_dyn_buf(AVIOContext *s, uint8_t **pbuffer)
 {
 DynBuffer *d;
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 827d755..27d83a6 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -367,6 +367,28 @@ static void end_ebml_master_crc32(AVIOContext *pb, 
AVIOContext **dyn_cp, Matrosk
 *dyn_cp = NULL;
 }
 
+/**
+* Complete ebml master whithout destroying the buffer, allowing for later 
updates
+*/
+static void end_ebml_master_crc32_preliminary(AVIOContext *pb, AVIOContext 
**dyn_cp, MatroskaMuxContext *mkv,
+ebml_master master)
+{
+uint8_t *buf, crc[4];
+int size, skip = 0;
+
+if (pb->seekable) {
+
+size = avio_get_dyn_buf(*dyn_cp, );
+if (mkv->write_crc && mkv->mode != MODE_WEBM) {
+skip = 6; /* Skip reserved 6-byte long void element from the 
dynamic buffer. */
+AV_WL32(crc, av_crc(av_crc_get_table(AV_CRC_32_IEEE_LE), 
UINT32_MAX, buf + skip, size - skip) ^ UINT32_MAX);
+put_ebml_binary(pb, EBML_ID_CRC32, crc, sizeof(crc));
+}
+avio_write(pb, buf + skip, size - skip);
+end_ebml_master(pb, master);
+}
+}
+
 static void put_xiph_size(AVIOContext *pb, int size)
 {
 ffio_fill(pb, 255, size / 255);
@@ -1309,7 +1331,7 @@ static int mkv_write_tracks(AVFormatContext *s)
 }
 
 if (pb->seekable && !mkv->is_live)
-put_ebml_void(pb, avio_tell(mkv->tracks_bc));
+end_ebml_master_crc32_preliminary(pb, >tracks_bc, mkv, 
mkv->tracks_master);
 else
 end_ebml_master_crc32(pb, >tracks_bc, mkv, mkv->tracks_master);
 
@@ -1554,7 +1576,7 @@ static int mkv_write_tags(AVFormatContext *s)
 
 if (mkv->tags.pos) {

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: Regression fix for invalid MKV headers

[Soft Works]

> From: ffmpeg-devel  on behalf of Michael 
> Niedermayer 
>
> this patch breaks fate
> make fate

I apologize, I had run fate but for some reason I hadn't got an error. 
But now I ran again, found and fixed the error.

Please find attached a revised patch (still single patch for now).

> changes to avio* should be in a seperate patch

No problem, I will split it.  For the procedure:

- Should I write individual patch descriptions?
- Should I post both patches in this thread or create two new threads or only 
one new thread for avio?

Thank you very much for your assistance,

softworkz



0002-avformat-matroskaenc-Regression-fix-for-invalid-MKV-.patch
Description: 0002-avformat-matroskaenc-Regression-fix-for-invalid-MKV-.patch
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/matroskaenc: Regression fix for invalid MKV headers

[Michael Niedermayer]

On Tue, Jan 03, 2017 at 11:49:07PM +, Soft Works wrote:
> The following three commits created a regression by writing initially
> invalid mkv headers:
> 
> 650e17d88b63b5aca6e0a43483e89e64b0f7d2dd avformat/matroskaenc: write a
> CRC32 element on Tags
> 3bcadf822711720ff0f8d14db71ae47cdf97e652 avformat/matroskaenc: write a
> CRC32 element on Info
> ee888cfbe777cd2916a3548c750e433ab8f8e6a5 avformat/matroskaenc: postpone
> writing the Tracks master
> 
> Symptoms:
> 
> - You can no longer playback a file that is still processed by ffmpeg,
> e.g. VLC fails playback
> - You can no longer stream a file to a client while if is still being
> processed
> - Various diagnosing tools show header errors or incomplete headers
> (e.g. ffprobe, mediainfo, mkvalidator)
> 
> Note: The symptoms do not apply to completed files or ffmpeg runs that
> were interrupted with 'q'
> 
> Cause:
> 
> The mentioned commits made changes in a way that some header elements
> are only partially written in
> mkv_write_header, leaving the header in an invalid state. Only in
> mkv_write_trailer, these elements
> are finished correctly, but that does only occur at the end of the
> process.
> 
> Regression:
> 
> Before these commits were applied, mkv headers have always been valid,
> even before completion of ffmpeg.
> This has worked reliably over many versions of ffmpeg, to it was an
> obvious regression.
> 
> Bugtracker:
> 
> This issue has been recorded as #5977 which is resolved by this patch
> 
> Patch:
> 
> The patch adds a new function 'end_ebml_master_crc32_preliminary' that
> preliminarily finishes the ebl
> element without destroying the buffer. The buffer can be used to update
> the ebml element later during
> mkv_write_trailer. But most important: mkv_write_header finishes with a
> valid mkv header again.
> ---
>  libavformat/avio.h| 12 
>  libavformat/aviobuf.c | 17 +
>  libavformat/matroskaenc.c | 34 +++---
>  3 files changed, 60 insertions(+), 3 deletions(-)

this patch breaks fate
make fate
...
--- ./tests/ref/fate/rgb24-mkv  2017-01-04 12:03:10.556691008 +0100
+++ tests/data/fate/rgb24-mkv   2017-01-04 13:45:50.260877087 +0100
@@ -1,5 +1,5 @@
-94cce0d7d5b14b4c86e74a1ca454c5aa *tests/data/fate/rgb24-mkv.matroska
-58361 tests/data/fate/rgb24-mkv.matroska
+4060a15b991c314120c51ae7e95958b9 *tests/data/fate/rgb24-mkv.matroska
+58734 tests/data/fate/rgb24-mkv.matroska
 #tb 0: 1/10
 #media_type 0: video
 #codec_id 0: rawvideo
Test rgb24-mkv failed. Look at tests/data/fate/rgb24-mkv.err for details.
make: *** [fate-rgb24-mkv] Error 1

> 
> diff --git a/libavformat/avio.h b/libavformat/avio.h
> index b1ce1d1..f2b9a6f 100644
> --- a/libavformat/avio.h
> +++ b/libavformat/avio.h

changes to avio* should be in a seperate patch

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel