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

[James Almer]

On 1/6/2017 8:33 PM, Soft Works wrote:
> Revision #4: Don't create CRC32 for preliminary headers
> 
> 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/matroskaenc.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 827d755..dd8ff8e 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -367,6 +367,22 @@ 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)
> +{
> +    if (pb->seekable) {
> +
> +        uint8_t *buf;
> +        int size = avio_get_dyn_buf(*dyn_cp, &buf);
> +
> +        avio_write(pb, buf, size);
> +        end_ebml_master(pb, master);
> +    }
> +}
> +
>  static void put_xiph_size(AVIOContext *pb, int size)
>  {
>      ffio_fill(pb, 255, size / 255);
> @@ -1309,7 +1325,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, &mkv->tracks_bc, mkv, mkv->tracks_master);
>      else
>          end_ebml_master_crc32(pb, &mkv->tracks_bc, mkv, mkv->tracks_master);
>  
> @@ -1554,7 +1570,7 @@ static int mkv_write_tags(AVFormatContext *s)
>  
>      if (mkv->tags.pos) {
>          if (s->pb->seekable && !mkv->is_live)
> -            put_ebml_void(s->pb, avio_tell(mkv->tags_bc));
> +            end_ebml_master_crc32_preliminary(s->pb, &mkv->tags_bc, mkv, mkv->tags);
>          else
>              end_ebml_master_crc32(s->pb, &mkv->tags_bc, mkv, mkv->tags);
>      }
> @@ -1811,7 +1827,7 @@ static int mkv_write_header(AVFormatContext *s)
>          }
>      }
>      if (s->pb->seekable && !mkv->is_live)
> -        put_ebml_void(s->pb, avio_tell(pb));
> +        end_ebml_master_crc32_preliminary(s->pb, &mkv->info_bc, mkv, mkv->info);
>      else
>          end_ebml_master_crc32(s->pb, &mkv->info_bc, mkv, mkv->info);
>      pb = s->pb;

Pushed, thanks.