mbox series

[FFmpeg-devel,0/1] Handle ASS format subtitle encoding ambiguity

Message ID 20230127172047.1024276-1-tim@ngus.net
Headers show
Series Handle ASS format subtitle encoding ambiguity | expand

Message

Tim Angus Jan. 27, 2023, 5:20 p.m. UTC 
Some matroska files embed ASS format subtitles. The header for said
subtitles include the header for the subtitle stream in the "codec
private data" section. It's not clear whether the last byte of this data
is supposed to be 0, i.e. a null terminator for the string data. Among
other tools, older versions of Handbrake do include the null terminator
and there are many files out in the wild created using Handbrake. Using
ffmpeg to extract subtitles for such a file, this header is copied
directly to the output file, including the null terminator, if it was
present. This results in a file in which there is a null terminator
after the header, but preceeding the actual content of the subtitle
file. Obviously this is not correct.

As a data point, of the ~600 mkvs I have locally, 22 of them have ASS
subtitles, and of them 20 include the null terminator, so it doesn't appear to
be a rare phenomenon.

As another data point, the tool mkvextract from mkvtoolnix avoids the ambiguity
by first assuming that the source buffer is *not* null terminated, and then
manually adding a (possibly second) null terminator. The buffer is then
interpreted as a null terminated string and processed that way.
(https://gitlab.com/mbunkus/mkvtoolnix/-/blob/main/src/extract/xtr_textsubs.cpp#L117)

My change here refactors the way the output file is created, by treating
the source buffer as a string. I posted an earlier iteration of this
patch to the list that dealt with the extra null explicitly, but
following some #ffmpeg-devel discussion, I suggested I rework it to work
more like how mkvtoolnix does things, and this was positively received,
so here we are. 

FATE succeeds as there are no mkvs in the suite that have ASS subtitles
embedded.

A small test file that exhibits the problem may be found here:
https://0x0.st/oFTT.mkv

Tim Angus (1):
  avformat/assenc: avoid incorrect copy of null terminator

 libavformat/assenc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)