Message ID | 20211011032308.11778-1-nguyenduydong@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/1] mov: read track title | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
andriy/make_ppc | success | Make finished |
andriy/make_fate_ppc | success | Make fate finished |
On Mon, Oct 11, 2021 at 6:23 AM Dong Nguyen <nguyenduydong@gmail.com> wrote: > > this change fix issue [9438](https://trac.ffmpeg.org/ticket/9438) > > after commit da9cc22d5bd5f59756c2037b02966376da2cf323 > ffmpeg is able to write track title metadata to mov/mp4 format file > but it is not able to read back the metadata > Huh, so I seem to have incorrectly read things previously :) .I started implementing reading of the 3GPP titl box due to the QTFF spec saying that `name` is not supposed to be an end user facing name, but an internal/debug reference [1]. Of course, then checking the actual contents of the 'name' box within the udta box, it seems like I was incorrect, as the box's structure doesn't match. The udta side of the spec has a one-liner that doesn't say much about how it should be utilized [2]. So I guess this udta `name` is in this case not something internal, after all? ┐(´д`)┌ [1] https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/Metadata/Metadata.html [2] https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP40000939-CH204-25538 > Signed-off-by: Dong Nguyen <nguyenduydong@gmail.com> > --- > libavformat/mov.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index a811bc7677..644e746d02 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -303,6 +303,7 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom) > int (*parse)(MOVContext*, AVIOContext*, unsigned, const char*) = NULL; > int raw = 0; > int num = 0; > + int track_title = 0; > > switch (atom.type) { > case MKTAG( '@','P','R','M'): key = "premiere_version"; raw = 1; break; > @@ -380,6 +381,12 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom) > case MKTAG(0xa9,'l','y','r'): key = "lyrics"; break; > case MKTAG(0xa9,'m','a','k'): key = "make"; break; > case MKTAG(0xa9,'m','o','d'): key = "model"; break; > + case MKTAG('n','a','m','e') : > + if (c->trak_index >= 0) { // meta inside track > + key = "title"; > + track_title = 1; > + } > + break; Since this seems to be a barebones string-only box, if you set `key = "title"; raw = 1;` the string parsing should work. No need to add the track_title variable to skip other types of string parsing (data_type is zero so it shouldn't hit any of those pieces of custom data parsing logic, either). > case MKTAG(0xa9,'n','a','m'): key = "title"; break; > case MKTAG(0xa9,'o','p','e'): key = "original_artist"; break; > case MKTAG(0xa9,'p','r','d'): key = "producer"; break; > @@ -428,7 +435,7 @@ retry: > return ret; > } > } else return 0; > - } else if (atom.size > 4 && key && !c->itunes_metadata && !raw) { > + } else if (atom.size > 4 && key && !c->itunes_metadata && !raw && !track_title) { > str_size = avio_rb16(pb); // string length > if (str_size > atom.size) { > raw = 1; > @@ -521,7 +528,11 @@ retry: > str[str_size] = 0; > } > c->fc->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; > - av_dict_set(&c->fc->metadata, key, str, 0); > + if (track_title) { > + av_dict_set(&c->fc->streams[c->trak_index]->metadata, key, str, 0); > + } else { > + av_dict_set(&c->fc->metadata, key, str, 0); > + } Since QTFF, ISOBMFF, 3GPP and such all seem to utilize udta in both movie and track boxes, I think the correct thing to do would be to first to poke in something a la https://github.com/jeeb/ffmpeg/commits/enable_writing_udta_metadata_for_tracks . Unfortunately currently movenc's track udta box writing seems to be *very* limited, which thus leads to things which were read from track udta boxes to not be propagated through (and thus you get a lot of FATE test diffs where information is only seemingly removed and not propagated from another location). Welcome to the mess that is mov(enc) :) . Jan
> On Tue, Oct 12, 2021 at 3:54 AM Jan Ekström <jeebjp@gmail.com> wrote: > Huh, so I seem to have incorrectly read things previously :) .I > started implementing reading of the 3GPP titl box due to the QTFF spec > saying that `name` is not supposed to be an end user facing name, but > an internal/debug reference [1]. > > Of course, then checking the actual contents of the 'name' box within > the udta box, it seems like I was incorrect, as the box's structure > doesn't match. The udta side of the spec has a one-liner that doesn't > say much about how it should be utilized [2]. > > So I guess this udta `name` is in this case not something internal, > after all? ┐(´д`)┌ > > [1] https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/Metadata/Metadata.html > [2] https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap2/qtff2.html#//apple_ref/doc/uid/TP40000939-CH204-25538 :) all I did is just trying to implement reverse path of [3] [3] https://github.com/FFmpeg/FFmpeg/commit/da9cc22d5bd5f59756c2037b02966376da2cf323#diff-e40972d5f339c353a7fef633294e1f13fbb2a972ec34f484e4a9fd6a516a11f4R1723-R1724 > Since this seems to be a barebones string-only box, if you set `key = > "title"; raw = 1;` the string parsing should work. No need to add the > track_title variable to skip other types of string parsing (data_type > is zero so it shouldn't hit any of those pieces of custom data parsing > logic, either). nice, will update the patch, thanks btw, `track_title` is also used to reduce checking that the `udta` is inside track boxes but I guess it is a minor point and more clearer if explicitly check it before setting metadata. > Since QTFF, ISOBMFF, 3GPP and such all seem to utilize udta in both > movie and track boxes, I think the correct thing to do would be to > first to poke in something a la > https://github.com/jeeb/ffmpeg/commits/enable_writing_udta_metadata_for_tracks > . > > Unfortunately currently movenc's track udta box writing seems to be > *very* limited, which thus leads to things which were read from track > udta boxes to not be propagated through (and thus you get a lot of > FATE test diffs where information is only seemingly removed and not > propagated from another location). Welcome to the mess that is > mov(enc) :) . > So should I update the patch as suggested and/or wait for properly movenc's udta patch? Thanks Dong
diff --git a/libavformat/mov.c b/libavformat/mov.c index a811bc7677..644e746d02 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -303,6 +303,7 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom) int (*parse)(MOVContext*, AVIOContext*, unsigned, const char*) = NULL; int raw = 0; int num = 0; + int track_title = 0; switch (atom.type) { case MKTAG( '@','P','R','M'): key = "premiere_version"; raw = 1; break; @@ -380,6 +381,12 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext *pb, MOVAtom atom) case MKTAG(0xa9,'l','y','r'): key = "lyrics"; break; case MKTAG(0xa9,'m','a','k'): key = "make"; break; case MKTAG(0xa9,'m','o','d'): key = "model"; break; + case MKTAG('n','a','m','e') : + if (c->trak_index >= 0) { // meta inside track + key = "title"; + track_title = 1; + } + break; case MKTAG(0xa9,'n','a','m'): key = "title"; break; case MKTAG(0xa9,'o','p','e'): key = "original_artist"; break; case MKTAG(0xa9,'p','r','d'): key = "producer"; break; @@ -428,7 +435,7 @@ retry: return ret; } } else return 0; - } else if (atom.size > 4 && key && !c->itunes_metadata && !raw) { + } else if (atom.size > 4 && key && !c->itunes_metadata && !raw && !track_title) { str_size = avio_rb16(pb); // string length if (str_size > atom.size) { raw = 1; @@ -521,7 +528,11 @@ retry: str[str_size] = 0; } c->fc->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; - av_dict_set(&c->fc->metadata, key, str, 0); + if (track_title) { + av_dict_set(&c->fc->streams[c->trak_index]->metadata, key, str, 0); + } else { + av_dict_set(&c->fc->metadata, key, str, 0); + } if (*language && strcmp(language, "und")) { snprintf(key2, sizeof(key2), "%s-%s", key, language); av_dict_set(&c->fc->metadata, key2, str, 0);
this change fix issue [9438](https://trac.ffmpeg.org/ticket/9438) after commit da9cc22d5bd5f59756c2037b02966376da2cf323 ffmpeg is able to write track title metadata to mov/mp4 format file but it is not able to read back the metadata Signed-off-by: Dong Nguyen <nguyenduydong@gmail.com> --- libavformat/mov.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)