diff mbox

[FFmpeg-devel,v3] avformat/matroskaenc: Use random TrackUID

Message ID 20191203151947.24685-1-andreas.rheinhardt@gmail.com
State Superseded
Headers show

Commit Message

Andreas Rheinhardt Dec. 3, 2019, 3:19 p.m. UTC
Up until now, the TrackUID of a Matroska track which is supposed to be
random was not random at all: It always coincided with the TrackNumber
which is usually the 1-based index of the corresponding stream in the
array of AVStreams. This has been changed: It is now set via an AVLFG
if AVFMT_FLAG_BITEXACT is not set. Otherwise it is set like it is set
now (the only change happens if an explicit track number has been
chosen via dash_track_number, because the system used in the normal
situation is now used, too). In particular, no FATE tests need to be
updated.

This also fixes a bug in case the dash_track_number option was used:
In this case the TrackUID was set to the track number, but the tags were
written with a TagTrackUID simply based upon the index, so that the tags
didn't apply to the track they ought to apply to.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
I have updated the patchset to rely on the AVLFG to provide sufficiently
many distinct values, as you said. I could add a check and a for if we still
have a collision after (say) 1000 tries in order to avoid the
potentially infinite loop.

 libavformat/matroskaenc.c | 52 +++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 8 deletions(-)

Comments

Michael Niedermayer Dec. 4, 2019, 6:58 p.m. UTC | #1
On Tue, Dec 03, 2019 at 04:19:47PM +0100, Andreas Rheinhardt wrote:
> Up until now, the TrackUID of a Matroska track which is supposed to be
> random was not random at all: It always coincided with the TrackNumber
> which is usually the 1-based index of the corresponding stream in the
> array of AVStreams. This has been changed: It is now set via an AVLFG
> if AVFMT_FLAG_BITEXACT is not set. Otherwise it is set like it is set
> now (the only change happens if an explicit track number has been
> chosen via dash_track_number, because the system used in the normal
> situation is now used, too). In particular, no FATE tests need to be
> updated.
> 
> This also fixes a bug in case the dash_track_number option was used:
> In this case the TrackUID was set to the track number, but the tags were
> written with a TagTrackUID simply based upon the index, so that the tags
> didn't apply to the track they ought to apply to.

LGTM
thx


> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> I have updated the patchset to rely on the AVLFG to provide sufficiently
> many distinct values, as you said. I could add a check and a for if we still
> have a collision after (say) 1000 tries in order to avoid the
> potentially infinite loop.

You could replace the LFG if you dont trust it.
But i dont understand why you would use it when you dont trust it and then
add a check

Thanks

[...]
Andreas Rheinhardt Dec. 5, 2019, 11:59 p.m. UTC | #2
Michael Niedermayer:
> On Tue, Dec 03, 2019 at 04:19:47PM +0100, Andreas Rheinhardt wrote:
>> Up until now, the TrackUID of a Matroska track which is supposed to be
>> random was not random at all: It always coincided with the TrackNumber
>> which is usually the 1-based index of the corresponding stream in the
>> array of AVStreams. This has been changed: It is now set via an AVLFG
>> if AVFMT_FLAG_BITEXACT is not set. Otherwise it is set like it is set
>> now (the only change happens if an explicit track number has been
>> chosen via dash_track_number, because the system used in the normal
>> situation is now used, too). In particular, no FATE tests need to be
>> updated.
>>
>> This also fixes a bug in case the dash_track_number option was used:
>> In this case the TrackUID was set to the track number, but the tags were
>> written with a TagTrackUID simply based upon the index, so that the tags
>> didn't apply to the track they ought to apply to.
> 
> LGTM
> thx
> 
Good to hear. When merging, the fix for ChapterUIDs
(https://ffmpeg.org/pipermail/ffmpeg-devel/2019-November/253440.html)
should get merged first.
> 
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> I have updated the patchset to rely on the AVLFG to provide sufficiently
>> many distinct values, as you said. I could add a check and a for if we still
>> have a collision after (say) 1000 tries in order to avoid the
>> potentially infinite loop.
> 
> You could replace the LFG if you dont trust it.
> But i dont understand why you would use it when you dont trust it and then
> add a check
> 
I actually trust that it works; it's just that I can't furnish a proof.

- Andreas
diff mbox

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 41c3f1a985..b2f28304c8 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -94,6 +94,7 @@  typedef struct mkv_cues {
 typedef struct mkv_track {
     int             write_dts;
     int             has_cue;
+    uint64_t        uid;
     int             sample_rate;
     int64_t         sample_rate_offset;
     int64_t         last_timestamp;
@@ -1199,8 +1200,7 @@  static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
     track = start_ebml_master(pb, MATROSKA_ID_TRACKENTRY, 0);
     put_ebml_uint (pb, MATROSKA_ID_TRACKNUMBER,
                    mkv->is_dash ? mkv->dash_track_number : i + 1);
-    put_ebml_uint (pb, MATROSKA_ID_TRACKUID,
-                   mkv->is_dash ? mkv->dash_track_number : i + 1);
+    put_ebml_uint (pb, MATROSKA_ID_TRACKUID, mkv->tracks[i].uid);
     put_ebml_uint (pb, MATROSKA_ID_TRACKFLAGLACING , 0);    // no lacing (yet)
 
     if ((tag = av_dict_get(st->metadata, "title", NULL, 0)))
@@ -1651,7 +1651,8 @@  static int mkv_write_tags(AVFormatContext *s)
         if (!mkv_check_tag(st->metadata, MATROSKA_ID_TAGTARGETS_TRACKUID))
             continue;
 
-        ret = mkv_write_tag(s, st->metadata, MATROSKA_ID_TAGTARGETS_TRACKUID, i + 1);
+        ret = mkv_write_tag(s, st->metadata, MATROSKA_ID_TAGTARGETS_TRACKUID,
+                            mkv->tracks[i].uid);
         if (ret < 0) return ret;
     }
 
@@ -1659,13 +1660,15 @@  static int mkv_write_tags(AVFormatContext *s)
         for (i = 0; i < s->nb_streams; i++) {
             AVIOContext *pb;
             AVStream *st = s->streams[i];
+            mkv_track *track = &mkv->tracks[i];
             ebml_master tag_target;
             ebml_master tag;
 
             if (st->codecpar->codec_type == AVMEDIA_TYPE_ATTACHMENT)
                 continue;
 
-            mkv_write_tag_targets(s, MATROSKA_ID_TAGTARGETS_TRACKUID, i + 1, &tag_target);
+            mkv_write_tag_targets(s, MATROSKA_ID_TAGTARGETS_TRACKUID,
+                                  track->uid, &tag_target);
             pb = mkv->tags_bc;
 
             tag = start_ebml_master(pb, MATROSKA_ID_SIMPLETAG, 0);
@@ -1865,10 +1868,6 @@  static int mkv_write_header(AVFormatContext *s)
             version = 4;
     }
 
-    mkv->tracks = av_mallocz_array(s->nb_streams, sizeof(*mkv->tracks));
-    if (!mkv->tracks) {
-        return AVERROR(ENOMEM);
-    }
     ebml_header = start_ebml_master(pb, EBML_ID_HEADER, MAX_EBML_HEADER_SIZE);
     put_ebml_uint  (pb, EBML_ID_EBMLVERSION       ,           1);
     put_ebml_uint  (pb, EBML_ID_EBMLREADVERSION   ,           1);
@@ -2670,8 +2669,29 @@  static int webm_query_codec(enum AVCodecID codec_id, int std_compliance)
     return 0;
 }
 
+static uint64_t mkv_get_uid(const mkv_track *tracks, int i, AVLFG *c)
+{
+    uint64_t uid;
+
+    while (1) {
+        int k;
+        uid  = (uint64_t)av_lfg_get(c) << 32;
+        uid |= av_lfg_get(c);
+        if (!uid)
+            continue;
+        for (k = 0; k < i; k++) {
+            if (tracks[k].uid == uid)
+                break;
+        }
+        if (k == i)
+            return uid;
+    }
+}
+
 static int mkv_init(struct AVFormatContext *s)
 {
+    MatroskaMuxContext *mkv = s->priv_data;
+    AVLFG c;
     int i;
 
     if (s->nb_streams > MAX_TRACKS) {
@@ -2700,7 +2720,23 @@  static int mkv_init(struct AVFormatContext *s)
         s->internal->avoid_negative_ts_use_pts = 1;
     }
 
+    mkv->tracks = av_mallocz_array(s->nb_streams, sizeof(*mkv->tracks));
+    if (!mkv->tracks) {
+        return AVERROR(ENOMEM);
+    }
+
+    if (!(s->flags & AVFMT_FLAG_BITEXACT))
+        av_lfg_init(&c, av_get_random_seed());
+
     for (i = 0; i < s->nb_streams; i++) {
+        mkv_track *track = &mkv->tracks[i];
+
+        if (s->flags & AVFMT_FLAG_BITEXACT) {
+            track->uid = i + 1;
+        } else {
+            track->uid = mkv_get_uid(mkv->tracks, i, &c);
+        }
+
         // ms precision is the de-facto standard timescale for mkv files
         avpriv_set_pts_info(s->streams[i], 64, 1, 1000);
     }