matroskaenc: Produce valid chapter ids

Message ID 1374659565-32771-1-git-send-email-martin@martin.st
State New
Headers show

Commit Message

Martin Storsjö July 24, 2013, 9:52 a.m.
From: Michael Niedermayer <michaelni@gmx.at>

Chapter ids need to start from 1 or higher in matroska. If
necessary, offset the chapter ids to the valid range.

Based on a patch by Fabian Neundorf.
---
Compared to the previous patch, this keeps the chapter ids intact
if remuxing from matroska (or any other source where the ids are
in the correct range to begin with).
---
 libavformat/matroskaenc.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Luca Barbato July 24, 2013, 12:30 p.m. | #1
On 24/07/13 11:52, Martin Storsjö wrote:
> From: Michael Niedermayer <michaelni@gmx.at>
> 
> Chapter ids need to start from 1 or higher in matroska. If
> necessary, offset the chapter ids to the valid range.
> 
> Based on a patch by Fabian Neundorf.
> ---
> Compared to the previous patch, this keeps the chapter ids intact
> if remuxing from matroska (or any other source where the ids are
> in the correct range to begin with).

Feels in the wrong place if we have other formats with the same problem,
beside that maybe using

for (i = 0; i > s->nb_chapters && s->chapters[i]->id; i++);

mkv->chapter_id_offset = !s->chapters[i]->id;

Is slightly simpler.

lu
Martin Storsjö July 24, 2013, 1:26 p.m. | #2
On Wed, 24 Jul 2013, Luca Barbato wrote:

> On 24/07/13 11:52, Martin Storsjö wrote:
>> From: Michael Niedermayer <michaelni@gmx.at>
>> 
>> Chapter ids need to start from 1 or higher in matroska. If
>> necessary, offset the chapter ids to the valid range.
>> 
>> Based on a patch by Fabian Neundorf.
>> ---
>> Compared to the previous patch, this keeps the chapter ids intact
>> if remuxing from matroska (or any other source where the ids are
>> in the correct range to begin with).
>
> Feels in the wrong place if we have other formats with the same problem,

That's certainly a valid point

> beside that maybe using
>
> for (i = 0; i > s->nb_chapters && s->chapters[i]->id; i++);
>
> mkv->chapter_id_offset = !s->chapters[i]->id;
>
> Is slightly simpler.

Hmm, I'm not sure I'd agree with you there, and I think your snippet has 
got a bit too many typos to figure out what you mean that it would do. 
(The solution in the patch also handles negative chapter ids, if that ever 
were to happen, FWIW.)

Anyway, I'm not all that much into chapters, so if you want to solve it in 
other ways, I'll drop this patch and let you finish it.

// Martin
Luca Barbato July 24, 2013, 1:38 p.m. | #3
On 24/07/13 15:26, Martin Storsjö wrote:
> On Wed, 24 Jul 2013, Luca Barbato wrote:
> 
>> On 24/07/13 11:52, Martin Storsjö wrote:
>>> From: Michael Niedermayer <michaelni@gmx.at>
>>>
>>> Chapter ids need to start from 1 or higher in matroska. If
>>> necessary, offset the chapter ids to the valid range.
>>>
>>> Based on a patch by Fabian Neundorf.
>>> ---
>>> Compared to the previous patch, this keeps the chapter ids intact
>>> if remuxing from matroska (or any other source where the ids are
>>> in the correct range to begin with).
>>
>> Feels in the wrong place if we have other formats with the same problem,
> 
> That's certainly a valid point

But I guess none of us know by heart or has time to check which does what.

> Hmm, I'm not sure I'd agree with you there, and I think your snippet has
> got a bit too many typos to figure out what you mean that it would do.
> (The solution in the patch also handles negative chapter ids, if that
> ever were to happen, FWIW.)

Negative chapters sound even more wrong.

> Anyway, I'm not all that much into chapters, so if you want to solve it
> in other ways, I'll drop this patch and let you finish it.

I'm a bit too busy this week. If you need this landing soon I can try to
carve 5 minutes to get the bulk of it or just commit it.

I do not have samples handy to check that.

lu
Luca Barbato July 25, 2013, 4:26 p.m. | #4
On 24/07/13 15:26, Martin Storsjö wrote:
> Hmm, I'm not sure I'd agree with you there, and I think your snippet has
> got a bit too many typos to figure out what you mean that it would do.
> (The solution in the patch also handles negative chapter ids, if that
> ever were to happen, FWIW.)

Negative chapters are just fine. We are talking about UID so they are
expect to be just unique and non-zero for reason I yet I have to know.

Still you do not need to run over all the chapters, thus checking for
the id being 0 in the loop and setting the offset to 1

lu

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 67d2350..b030295 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -101,6 +101,8 @@  typedef struct MatroskaMuxContext {
 
     int reserve_cues_space;
     int64_t cues_pos;
+
+    int chapter_id_offset;
 } MatroskaMuxContext;
 
 
@@ -722,7 +724,7 @@  static int mkv_write_chapters(AVFormatContext *s)
         AVDictionaryEntry *t = NULL;
 
         chapteratom = start_ebml_master(pb, MATROSKA_ID_CHAPTERATOM, 0);
-        put_ebml_uint(pb, MATROSKA_ID_CHAPTERUID, c->id);
+        put_ebml_uint(pb, MATROSKA_ID_CHAPTERUID, c->id + mkv->chapter_id_offset);
         put_ebml_uint(pb, MATROSKA_ID_CHAPTERTIMESTART,
                       av_rescale_q(c->start, c->time_base, scale));
         put_ebml_uint(pb, MATROSKA_ID_CHAPTERTIMEEND,
@@ -803,6 +805,7 @@  static int mkv_write_tag(AVFormatContext *s, AVDictionary *m, unsigned int eleme
 
 static int mkv_write_tags(AVFormatContext *s)
 {
+    MatroskaMuxContext *mkv = s->priv_data;
     ebml_master tags = {0};
     int i, ret;
 
@@ -829,7 +832,7 @@  static int mkv_write_tags(AVFormatContext *s)
         if (!av_dict_get(ch->metadata, "", NULL, AV_DICT_IGNORE_SUFFIX))
             continue;
 
-        ret = mkv_write_tag(s, ch->metadata, MATROSKA_ID_TAGTARGETS_CHAPTERUID, ch->id, &tags);
+        ret = mkv_write_tag(s, ch->metadata, MATROSKA_ID_TAGTARGETS_CHAPTERUID, ch->id + mkv->chapter_id_offset, &tags);
         if (ret < 0) return ret;
     }
 
@@ -967,6 +970,9 @@  static int mkv_write_header(AVFormatContext *s)
     ret = mkv_write_tracks(s);
     if (ret < 0) return ret;
 
+    for (i = 0; i < s->nb_chapters; i++)
+        mkv->chapter_id_offset = FFMAX(mkv->chapter_id_offset, 1 - s->chapters[i]->id);
+
     if (mkv->mode != MODE_WEBM) {
         ret = mkv_write_chapters(s);
         if (ret < 0) return ret;