Re: [FFmpeg-devel] [PATCH] avformat/mov: Store trak > udta metadata on each stream

Re: [FFmpeg-devel] [PATCH] avformat/mov: Store trak > udta metadata on each stream

[Martin Storsjö]

Hi Rémi,

On Wed, 15 Jan 2025, Rémi Bernon wrote:

> Some files keep extra metadata such as 'name' fields within udta, and
> it is useful for Wine to access them with the "export_all" option so
> they can then be exposed to Windows applications.
>
> Signed-off-by: Rémi Bernon <rbernon@codeweavers.com>
> ---
> libavformat/mov.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index c016ce8e41..f067ca4905 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -354,6 +354,12 @@ 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;
> +    AVDictionary **metadata;
> +
> +    if (c->trak_index >= 0 && c->trak_index < c->fc->nb_streams)
> +        metadata = &c->fc->streams[c->trak_index]->metadata;
> +    else
> +        metadata = &c->fc->metadata;
>
>     switch (atom.type) {
>     case MKTAG( '@','P','R','M'): key = "premiere_version"; raw = 1; break;
> @@ -572,10 +578,10 @@ retry:
>             str[str_size] = 0;
>         }
>         c->fc->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED;
> -        av_dict_set(&c->fc->metadata, key, str, 0);
> +        av_dict_set(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);
> +            av_dict_set(metadata, key2, str, 0);
>         }
>         if (!strcmp(key, "encoder")) {
>             int major, minor, micro;
> -- 
> 2.45.2

So instead of storing metadata on the demuxer level, it is stored on the 
stream level, to avoid clobbering metadata if multiple streams provide 
metadata with the same name.

I guess that sounds reasonable. However, wouldn't this be a notable change 
for consumers that currently expect to see such metadata on the demuxer 
level?

I guess we don't have any firm guarantees about such things, and if the 
metadata specifically is for a track, it is the more correct thing to do 
anyway.

I don't have all the usual cases around mov/mp4 metadata fresh in mind 
though. CCing James to hear his opinion on this.

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/mov: Store trak > udta metadata on each stream

[Martin Storsjö]

On Tue, 4 Feb 2025, Martin Storsjö wrote:

> Hi Rémi,
>
> On Wed, 15 Jan 2025, Rémi Bernon wrote:
>
>> Some files keep extra metadata such as 'name' fields within udta, and
>> it is useful for Wine to access them with the "export_all" option so
>> they can then be exposed to Windows applications.
>> 
>> Signed-off-by: Rémi Bernon <rbernon@codeweavers.com>
>> ---
>> libavformat/mov.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index c016ce8e41..f067ca4905 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -354,6 +354,12 @@ 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;
>> +    AVDictionary **metadata;
>> +
>> +    if (c->trak_index >= 0 && c->trak_index < c->fc->nb_streams)
>> +        metadata = &c->fc->streams[c->trak_index]->metadata;
>> +    else
>> +        metadata = &c->fc->metadata;
>>
>>     switch (atom.type) {
>>     case MKTAG( '@','P','R','M'): key = "premiere_version"; raw = 1; break;
>> @@ -572,10 +578,10 @@ retry:
>>             str[str_size] = 0;
>>         }
>>         c->fc->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED;
>> -        av_dict_set(&c->fc->metadata, key, str, 0);
>> +        av_dict_set(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);
>> +            av_dict_set(metadata, key2, str, 0);
>>         }
>>         if (!strcmp(key, "encoder")) {
>>             int major, minor, micro;
>> -- 
>> 2.45.2
>
> So instead of storing metadata on the demuxer level, it is stored on the 
> stream level, to avoid clobbering metadata if multiple streams provide 
> metadata with the same name.
>
> I guess that sounds reasonable. However, wouldn't this be a notable change 
> for consumers that currently expect to see such metadata on the demuxer 
> level?
>
> I guess we don't have any firm guarantees about such things, and if the 
> metadata specifically is for a track, it is the more correct thing to do 
> anyway.
>
> I don't have all the usual cases around mov/mp4 metadata fresh in mind 
> though. CCing James to hear his opinion on this.

I discussed this with James on irc, and he pointed out that udta can exist 
both on the top level and within the track boxes. And this patch does the 
right thing wrt that, as trak_index is >= 0 while parsing a trak, 
otherwise it's < 0.

So this patch seems correct here, so I'll go ahead and push it.

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".