[FFmpeg-devel] [RFC] Documenting metadata keys, informative (non-copied) metadata

[FFmpeg-devel] [RFC] Documenting metadata keys, informative (non-copied) metadata

[Tomas Härdin]

Hi

In the samples_md5 patch discussion Michael wanted the proposed key to
be documented. But it turns out we don't have any documentation for
metadata keys! So I'm starting this thread to talk about it. I reckon
we create a new file called doc/metadata_keys.texi with a table listing
keys and where they can appear (format, stream, frames), sorted by
name, with a brief description of each one. Any keys that require more
detailed documentation we could add sections for.

Another issue raised was that some metadata keys shouldn't be carried
over automatically to muxers. In the samples_md5 thread it was pointed
out by Andreas that we don't want to mux that in AIFF. It was also
pointed out that it stops being valid if the audio is cut. This isn't
the first time I've come across cases where we don't want metadata to
be copied, so I'm taking the opportunity to propose informative output-
only metadata could reside in their own namespace. I propose info: for
that, so info:samples_md5 in this specific case, or maybe just
info:md5. HEVC frames could similarly have such metadata applied.

/Tomas

_______________________________________________
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] [RFC] Documenting metadata keys, informative (non-copied) metadata

[Tomas Härdin]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

tor 2025-02-13 klockan 12:54 +0100 skrev Tomas Härdin:
> Hi
> 
> In the samples_md5 patch discussion Michael wanted the proposed key
> to
> be documented. But it turns out we don't have any documentation for
> metadata keys! So I'm starting this thread to talk about it. I reckon
> we create a new file called doc/metadata_keys.texi with a table
> listing
> keys and where they can appear (format, stream, frames), sorted by
> name, with a brief description of each one. Any keys that require
> more
> detailed documentation we could add sections for.
> 
> Another issue raised was that some metadata keys shouldn't be carried
> over automatically to muxers. In the samples_md5 thread it was
> pointed
> out by Andreas that we don't want to mux that in AIFF. It was also
> pointed out that it stops being valid if the audio is cut. This isn't
> the first time I've come across cases where we don't want metadata to
> be copied, so I'm taking the opportunity to propose informative
> output-
> only metadata could reside in their own namespace. I propose info:
> for
> that, so info:samples_md5 in this specific case, or maybe just
> info:md5. HEVC frames could similarly have such metadata applied.

Attached is a PoC for the latter part. Unlike the previous samples_md5
patch, this one only requires updating a FATE ref concerning a .flac
file.

I have to go, else I'd also post the documentation thing as well.

/Tomas

[-- Attachment #2: 0001-lavu-dict-Add-info-namespace-and-AV_DICT_COPY_INFO.patch --]
[-- Type: text/x-patch, Size: 1587 bytes --]

From 2930d42c1dba1f608c1058b95ebcff3909695715 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Tue, 18 Feb 2025 16:45:41 +0100
Subject: [PATCH 1/3] lavu/dict: Add info: namespace and AV_DICT_COPY_INFO

The intent is to enable demuxers to provide informative metadata that is not to be copied to muxers
---
 libavutil/dict.c | 3 +++
 libavutil/dict.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/libavutil/dict.c b/libavutil/dict.c
index 6fb09399ba..067107b7cd 100644
--- a/libavutil/dict.c
+++ b/libavutil/dict.c
@@ -239,9 +239,12 @@ int av_dict_copy(AVDictionary **dst, const AVDictionary *src, int flags)
     const AVDictionaryEntry *t = NULL;
 
     while ((t = av_dict_iterate(src, t))) {
+        // do not copy info: keys unless explicitly told to do so
+        if ((flags & AV_DICT_COPY_INFO) || strncmp(t->key, "info:", 5)) {
         int ret = av_dict_set(dst, t->key, t->value, flags);
         if (ret < 0)
             return ret;
+        }
     }
 
     return 0;
diff --git a/libavutil/dict.h b/libavutil/dict.h
index 713c9e361a..5012374d26 100644
--- a/libavutil/dict.h
+++ b/libavutil/dict.h
@@ -82,6 +82,7 @@
 #define AV_DICT_APPEND         32   /**< If the entry already exists, append to it.  Note that no
                                          delimiter is added, the strings are simply concatenated. */
 #define AV_DICT_MULTIKEY       64   /**< Allow to store several equal keys in the dictionary */
+#define AV_DICT_COPY_INFO     128   /**< In av_dict_copy(), also copy info: keys */
 /**
  * @}
  */
-- 
2.39.5


[-- Attachment #3: 0002-lavu-dict-Indent.patch --]
[-- Type: text/x-patch, Size: 960 bytes --]

From a381e9e26955cd6b2d105cf0f3bcb3b61997febe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Tue, 18 Feb 2025 16:45:59 +0100
Subject: [PATCH 2/3] lavu/dict: Indent

---
 libavutil/dict.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavutil/dict.c b/libavutil/dict.c
index 067107b7cd..e53ab4e554 100644
--- a/libavutil/dict.c
+++ b/libavutil/dict.c
@@ -241,9 +241,9 @@ int av_dict_copy(AVDictionary **dst, const AVDictionary *src, int flags)
     while ((t = av_dict_iterate(src, t))) {
         // do not copy info: keys unless explicitly told to do so
         if ((flags & AV_DICT_COPY_INFO) || strncmp(t->key, "info:", 5)) {
-        int ret = av_dict_set(dst, t->key, t->value, flags);
-        if (ret < 0)
-            return ret;
+            int ret = av_dict_set(dst, t->key, t->value, flags);
+            if (ret < 0)
+                return ret;
         }
     }
 
-- 
2.39.5


[-- Attachment #4: 0003-libavformat-flacdec-Export-samples-md5-as-metadata.patch --]
[-- Type: text/x-patch, Size: 2146 bytes --]

From ef53493e059e8f952cdb9faa338d1c70b8cb9856 Mon Sep 17 00:00:00 2001
From: Mattias Wadman <wader@spotify.com>
Date: Mon, 11 Oct 2021 15:38:13 +0200
Subject: [PATCH 3/3] libavformat/flacdec: Export samples md5 as metadata

Will be used by mal to compare metadat md5 with decoded samples md5.

Part of fixing https://jira.spotify.net/browse/GOL-681
---
 libavformat/flacdec.c               | 6 ++++++
 tests/ref/fate/cover-art-flac-remux | 1 +
 2 files changed, 7 insertions(+)

diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
index 3c317acaee..b4bfbc4f85 100644
--- a/libavformat/flacdec.c
+++ b/libavformat/flacdec.c
@@ -33,6 +33,7 @@
 #include "replaygain.h"
 
 #define SEEKPOINT_SIZE 18
+#define MD5_BYTE_SIZE 16
 
 typedef struct FLACDecContext {
     FFRawDemuxerContext rawctx;
@@ -109,6 +110,7 @@ static int flac_read_header(AVFormatContext *s)
         if (metadata_type == FLAC_METADATA_TYPE_STREAMINFO) {
             uint32_t samplerate;
             uint64_t samples;
+            char md5hex[MD5_BYTE_SIZE*2+1]; // hex representation plus null terminator
 
             /* STREAMINFO can only occur once */
             if (found_streaminfo) {
@@ -133,6 +135,10 @@ static int flac_read_header(AVFormatContext *s)
                 if (samples > 0)
                     st->duration = samples;
             }
+
+            ff_data_to_hex(md5hex, st->codecpar->extradata+18, MD5_BYTE_SIZE, 1 /* lowercase */);
+            md5hex[sizeof(md5hex)-1] = '\0';
+            av_dict_set(&s->metadata, "info:samples_md5", md5hex, 0);
         } else if (metadata_type == FLAC_METADATA_TYPE_CUESHEET) {
             uint8_t isrc[13];
             uint64_t start;
diff --git a/tests/ref/fate/cover-art-flac-remux b/tests/ref/fate/cover-art-flac-remux
index fa91975881..857f1a15ad 100644
--- a/tests/ref/fate/cover-art-flac-remux
+++ b/tests/ref/fate/cover-art-flac-remux
@@ -90,6 +90,7 @@ TAG:comment=Publisher/Studio logotype
 TAG:title=White King Granulated Soap
 [/STREAM]
 [FORMAT]
+TAG:info:samples_md5=496206705f222f9a63bf23dc874d9d71
 TAG:major_brand=M4A 
 TAG:minor_version=0
 TAG:compatible_brands=M4A mp42isom
-- 
2.39.5


[-- Attachment #5: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] [RFC] Documenting metadata keys, informative (non-copied) metadata

[Marvin S.]

On 18 Feb 2025, at 17:56, Tomas Härdin wrote:

> tor 2025-02-13 klockan 12:54 +0100 skrev Tomas Härdin:
>> Hi
>>
>> In the samples_md5 patch discussion Michael wanted the proposed key
>> to
>> be documented. But it turns out we don't have any documentation for
>> metadata keys! So I'm starting this thread to talk about it. I reckon
>> we create a new file called doc/metadata_keys.texi with a table
>> listing
>> keys and where they can appear (format, stream, frames), sorted by
>> name, with a brief description of each one. Any keys that require
>> more
>> detailed documentation we could add sections for.
>>
>> Another issue raised was that some metadata keys shouldn't be carried
>> over automatically to muxers. In the samples_md5 thread it was
>> pointed
>> out by Andreas that we don't want to mux that in AIFF. It was also
>> pointed out that it stops being valid if the audio is cut. This isn't
>> the first time I've come across cases where we don't want metadata to
>> be copied, so I'm taking the opportunity to propose informative
>> output-
>> only metadata could reside in their own namespace. I propose info:
>> for
>> that, so info:samples_md5 in this specific case, or maybe just
>> info:md5. HEVC frames could similarly have such metadata applied.

In reference to the AVDictionary patches (it was an attachment so I can
not comment inline):

IMHO such handling doesn't belong in an abstract container class.

It is surprising behaviour that a dictionary cares about
how the keys look (beyond the case-sensitive handling).

I am not saying this isn't useful but I do not think it belongs in
AVDictionary directly.

>
> Attached is a PoC for the latter part. Unlike the previous samples_md5
> patch, this one only requires updating a FATE ref concerning a .flac
> file.
>
> I have to go, else I'd also post the documentation thing as well.
>
> /Tomas
> _______________________________________________
> 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".
_______________________________________________
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] [RFC] Documenting metadata keys, informative (non-copied) metadata

[Tomas Härdin]

tis 2025-02-18 klockan 18:35 +0100 skrev Marvin S.:
> 
> 
> On 18 Feb 2025, at 17:56, Tomas Härdin wrote:
> 
> > tor 2025-02-13 klockan 12:54 +0100 skrev Tomas Härdin:
> > > Hi
> > > 
> > > In the samples_md5 patch discussion Michael wanted the proposed
> > > key
> > > to
> > > be documented. But it turns out we don't have any documentation
> > > for
> > > metadata keys! So I'm starting this thread to talk about it. I
> > > reckon
> > > we create a new file called doc/metadata_keys.texi with a table
> > > listing
> > > keys and where they can appear (format, stream, frames), sorted
> > > by
> > > name, with a brief description of each one. Any keys that require
> > > more
> > > detailed documentation we could add sections for.
> > > 
> > > Another issue raised was that some metadata keys shouldn't be
> > > carried
> > > over automatically to muxers. In the samples_md5 thread it was
> > > pointed
> > > out by Andreas that we don't want to mux that in AIFF. It was
> > > also
> > > pointed out that it stops being valid if the audio is cut. This
> > > isn't
> > > the first time I've come across cases where we don't want
> > > metadata to
> > > be copied, so I'm taking the opportunity to propose informative
> > > output-
> > > only metadata could reside in their own namespace. I propose
> > > info:
> > > for
> > > that, so info:samples_md5 in this specific case, or maybe just
> > > info:md5. HEVC frames could similarly have such metadata applied.
> 
> In reference to the AVDictionary patches (it was an attachment so I
> can
> not comment inline):
> 
> IMHO such handling doesn't belong in an abstract container class.
> 
> It is surprising behaviour that a dictionary cares about
> how the keys look (beyond the case-sensitive handling).
> 
> I am not saying this isn't useful but I do not think it belongs in
> AVDictionary directly.

Fair enough. Having a separate internal function for it removes the
need for the flag as well

/Tomas
_______________________________________________
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".