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

[Andreas Rheinhardt]

Jan Chren (rindeal):
> On Sat Apr 4 09:29:20 EEST 2020, Andreas Rheinhardt
> <andreas.rheinhardt at 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