| Message ID | 1374659565-32771-1-git-send-email-martin@martin.st |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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;
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(-)