[FFmpeg-devel,1/1] mov: read track title

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(-)

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