diff mbox

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

Message ID 20170104124927.GT4749@nb4
State Not Applicable
Headers show

Commit Message

Michael Niedermayer Jan. 4, 2017, 12:49 p.m. UTC 
On Tue, Jan 03, 2017 at 11:49:07PM +0000, 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
...
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

[...]

Comments

Soft Works Jan. 4, 2017, 6:06 p.m. UTC | #1 
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> on behalf of Michael Niedermayer <michael@niedermayer.cc>
>
> 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
diff mbox

Patch

--- ./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