diff mbox series

[FFmpeg-devel,v2] avformat/matroskaenc: remove MAX_TRACKS limit

Message ID CANgLvuDJv6Xd=NFbYXxDPeKdsoA6yK8DqEf0Dr_LnVbUJpjBnw@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avformat/matroskaenc: remove MAX_TRACKS limit | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork fail Make failed

Commit Message

Jan Chren (rindeal) April 4, 2020, 10:09 p.m. UTC
On Sat Apr 4 09:29:20 EEST 2020, Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
> Are you running into this limitation?

Yes.

> If so, do the files that you
> create with this patch that have more than 127 tracks work? They
> shouldn't. The reason for this (and the reason that I didn't remove this
> limit altogether in 65ef74f74900590f134b4a130e8f56e5272d1925) lies in
> the way the TrackNumber is encoded in the (Simple)Blocks: They are
> encoded as variable-length numbers, so that encoding small TrackNumbers
> takes up less bytes than encoding bigger TrackNumbers. More precisely,
> TrackNumbers in the range 1..127 are encodable on one byte. And the way
> they are written in mkv_write_block() and mkv_write_vtt_blocks() relies
> on this. If you simply remove said limit, the tracks with TrackNumbers >
> 127 will not have any (Simple)Blocks associated with them; instead the
> encoded TrackNumber will be the desired TrackNumber modulo 128 and the
> (Simple)Block will appear to belong to the track with the encoded
> TrackNumber (if one exists).** The results will of course be catastrophic.

I skimmed through libmatroska's code in src/KaxBlock.cpp and their
limit is 0x4000-1, which is way more reasonable.

> Notice that I have sent a patchset that slightly increases the number of
> allowed tracks (without fixing the root cause though, because I didn't
> know if there are really people who run into this): [1] makes
> attachments no longer count towards the limit; [2] increases the limit
> to 127. I will resend said patchset soon.
>
> - Andreas
>
> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-November/253452.html
> [2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-November/252563.html
>
> *: The reason for setting MAX_TRACKS to 126 instead of 127 is probably
> that the encoding corresponding to the number 127 is special when
> encoding lengths (which use the same variable-length number scheme):
> Here it denotes an unknown length instead of a length of 127. But there
> are no such values with a special meaning for encoded TrackNumbers.

Indeed, libmatroska allows 0x80-1 (127) per byte. Your second patch
should have been merged.

> **: Notice that a TrackNumber of 0 is invalid, so any (Simple)Block that
> ought to belong to the tracks with TrackNumber 128, 256, 384, ... will
> have no matching track at all. FFmpeg's Matroska demuxer treats
> encountering such (Simple)Blocks as a sign of invalid data and it will
> try to resync to the next level 1 element (typically the next Cluster).
> Similar things can happen if you use attachments (in this case there may
> be gaps in the used TrackNumbers).

I have included a new patch, which writes the track_number using
put_ebml_num(), which should work perhaps?

Comments

Andreas Rheinhardt April 15, 2020, 6:01 a.m. UTC | #1
Jan Chren (rindeal):
> On Sat Apr 4 09:29:20 EEST 2020, Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com> wrote:
>> Are you running into this limitation?
> 
> Yes.
> 
>> If so, do the files that you
>> create with this patch that have more than 127 tracks work? They
>> shouldn't. The reason for this (and the reason that I didn't remove this
>> limit altogether in 65ef74f74900590f134b4a130e8f56e5272d1925) lies in
>> the way the TrackNumber is encoded in the (Simple)Blocks: They are
>> encoded as variable-length numbers, so that encoding small TrackNumbers
>> takes up less bytes than encoding bigger TrackNumbers. More precisely,
>> TrackNumbers in the range 1..127 are encodable on one byte. And the way
>> they are written in mkv_write_block() and mkv_write_vtt_blocks() relies
>> on this. If you simply remove said limit, the tracks with TrackNumbers >
>> 127 will not have any (Simple)Blocks associated with them; instead the
>> encoded TrackNumber will be the desired TrackNumber modulo 128 and the
>> (Simple)Block will appear to belong to the track with the encoded
>> TrackNumber (if one exists).** The results will of course be catastrophic.
> 
> I skimmed through libmatroska's code in src/KaxBlock.cpp and their
> limit is 0x4000-1, which is way more reasonable.
> 
>> Notice that I have sent a patchset that slightly increases the number of
>> allowed tracks (without fixing the root cause though, because I didn't
>> know if there are really people who run into this): [1] makes
>> attachments no longer count towards the limit; [2] increases the limit
>> to 127. I will resend said patchset soon.
>>
>> - Andreas
>>
>> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-November/253452.html
>> [2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2019-November/252563.html
>>
>> *: The reason for setting MAX_TRACKS to 126 instead of 127 is probably
>> that the encoding corresponding to the number 127 is special when
>> encoding lengths (which use the same variable-length number scheme):
>> Here it denotes an unknown length instead of a length of 127. But there
>> are no such values with a special meaning for encoded TrackNumbers.
> 
> Indeed, libmatroska allows 0x80-1 (127) per byte. Your second patch
> should have been merged.
> 
>> **: Notice that a TrackNumber of 0 is invalid, so any (Simple)Block that
>> ought to belong to the tracks with TrackNumber 128, 256, 384, ... will
>> have no matching track at all. FFmpeg's Matroska demuxer treats
>> encountering such (Simple)Blocks as a sign of invalid data and it will
>> try to resync to the next level 1 element (typically the next Cluster).
>> Similar things can happen if you use attachments (in this case there may
>> be gaps in the used TrackNumbers).
> 
> I have included a new patch, which writes the track_number using
> put_ebml_num(), which should work perhaps?
> 
I have finally found the time to implement this. And now I find out that
you have actually sent a second patch that is much better than the first
one. It would mostly work except in some edge cases: You forgot to
update mkv_blockgroup_size(). Said function estimates the size of
BlockGroups in order to know how many bytes to use for the length field
of the BlockGroup itself. Right now it overestimates it: It thinks the
duration will always need eight bytes, when most durations fit into two
bytes; so that unless the duration is gigantic this overestimation can
make up for a potential underestimation of the size of the included
Block. (Notice that in order for this to be dangerous the size would not
only be gigantic, but it would also need to change the number of bytes
chosen for the length field of the BlockGroup. Yet even such edge cases
are unacceptable.) And I also don't like that you did not use the
minimal amount of bytes.

I do not want to simply ignore your patch. Please tell me if you want to
send a new improved and rebased (your current patch would not apply any
more) version or if I should just send my version.

- Andreas
diff mbox series

Patch

From 477d1ee08115e09f2f61f75dd70ce7bc882d2c34 Mon Sep 17 00:00:00 2001
From: Jan Chren (rindeal) <dev.rindeal@gmail.com>
Date: Sat, 4 Apr 2020 00:00:00 +0000
Subject: [PATCH] avformat/matroskaenc: remove track limit

Strictly speaking, ebml_num_size() wastes one bit here.
Employing new ebml_num_size() and put_ebml_num() just for track_number
would solve this.

---
 libavformat/matroskaenc.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 5dae53026d..30132a53e8 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -117,10 +117,6 @@  typedef struct mkv_attachments {
 #define MODE_MATROSKAv2 0x01
 #define MODE_WEBM       0x02
 
-/** Maximum number of tracks allowed in a Matroska file (with track numbers in
- * range 1 to 126 (inclusive) */
-#define MAX_TRACKS 126
-
 typedef struct MatroskaMuxContext {
     const AVClass   *class;
     int             mode;
@@ -2089,9 +2085,8 @@  static int mkv_write_block(AVFormatContext *s, AVIOContext *pb,
     }
 
     put_ebml_id(pb, blockid);
-    put_ebml_num(pb, size + 4, 0);
-    // this assumes stream_index is less than 126
-    avio_w8(pb, 0x80 | track_number);
+    put_ebml_num(pb, size + ebml_num_size(track_number) + 2 + 1, 0);
+    put_ebml_num(track_number, 0);
     avio_wb16(pb, ts - mkv->cluster_pts);
     avio_w8(pb, (blockid == MATROSKA_ID_SIMPLEBLOCK && keyframe) ? (1 << 7) : 0);
     avio_write(pb, data + offset, size);
@@ -2155,8 +2150,8 @@  static int mkv_write_vtt_blocks(AVFormatContext *s, AVIOContext *pb, AVPacket *p
     blockgroup = start_ebml_master(pb, MATROSKA_ID_BLOCKGROUP, mkv_blockgroup_size(size));
 
     put_ebml_id(pb, MATROSKA_ID_BLOCK);
-    put_ebml_num(pb, size + 4, 0);
-    avio_w8(pb, 0x80 | (pkt->stream_index + 1));     // this assumes stream_index is less than 126
+    put_ebml_num(pb, size + ebml_num_size(pkt->stream_index + 1) + 2 + 1, 0);
+    put_ebml_num(pkt->stream_index + 1, 0);
     avio_wb16(pb, ts - mkv->cluster_pts);
     avio_w8(pb, flags);
     avio_printf(pb, "%.*s\n%.*s\n%.*s", id_size, id, settings_size, settings, pkt->size, pkt->data);
@@ -2604,13 +2599,6 @@  static int mkv_init(struct AVFormatContext *s)
 {
     int i;
 
-    if (s->nb_streams > MAX_TRACKS) {
-        av_log(s, AV_LOG_ERROR,
-               "At most %d streams are supported for muxing in Matroska\n",
-               MAX_TRACKS);
-        return AVERROR(EINVAL);
-    }
-
     for (i = 0; i < s->nb_streams; i++) {
         if (s->streams[i]->codecpar->codec_id == AV_CODEC_ID_ATRAC3 ||
             s->streams[i]->codecpar->codec_id == AV_CODEC_ID_COOK ||
-- 
2.26.0