Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On Mon, Oct 26, 2015 at 8:06 AM, Derek Buitenhuis < derek.buitenh...@gmail.com> wrote: > On 10/23/2015 7:41 PM, Tinglin Liu wrote: > > Derek, would you do the amend and push? Let me know if you need me to > > resend an amended patch. Thanks. > > Amended and pushed. > > As before: Is there a sample somewhere we can add a FATE test for? > One example: https://drive.google.com/file/d/0Bxqhl1579b3ETmVQb01pRDZjVjQ/view?usp=sharing It has two meta key-value pairs com.android.version 6.0 com.android.capture.fps 120.00 Thanks for the pushing. > > - Derek > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On 10/23/2015 7:41 PM, Tinglin Liu wrote: > Derek, would you do the amend and push? Let me know if you need me to > resend an amended patch. Thanks. Amended and pushed. As before: Is there a sample somewhere we can add a FATE test for? - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
Le quartidi 4 brumaire, an CCXXIV, wm4 a écrit : > Well, I'm not sure if this only affects LC_NUMERIC and maybe LC_COLLATE? I grant you that LC_PAPER or LC_TELEPHONE are probably harmless; they were added after I made my mental list. It should work by whitelist, not by blacklist. > On the other hand, setting LC_CTYPE could have some bad effects too. Very true. Unfortunately, some genius decided to use LC_CTYPE to determine the tty's encoding, instead of $TERM, this makes LC_CTYPE unavoidable. > I'd be fine with that, but then we should maybe check the current > locale for sanity at certain points, and raise a clear error if it's > broken. > > Many programs set locale by default, including GUI toolkits like Qt (!). > Qt in particular document this explicitly: > > http://doc.qt.io/qt-5/qcoreapplication.html#locale-settings > > So we shouldn't silently break everything. But again, breaking loudly > is ok IMHO. Agree. > You could demand the same from FFmpeg. It's impractical because not all > systems support these functions. Exactly. Printing number with a localized separator is a minor and optional feature, and rather easy to reimplement in the application. Parsing floating point numbers is used everywhere and hard to do if you want to keep the full precision. Which one should work by default is really a no-brainer. (Well, actually, I consider localizing the decimal separator to be completely stupid. Numbers are numbers.) > I think we don't need to block the patch on this issue anymore, because: Agree too. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On 10/25/2015 12:06 PM, wm4 wrote: > I think we don't need to block the patch on this issue anymore, because: > 1. We don't have any workarounds in place this patch could use, > 2. Lots of other code is probably affected anyway I agree, it is beyond the scope of this patch. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On Sun, 25 Oct 2015 12:39:08 +0100 Nicolas George wrote: > Le quartidi 4 brumaire, an CCXXIV, Derek Buitenhuis a écrit : > > Perhaps wm4 or Michael or someone can chime in... I don't know of a single > > good > > way to handle this. Just failing if the locale has a radix point as a comma > > instead > > of a period seems less bad than mucking with the thread/process locale > > inside a > > library. > > Personally, I consider that users setting locale variables other than > LC_CTYPE and LC_MESSAGES mean literally "please break random programs on my > system"; I am convinced of this since more than fifteen years when initing > Gtk in the OCaml toplevel would cause "let pi = 3.14" to set pi to 3.0. Well, I'm not sure if this only affects LC_NUMERIC and maybe LC_COLLATE? On the other hand, setting LC_CTYPE could have some bad effects too. > Therefore, my opinion on this is: let us document that for all FFmpeg > libraries, setting any locale except these two to anything other than > C/POSIX yields undefined behaviours. I'd be fine with that, but then we should maybe check the current locale for sanity at certain points, and raise a clear error if it's broken. Many programs set locale by default, including GUI toolkits like Qt (!). Qt in particular document this explicitly: http://doc.qt.io/qt-5/qcoreapplication.html#locale-settings So we shouldn't silently break everything. But again, breaking loudly is ok IMHO. > If people want to write applications using various locales, they should use > the version with explicit locale argument introduced in the latest version > of the standard. You could demand the same from FFmpeg. It's impractical because not all systems support these functions. PS: I think we don't need to block the patch on this issue anymore, because: 1. We don't have any workarounds in place this patch could use, 2. Lots of other code is probably affected anyway ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
Le quartidi 4 brumaire, an CCXXIV, Derek Buitenhuis a écrit : > Perhaps wm4 or Michael or someone can chime in... I don't know of a single > good > way to handle this. Just failing if the locale has a radix point as a comma > instead > of a period seems less bad than mucking with the thread/process locale inside > a > library. Personally, I consider that users setting locale variables other than LC_CTYPE and LC_MESSAGES mean literally "please break random programs on my system"; I am convinced of this since more than fifteen years when initing Gtk in the OCaml toplevel would cause "let pi = 3.14" to set pi to 3.0. Therefore, my opinion on this is: let us document that for all FFmpeg libraries, setting any locale except these two to anything other than C/POSIX yields undefined behaviours. If people want to write applications using various locales, they should use the version with explicit locale argument introduced in the latest version of the standard. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On 10/23/2015 7:41 PM, Tinglin Liu wrote: > > http://stackoverflow.com/questions/3457968/snprintf-simple-way-to-force-as-radix > > Here it mentioned using the setlocale() function, but I didn't find any > examples elsewhere though > > Derek, would you do the amend and push? Let me know if you need me to > resend an amended patch. Thanks. This may be enough of a pain that it needs a wrapper function... e.g. on POSIX, it says the locale is common to all threads in a process, but on Windows, you have _configthreadlocale, to define the behavior, and I'm not sure how systems like Solaris work. I'm not sure how setting the locale could mess with other bits of the library running concurrently, or worse, a user application which links to libavformat may/will be affected. I hate to hold up this patch for spec-arguign reasons, but this *should* probably be addressed somehow. Lordy, C locales are crap. Perhaps wm4 or Michael or someone can chime in... I don't know of a single good way to handle this. Just failing if the locale has a radix point as a comma instead of a period seems less bad than mucking with the thread/process locale inside a library. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On Thu, Oct 22, 2015 at 4:57 PM, Derek Buitenhuis < derek.buitenh...@gmail.com> wrote: > On 10/23/2015 12:19 AM, wm4 wrote: > > Wrong, snprintf() always returns the number of characters the string > > would have been, even if the buffer size is smaller. > > That'll teach me to reply past midnight. > > I am dumb at night. > > > Also, shouldn't this use some av_ wrapper? What about locale issues? > > Some locales won't print a "." but a ",". > > Oh right. C locales. > > I don't know if we have a function for this specific problem or not, > but it would not surprise me if we do. > http://stackoverflow.com/questions/3457968/snprintf-simple-way-to-force-as-radix Here it mentioned using the setlocale() function, but I didn't find any examples elsewhere though Derek, would you do the amend and push? Let me know if you need me to resend an amended patch. Thanks. > > - Derek > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On 10/23/2015 12:19 AM, wm4 wrote: > Wrong, snprintf() always returns the number of characters the string > would have been, even if the buffer size is smaller. That'll teach me to reply past midnight. I am dumb at night. > Also, shouldn't this use some av_ wrapper? What about locale issues? > Some locales won't print a "." but a ",". Oh right. C locales. I don't know if we have a function for this specific problem or not, but it would not surprise me if we do. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On Fri, 23 Oct 2015 00:10:20 +0100
Derek Buitenhuis wrote:
> On 10/22/2015 11:04 PM, Tinglin Liu wrote:
> > +} else if (data_type == 23 && str_size >= 4) { // BE float32
> > +union av_intfloat32 val;
> > +val.i = avio_rb32(pb);
>
> I found we have a function to to this: av_int2float().
>
> > +if (snprintf(str, str_size_alloc, "%f", val.f) >=
> > str_size_alloc) {
>
> snprintf can never return a value greater than the length passed to it, so
> just == is fine.
Wrong, snprintf() always returns the number of characters the string
would have been, even if the buffer size is smaller.
Also, str_size_alloc is unsigned, so in theory this catches cases when
snprintf fails (negative return value) too. But probably doesn't matter
here.
Also, shouldn't this use some av_ wrapper? What about locale issues?
Some locales won't print a "." but a ",".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On 10/22/2015 11:04 PM, Tinglin Liu wrote:
> +} else if (data_type == 23 && str_size >= 4) { // BE float32
> +union av_intfloat32 val;
> +val.i = avio_rb32(pb);
I found we have a function to to this: av_int2float().
> +if (snprintf(str, str_size_alloc, "%f", val.f) >=
> str_size_alloc) {
snprintf can never return a value greater than the length passed to it, so just
== is fine.
Although, thinking about it, I was incorrect in thinking it could overflow the
512+1 length
buffer, since the largest float with precision of 6 (the default in C) + radix
is smaller
than that by a lot. My bad.
Nothing here warrants resending, IMO, and I will amend and push tomorrow, to
allow time
for others to review or comment in the meantime.
- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On 10/22/2015 11:28 PM, Tinglin Liu wrote: > 2) The key and value are stored separately for each key-value pair. 'keys' > atom stores the key name table, while 'ilst' atom stores the values > corresponding to the indices in the key table. And since they are stored in > two different atoms, I have to store the name and count of the keys in the > MOVContext, and use them when parsing 'ilst'. Right. For some reason I thought it was begin exported. It's clear to me now. >>> +union av_intfloat32 val; >>> +val.i = avio_rb32(pb); >> >> I'm not entirely sure of the portability of this code. Would this not fail >> on >> any system without IEEE floats? Still not sure about this, but I see such stuff used elsewhere in avformat, via av_int2float, so I guess we probably don't care. Someone correct me if I am mistaken. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
Thank you Derek for the reply.
Basically the metadata is stored in the structure like:
|--meta |hdlr |keys |ilst
1) Firstly, we check if the handler type in the metadata handler atom is
‘mdta’. If it is, we set found_hdlr_mdta in
MOVContext be 1.
2) The key and value are stored separately for each key-value pair. 'keys'
atom stores the key name table, while 'ilst' atom stores the values
corresponding to the indices in the key table. And since they are stored in
two different atoms, I have to store the name and count of the keys in the
MOVContext, and use them when parsing 'ilst'.
Cheers,
Tinglin
On Thu, Oct 22, 2015 at 11:43 AM, Derek Buitenhuis <
derek.buitenh...@gmail.com> wrote:
> On 10/20/2015 7:29 PM, Tinglin Liu wrote:
> > The Apple dev specification:
> >
> https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/Metadata/Metadata.html
> > ---
> > libavformat/isom.h | 3 +++
> > libavformat/mov.c | 77
> +-
> > 2 files changed, 74 insertions(+), 6 deletions(-)
>
> Is there a test file around we can add to FATE?
>
> > @@ -177,6 +177,9 @@ typedef struct MOVContext {
> > int64_t duration; ///< duration of the longest track
> > int found_moov; ///< 'moov' atom has been found
> > int found_mdat; ///< 'mdat' atom has been found
> > +int found_hdlr_mdta; ///< 'hdlr' atom with type 'mdta' has been
> found
> > +char **meta_keys;
> > +unsigned meta_keys_count;
>
> How exactly is it intended for these gets to be accessed?
>
> MOVContext is an internal
> structure only. Perhaps this should be exported via side-data?
> @@ -368,6 +369,11 @@ retry:
> > av_log(c->fc, AV_LOG_ERROR, "Error parsing cover
> art.\n");
> > }
> > return ret;
> > +} else if (!key &&
>
> c->found_hdlr_mdta && c->meta_keys) {
>
> Is it ever possible for c->meta_keys to not be allocated here? (Patch
> doesn't
> provide me enough context liens to determine that.)
>
c->found_hdlr_mdta checks the handler type of 'hdlr' is good, but
c->meta_keys is read from 'keys'. I believe c->meta_keys is necessary in
case the 'keys' atom is missing.
>
> > +uint32_t index = AV_RB32(&atom.type);
> > +if (index < c->meta_keys_count && c->meta_keys[index]) {
>
> If we have already allocated c->meta_keys_count * sizeof(*c->meta_keys)
> entries,
> the latter check is redundant.
>
Updated in the patch, thanks.
>
> > +key = c->meta_keys[index];
> > +}
>
> Perhaps some sort of warning if the index is out-of-range?
>
Updated in the patch, thanks.
>
> > +// Allocates enough space if data_type is a float32 number,
> otherwise
> > // worst-case requirement for output string in case of utf8 coded
> input
> > -str_size_alloc = (raw ? str_size : str_size * 2) + 1;
> > +num = (data_type == 23) ? 1 : 0;
>
> Redundant ternary operator.
>
Updated in the patch, thanks.
> > +str_size_alloc = (num ? 512 : (raw ? str_size : str_size * 2)) + 1;
> > str = av_mallocz(str_size_alloc);
> > if (!str)
> > return AVERROR(ENOMEM);
> > @@ -405,6 +413,10 @@ retry:
> > else {
> > if (!raw && (data_type == 3 || (data_type == 0 && (langcode <
> 0x400 || langcode == 0x7fff { // MAC Encoded
> > mov_read_mac_string(c, pb, str_size, str, str_size_alloc);
> > +} else if (data_type == 23 && str_size >= 4) { // BE float32
>
> Where does 4 come from?
>
Because data_type 23 indicates it's a float32 data stored, so we need make
sure the size of data >= (32/8 = 4).
>
> > +union av_intfloat32 val;
> > +val.i = avio_rb32(pb);
>
> I'm not entirely sure of the portability of this code. Would this not fail
> on
> any system without IEEE floats?
>
> > +snprintf(str, str_size_alloc, "%f", val.f);
>
> Return value should be checked when using %f, in case of insane input.
>
Updated in the patch, thanks.
>
> > @@ -614,6 +621,15 @@ static int mov_read_hdlr(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
> > av_log(c->fc, AV_LOG_TRACE, "ctype= %.4s (0x%08x)\n",
> (char*)&ctype, ctype);
> > av_log(c->fc, AV_LOG_TRACE, "stype= %.4s\n", (char*)&type);
> >
> > +if (c->fc->nb_streams < 1) { // meta before first trak
> > +if (type == MKTAG('m','d','t','a')) {
> > +c->found_hdlr_mdta = 1;
> > +}
> > +return 0;
> > +}
> > +
> > +st = c->fc->streams[c->fc->nb_streams-1];
>
> Any reason this was moved lower?
>
Because we need check the type of the hdlr before first trak.
>
> > +static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> > +{
> > +uint32_t count;
> > +uint32_t i;
> > +
> > +if (atom.size < 8)
> > +return 0;
> > +
> > +avio_skip(pb, 4);
> > +count = avio_rb32(pb);
> > +if (cou
[FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
The Apple dev specification:
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/Metadata/Metadata.html
Basically the structure is like:
|--meta
|hdlr
|keys
|ilst
1) The handler type in the metadata handler atom is ‘mdta’.
2) The key and value are stored separately for each key-value pair. 'keys' atom
stores the key table, while 'ilst' atom stores the values corresponding to the
indices in the key table.
---
libavformat/isom.h | 3 ++
libavformat/mov.c | 93 ++
2 files changed, 90 insertions(+), 6 deletions(-)
diff --git a/libavformat/isom.h b/libavformat/isom.h
index 6e921c0..dba30a2 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -177,6 +177,9 @@ typedef struct MOVContext {
int64_t duration; ///< duration of the longest track
int found_moov; ///< 'moov' atom has been found
int found_mdat; ///< 'mdat' atom has been found
+int found_hdlr_mdta; ///< 'hdlr' atom with type 'mdta' has been found
+char **meta_keys;
+unsigned meta_keys_count;
DVDemuxContext *dv_demux;
AVFormatContext *dv_fctx;
int isom; ///< 1 if file is ISO Media (mp4/3gp)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 7c90d40..c7905a1 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -265,6 +265,7 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext
*pb, MOVAtom atom)
uint32_t data_type = 0, str_size, str_size_alloc;
int (*parse)(MOVContext*, AVIOContext*, unsigned, const char*) = NULL;
int raw = 0;
+int num = 0;
switch (atom.type) {
case MKTAG( '@','P','R','M'): key = "premiere_version"; raw = 1; break;
@@ -368,6 +369,15 @@ retry:
av_log(c->fc, AV_LOG_ERROR, "Error parsing cover art.\n");
}
return ret;
+} else if (!key && c->found_hdlr_mdta && c->meta_keys) {
+uint32_t index = AV_RB32(&atom.type);
+if (index < c->meta_keys_count) {
+key = c->meta_keys[index];
+} else {
+av_log(c->fc, AV_LOG_WARNING,
+ "The index of 'data' is out of range: %d >= %d.\n",
+ index, c->meta_keys_count);
+}
}
} else return 0;
} else if (atom.size > 4 && key && !c->itunes_metadata && !raw) {
@@ -394,8 +404,10 @@ retry:
if (atom.size < 0 || str_size >= INT_MAX/2)
return AVERROR_INVALIDDATA;
+// Allocates enough space if data_type is a float32 number, otherwise
// worst-case requirement for output string in case of utf8 coded input
-str_size_alloc = (raw ? str_size : str_size * 2) + 1;
+num = (data_type == 23);
+str_size_alloc = (num ? 512 : (raw ? str_size : str_size * 2)) + 1;
str = av_mallocz(str_size_alloc);
if (!str)
return AVERROR(ENOMEM);
@@ -405,6 +417,14 @@ retry:
else {
if (!raw && (data_type == 3 || (data_type == 0 && (langcode < 0x400 ||
langcode == 0x7fff { // MAC Encoded
mov_read_mac_string(c, pb, str_size, str, str_size_alloc);
+} else if (data_type == 23 && str_size >= 4) { // BE float32
+union av_intfloat32 val;
+val.i = avio_rb32(pb);
+if (snprintf(str, str_size_alloc, "%f", val.f) >= str_size_alloc) {
+av_log(c->fc, AV_LOG_ERROR,
+ "Failed to store the float32 number (0x%x) in
string.\n", val.i);
+return AVERROR_INVALIDDATA;
+}
} else {
int ret = ffio_read_size(pb, str, str_size);
if (ret < 0) {
@@ -599,11 +619,6 @@ static int mov_read_hdlr(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
char *title_str;
int ret;
-if (c->fc->nb_streams < 1) // meta before first trak
-return 0;
-
-st = c->fc->streams[c->fc->nb_streams-1];
-
avio_r8(pb); /* version */
avio_rb24(pb); /* flags */
@@ -614,6 +629,15 @@ static int mov_read_hdlr(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
av_log(c->fc, AV_LOG_TRACE, "ctype= %.4s (0x%08x)\n", (char*)&ctype,
ctype);
av_log(c->fc, AV_LOG_TRACE, "stype= %.4s\n", (char*)&type);
+if (c->fc->nb_streams < 1) { // meta before first trak
+if (type == MKTAG('m','d','t','a')) {
+c->found_hdlr_mdta = 1;
+}
+return 0;
+}
+
+st = c->fc->streams[c->fc->nb_streams-1];
+
if (type == MKTAG('v','i','d','e'))
st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
else if (type == MKTAG('s','o','u','n'))
@@ -3127,6 +3151,48 @@ static int mov_read_ilst(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
return ret;
}
+static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+uint32_t count;
+uint32_t i;
+
+if (atom.size < 8)
+return 0;
+
+avio_skip(pb, 4);
+count = avio_rb32(pb);
+
Re: [FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
On 10/20/2015 7:29 PM, Tinglin Liu wrote:
> The Apple dev specification:
> https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/Metadata/Metadata.html
> ---
> libavformat/isom.h | 3 +++
> libavformat/mov.c | 77
> +-
> 2 files changed, 74 insertions(+), 6 deletions(-)
Is there a test file around we can add to FATE?
> @@ -177,6 +177,9 @@ typedef struct MOVContext {
> int64_t duration; ///< duration of the longest track
> int found_moov; ///< 'moov' atom has been found
> int found_mdat; ///< 'mdat' atom has been found
> +int found_hdlr_mdta; ///< 'hdlr' atom with type 'mdta' has been found
> +char **meta_keys;
> +unsigned meta_keys_count;
How exactly is it intended for these gets to be accessed? MOVContext is an
internal
structure only. Perhaps this should be exported via side-data?
> @@ -368,6 +369,11 @@ retry:
> av_log(c->fc, AV_LOG_ERROR, "Error parsing cover
> art.\n");
> }
> return ret;
> +} else if (!key && c->found_hdlr_mdta && c->meta_keys) {
Is it ever possible for c->meta_keys to not be allocated here? (Patch doesn't
provide me enough context liens to determine that.)
> +uint32_t index = AV_RB32(&atom.type);
> +if (index < c->meta_keys_count && c->meta_keys[index]) {
If we have already allocated c->meta_keys_count * sizeof(*c->meta_keys) entries,
the latter check is redundant.
> +key = c->meta_keys[index];
> +}
Perhaps some sort of warning if the index is out-of-range?
> +// Allocates enough space if data_type is a float32 number, otherwise
> // worst-case requirement for output string in case of utf8 coded input
> -str_size_alloc = (raw ? str_size : str_size * 2) + 1;
> +num = (data_type == 23) ? 1 : 0;
Redundant ternary operator.
> +str_size_alloc = (num ? 512 : (raw ? str_size : str_size * 2)) + 1;
> str = av_mallocz(str_size_alloc);
> if (!str)
> return AVERROR(ENOMEM);
> @@ -405,6 +413,10 @@ retry:
> else {
> if (!raw && (data_type == 3 || (data_type == 0 && (langcode < 0x400
> || langcode == 0x7fff { // MAC Encoded
> mov_read_mac_string(c, pb, str_size, str, str_size_alloc);
> +} else if (data_type == 23 && str_size >= 4) { // BE float32
Where does 4 come from?
> +union av_intfloat32 val;
> +val.i = avio_rb32(pb);
I'm not entirely sure of the portability of this code. Would this not fail on
any system without IEEE floats?
> +snprintf(str, str_size_alloc, "%f", val.f);
Return value should be checked when using %f, in case of insane input.
> @@ -614,6 +621,15 @@ static int mov_read_hdlr(MOVContext *c, AVIOContext *pb,
> MOVAtom atom)
> av_log(c->fc, AV_LOG_TRACE, "ctype= %.4s (0x%08x)\n", (char*)&ctype,
> ctype);
> av_log(c->fc, AV_LOG_TRACE, "stype= %.4s\n", (char*)&type);
>
> +if (c->fc->nb_streams < 1) { // meta before first trak
> +if (type == MKTAG('m','d','t','a')) {
> +c->found_hdlr_mdta = 1;
> +}
> +return 0;
> +}
> +
> +st = c->fc->streams[c->fc->nb_streams-1];
Any reason this was moved lower?
> +static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> +{
> +uint32_t count;
> +uint32_t i;
> +
> +if (atom.size < 8)
> +return 0;
> +
> +avio_skip(pb, 4);
> +count = avio_rb32(pb);
> +if (count > UINT_MAX / sizeof(*c->meta_keys))
> +return 0;
This shouldn't really return success.
Also, probably could do with a warning.
> +av_freep(&c->meta_keys);
Why are we freeing this here? It should not be set at all, or it should not be
overwritten silently, no?
> +c->meta_keys_count = count + 1;
Some may complain we are wasting 1 entry's worth of memory ;)... not that I
particularily
care, but it may end up with some funny bugs later on if others assume a
0-origin.
> +c->meta_keys = av_mallocz(c->meta_keys_count * sizeof(*c->meta_keys));
> +if (!c->meta_keys)
> +return AVERROR(ENOMEM);
> +
> +for (i = 1; i <= count; ++i) {
> +uint32_t key_size = avio_rb32(pb);
> +uint32_t type = avio_rl32(pb);
> +if (type != MKTAG('m','d','t','a') || key_size < 8)
> +return 0;
Logging? Also, is it reasonable to return success here as you do?
The rest is just simple stuff like inconsistent use of braces and
post/pre-increment,
and not worth noting.
- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] Adds support parsing the QuickTime Metadata Keys.
The Apple dev specification:
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/Metadata/Metadata.html
---
libavformat/isom.h | 3 +++
libavformat/mov.c | 77 +-
2 files changed, 74 insertions(+), 6 deletions(-)
diff --git a/libavformat/isom.h b/libavformat/isom.h
index 6e921c0..dba30a2 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -177,6 +177,9 @@ typedef struct MOVContext {
int64_t duration; ///< duration of the longest track
int found_moov; ///< 'moov' atom has been found
int found_mdat; ///< 'mdat' atom has been found
+int found_hdlr_mdta; ///< 'hdlr' atom with type 'mdta' has been found
+char **meta_keys;
+unsigned meta_keys_count;
DVDemuxContext *dv_demux;
AVFormatContext *dv_fctx;
int isom; ///< 1 if file is ISO Media (mp4/3gp)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 7c90d40..1ddd8cb 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -265,6 +265,7 @@ static int mov_read_udta_string(MOVContext *c, AVIOContext
*pb, MOVAtom atom)
uint32_t data_type = 0, str_size, str_size_alloc;
int (*parse)(MOVContext*, AVIOContext*, unsigned, const char*) = NULL;
int raw = 0;
+int num = 0;
switch (atom.type) {
case MKTAG( '@','P','R','M'): key = "premiere_version"; raw = 1; break;
@@ -368,6 +369,11 @@ retry:
av_log(c->fc, AV_LOG_ERROR, "Error parsing cover art.\n");
}
return ret;
+} else if (!key && c->found_hdlr_mdta && c->meta_keys) {
+uint32_t index = AV_RB32(&atom.type);
+if (index < c->meta_keys_count && c->meta_keys[index]) {
+key = c->meta_keys[index];
+}
}
} else return 0;
} else if (atom.size > 4 && key && !c->itunes_metadata && !raw) {
@@ -394,8 +400,10 @@ retry:
if (atom.size < 0 || str_size >= INT_MAX/2)
return AVERROR_INVALIDDATA;
+// Allocates enough space if data_type is a float32 number, otherwise
// worst-case requirement for output string in case of utf8 coded input
-str_size_alloc = (raw ? str_size : str_size * 2) + 1;
+num = (data_type == 23) ? 1 : 0;
+str_size_alloc = (num ? 512 : (raw ? str_size : str_size * 2)) + 1;
str = av_mallocz(str_size_alloc);
if (!str)
return AVERROR(ENOMEM);
@@ -405,6 +413,10 @@ retry:
else {
if (!raw && (data_type == 3 || (data_type == 0 && (langcode < 0x400 ||
langcode == 0x7fff { // MAC Encoded
mov_read_mac_string(c, pb, str_size, str, str_size_alloc);
+} else if (data_type == 23 && str_size >= 4) { // BE float32
+union av_intfloat32 val;
+val.i = avio_rb32(pb);
+snprintf(str, str_size_alloc, "%f", val.f);
} else {
int ret = ffio_read_size(pb, str, str_size);
if (ret < 0) {
@@ -599,11 +611,6 @@ static int mov_read_hdlr(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
char *title_str;
int ret;
-if (c->fc->nb_streams < 1) // meta before first trak
-return 0;
-
-st = c->fc->streams[c->fc->nb_streams-1];
-
avio_r8(pb); /* version */
avio_rb24(pb); /* flags */
@@ -614,6 +621,15 @@ static int mov_read_hdlr(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
av_log(c->fc, AV_LOG_TRACE, "ctype= %.4s (0x%08x)\n", (char*)&ctype,
ctype);
av_log(c->fc, AV_LOG_TRACE, "stype= %.4s\n", (char*)&type);
+if (c->fc->nb_streams < 1) { // meta before first trak
+if (type == MKTAG('m','d','t','a')) {
+c->found_hdlr_mdta = 1;
+}
+return 0;
+}
+
+st = c->fc->streams[c->fc->nb_streams-1];
+
if (type == MKTAG('v','i','d','e'))
st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
else if (type == MKTAG('s','o','u','n'))
@@ -3127,6 +3143,40 @@ static int mov_read_ilst(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
return ret;
}
+static int mov_read_keys(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+uint32_t count;
+uint32_t i;
+
+if (atom.size < 8)
+return 0;
+
+avio_skip(pb, 4);
+count = avio_rb32(pb);
+if (count > UINT_MAX / sizeof(*c->meta_keys))
+return 0;
+
+av_freep(&c->meta_keys);
+c->meta_keys_count = count + 1;
+c->meta_keys = av_mallocz(c->meta_keys_count * sizeof(*c->meta_keys));
+if (!c->meta_keys)
+return AVERROR(ENOMEM);
+
+for (i = 1; i <= count; ++i) {
+uint32_t key_size = avio_rb32(pb);
+uint32_t type = avio_rl32(pb);
+if (type != MKTAG('m','d','t','a') || key_size < 8)
+return 0;
+key_size -= 8;
+c->meta_keys[i] = av_mallocz(key_size + 1);
+if (!c->meta_keys[i])
+return AVERROR(ENOMEM);
+avio_read(pb, c->meta_keys[i], key_size);
+}
+
+return 0;
+}
+
static int mov_read_
