Trying to compile cmus from git fails at mp4.lo

Discussion:

Evan

2010-05-16 03:39:44 UTC

Hello,

I'm trying to compile cmus from the git repository and make fails when
it gets to mp4.lo. The output from make is:

CC mp4.lo
mp4.c: In function 'mp4_open':
mp4.c:147: warning: passing argument 4 of 'NeAACDecInit2' from incompatible pointer type
/usr/local/include/neaacdec.h:220: note: expected 'long unsigned int *' but argument is of type 'uint32_t *'
mp4.c: In function 'mp4_read_comments':
mp4.c:331: error: implicit declaration of function 'MP4GetMetadataArtist'
mp4.c:333: error: implicit declaration of function 'MP4GetMetadataAlbum'
mp4.c:335: error: implicit declaration of function 'MP4GetMetadataName'
mp4.c:337: error: implicit declaration of function 'MP4GetMetadataGenre'
mp4.c:339: error: implicit declaration of function 'MP4GetMetadataYear'
mp4.c:342: error: implicit declaration of function 'MP4GetMetadataCompilation'
mp4.c:366: error: implicit declaration of function 'MP4GetMetadataTrack'
mp4.c:371: error: implicit declaration of function 'MP4GetMetadataDisk'
make: *** [mp4.lo] Error 1

I have mp4v2 installed, and I've tried googling for
MP4GetMetadataArtist, along with searching the cmus-devel archives, but
I can't find any reference to MP4GetMetadataArtist outside of mp4.c.
What am I missing?

Thanks.

Gregory Petrosyan

2010-05-16 09:37:24 UTC

Permalink

Post by Evan
I'm trying to compile cmus from the git repository and make fails when
CC mp4.lo
mp4.c:147: warning: passing argument 4 of 'NeAACDecInit2' from incompatible pointer type
/usr/local/include/neaacdec.h:220: note: expected 'long unsigned int *' but argument is of type 'uint32_t *'
mp4.c:331: error: implicit declaration of function 'MP4GetMetadataArtist'
mp4.c:333: error: implicit declaration of function 'MP4GetMetadataAlbum'
mp4.c:335: error: implicit declaration of function 'MP4GetMetadataName'
mp4.c:337: error: implicit declaration of function 'MP4GetMetadataGenre'
mp4.c:339: error: implicit declaration of function 'MP4GetMetadataYear'
mp4.c:342: error: implicit declaration of function 'MP4GetMetadataCompilation'
mp4.c:366: error: implicit declaration of function 'MP4GetMetadataTrack'
mp4.c:371: error: implicit declaration of function 'MP4GetMetadataDisk'
make: *** [mp4.lo] Error 1
I have mp4v2 installed, and I've tried googling for
MP4GetMetadataArtist, along with searching the cmus-devel archives, but
I can't find any reference to MP4GetMetadataArtist outside of mp4.c.
What am I missing?

Looks like cmus depends on API that is marked as deprecated in mp4v2:
http://mp4v2.googlecode.com/svn/doc/1.9.0/api/group__mp4__meta.html

And you have recent enough version of libmp4v2 with this API removed.

It should be fairly easy to modify cmus to support both the legacy API (which
is exposed in /usr/include/mp4.h) and the new one: the only things that should
be done are

- rename MP4_USE_OLD_HEADER to something more meaningful
- modify mp4_read_comments in mp4.c to conditionally use old or new API
(http://mp4v2.googlecode.com/svn/doc/1.9.0/api/example_2itmf_2tags_8c-example.html)

If you can send a patch for this, it will be very appreciated (I don't have
newer libmp4v2 installed ATM).

Gregory

Evan Niessen-Derry

2010-05-18 21:41:16 UTC

Permalink

This patch should do the trick. I didn't include support for every
metadata type, only the ones already present. Let me know if I missed
something (though I don't think I did).
Patch: http://cmus.pastebin.com/VbszeUeq

Post by Gregory Petrosyan

http://mp4v2.googlecode.com/svn/doc/1.9.0/api/group__mp4__meta.html
And you have recent enough version of libmp4v2 with this API removed.
It should be fairly easy to modify cmus to support both the legacy API (which
is exposed in /usr/include/mp4.h) and the new one: the only things that should
be done are
- rename MP4_USE_OLD_HEADER to something more meaningful
- modify mp4_read_comments in mp4.c to conditionally use old or new API
(http://mp4v2.googlecode.com/svn/doc/1.9.0/api/example_2itmf_2tags_8c-example.html)
If you can send a patch for this, it will be very appreciated (I don't have
newer libmp4v2 installed ATM).
Gregory

Evan Niessen-Derry

2010-05-18 21:54:11 UTC

Permalink

I may have overlooked something, because when I added a bunch of m4a
files to my library, the names were definitely not correct. So the patch
I just wrote should maybe be ignored for the moment (seems I did
miss something after all). I'll send out again when I figure out what's
wrong.

Post by Evan Niessen-Derry
This patch should do the trick. I didn't include support for every
metadata type, only the ones already present. Let me know if I missed
something (though I don't think I did).
Patch: http://cmus.pastebin.com/VbszeUeq

<SNIP>

Post by Evan Niessen-Derry
------------------------------------------------------------------------------

Gregory Petrosyan

2010-05-18 22:03:59 UTC

Permalink

Post by Evan Niessen-Derry
I may have overlooked something, because when I added a bunch of m4a
files to my library, the names were definitely not correct. So the patch
I just wrote should maybe be ignored for the moment (seems I did
miss something after all). I'll send out again when I figure out what's
wrong.

BTW, this

+ if(tags->releaseDate)
+ comments_add(&c, "date", (char *)tags->genre);

does not look right.

Gregory

Evan Niessen-Derry

2010-05-22 17:23:38 UTC

Permalink

My understanding of the issue with messed up artist names for mp4v2 is,
in keyval.c, in keyvals_add, when it adds the value to the key pair,
it's not making a new copy of the string, and instead is using the
memory address, which then later get's written over with something else,
thus causing messed up names. I noticed this by adding one m4a file,
looking at it in the library, noticing it's fine, adding another one,
looking at them in the library and they're completely messed up.

The strange thing is that this didn't happen for anything but m4a files,
so maybe it has something to do with freeing the "tags" struct. Not
sure.

Anyways, TL;DR: here's the patch, I read over it a couple times, doesn't
mean I didn't miss something, so let me know if I did.
http://cmus.pastebin.com/49Va4ni0

Gregory Petrosyan

2010-05-23 14:44:20 UTC

Permalink

Post by Evan Niessen-Derry
My understanding of the issue with messed up artist names for mp4v2 is,
in keyval.c, in keyvals_add, when it adds the value to the key pair,
it's not making a new copy of the string, and instead is using the
memory address, which then later get's written over with something else,
thus causing messed up names. I noticed this by adding one m4a file,
looking at it in the library, noticing it's fine, adding another one,
looking at them in the library and they're completely messed up.

You've found the source of the problem, but your solution is wrong –
keyvals_add takes a 'char *' (non-const) pointer, and that indicates
that you should take care of allocating a copy on a caller side. So,
basically, you should do 'comments_add(xstrdup(val))' instead of
'comments_add(val)'.

Post by Evan Niessen-Derry
Anyways, TL;DR: here's the patch, I read over it a couple times, doesn't
mean I didn't miss something, so let me know if I did.
http://cmus.pastebin.com/49Va4ni0

Can you please fix the patch and resubmit once more? Also, attaching
git-format-patch'ed patches to email is the preferred way of
submitting them – that way, it is much easier to review them, and
reply, quoting code in question.

Gregory

Evan Niessen-Derry

2010-05-23 23:16:52 UTC

Permalink

On Sun, May 23, 2010 at 06:44:20PM +0400, Gregory Petrosyan wrote:
<snip>

Post by Gregory Petrosyan
Can you please fix the patch and resubmit once more? Also, attaching
git-format-patch'ed patches to email is the preferred way of
submitting them ??? that way, it is much easier to review them, and
reply, quoting code in question.
Gregory

I'm not really sure how to submit this patch, as an attachment, or as
it's own e-mail, so I guess I'll do some amalgamation of both. Let me know.

-Evan

Gregory Petrosyan

2010-05-24 17:52:38 UTC

Permalink

From: Evan Niessen-Derry <***@cs.umn.edu>

[gp: clean up a bit, add support for more tags]
Signed-off-by: Gregory Petrosyan <***@gmail.com>
---

Post by Evan Niessen-Derry
I'm not really sure how to submit this patch, as an attachment, or as
it's own e-mail, so I guess I'll do some amalgamation of both. Let me know.

IMO the best way is to use 'git send-email', but attachment is fine as well.
Manually inserting patch in the message body can result in damaged whitespace.

Can you please verify that this version of the patch works for you? (You can
find it in the new 'mp4v2' branch of the repo).

configure | 4 ++--
mp4.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index c17e1cb..4ed6eb2 100755
--- a/configure
+++ b/configure
@@ -263,7 +263,7 @@ check_waveout()
check_mp4()
{
check_header mp4v2/mp4v2.h
- MP4_USE_OLD_HEADER=$?
+ USE_MPEG4IP=$?
check_header neaacdec.h &&
check_library MP4 "" "-lmp4v2 -lfaad -lm"
return $?
@@ -387,7 +387,7 @@ config_header config/libdir.h LIBDIR
config_header config/debug.h DEBUG
config_header config/tremor.h CONFIG_TREMOR
config_header config/mpc.h MPC_SV7
-config_header config/mp4.h MP4_USE_OLD_HEADER
+config_header config/mp4.h USE_MPEG4IP
config_header config/curses.h HAVE_RESIZETERM HAVE_USE_DEFAULT_COLORS

makefile_vars bindir datadir libdir mandir exampledir
diff --git a/mp4.c b/mp4.c
index a26569d..9c5d4f2 100644
--- a/mp4.c
+++ b/mp4.c
@@ -23,7 +23,7 @@
#include "file.h"
#include "config/mp4.h"

-#if MP4_USE_OLD_HEADER
+#if USE_MPEG4IP
#include <mp4.h>
#else
#include <mp4v2/mp4v2.h>
@@ -317,15 +317,20 @@ static int mp4_read_comments(struct input_plugin_data *ip_data,
struct keyval **comments)
{
struct mp4_private *priv;
+#if USE_MPEG4IP
uint16_t meta_num, meta_total;
uint8_t val;
+ char *str;
/*uint8_t *ustr;
uint32_t size;*/
- char *str;
+#else
+ const MP4Tags *tags;
+#endif
GROWING_KEYVALS(c);

priv = ip_data->private;

+#if USE_MPEG4IP
/* MP4GetMetadata* provides malloced pointers, and the data
* is in UTF-8 (or at least it should be). */
if (MP4GetMetadataArtist(priv->mp4.handle, &str))
@@ -374,6 +379,43 @@ static int mp4_read_comments(struct input_plugin_data *ip_data,
comments_add_const(&c, "discnumber", buf);
}

+#else /* !USE_MPEG4IP, new interface */
+
+ tags = MP4TagsAlloc();
+
+ MP4TagsFetch(tags, priv->mp4.handle);
+
+ if (tags->artist)
+ comments_add_const(&c, "artist", tags->artist);
+ if (tags->albumArtist)
+ comments_add_const(&c, "albumartist", tags->albumArtist);
+ if (tags->sortArtist)
+ comments_add_const(&c, "artistsort", tags->sortArtist);
+ if (tags->sortAlbumArtist)
+ comments_add_const(&c, "albumartistsort", tags->sortAlbumArtist);
+ if (tags->album)
+ comments_add_const(&c, "album", tags->album);
+ if (tags->name)
+ comments_add_const(&c, "title", tags->name);
+ if (tags->releaseDate)
+ comments_add_const(&c, "date", tags->releaseDate);
+ if (tags->compilation)
+ comments_add_const(&c, "compilation", *tags->compilation ? "yes" : "no");
+ if (tags->track) {
+ char buf[6];
+ snprintf(buf, 6, "%u", tags->track->index);
+ comments_add_const(&c, "tracknumber", buf);
+ }
+ if (tags->disc) {
+ char buf[6];
+ snprintf(buf, 6, "%u", tags->disc->index);
+ comments_add_const(&c, "discnumber", buf);
+ }
+
+ MP4TagsFree(tags);
+
+#endif
+
keyvals_terminate(&c);
*comments = c.keyvals;
return 0;

--
1.7.0.4

Xavier Chantry

2010-05-24 18:33:05 UTC

Permalink

On Mon, May 24, 2010 at 7:52 PM, Gregory Petrosyan

Post by Gregory Petrosyan
[gp: clean up a bit, add support for more tags]
---

Post by Evan Niessen-Derry
I'm not really sure how to submit this patch, as an attachment, or as
it's own e-mail, so I guess I'll do some amalgamation of both. Let me know.

IMO the best way is to use 'git send-email', but attachment is fine as well.
Manually inserting patch in the message body can result in damaged whitespace.
Can you please verify that this version of the patch works for you? (You can
find it in the new 'mp4v2' branch of the repo).
configure | 4 ++--
mp4.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/configure b/configure
index c17e1cb..4ed6eb2 100755
--- a/configure
+++ b/configure
@@ -263,7 +263,7 @@ check_waveout()
check_mp4()
{
check_header mp4v2/mp4v2.h
- MP4_USE_OLD_HEADER=$?
+ USE_MPEG4IP=$?
check_header neaacdec.h &&
check_library MP4 "" "-lmp4v2 -lfaad -lm"
return $?
@@ -387,7 +387,7 @@ config_header config/libdir.h LIBDIR
config_header config/debug.h DEBUG
config_header config/tremor.h CONFIG_TREMOR
config_header config/mpc.h MPC_SV7
-config_header config/mp4.h MP4_USE_OLD_HEADER
+config_header config/mp4.h USE_MPEG4IP
config_header config/curses.h HAVE_RESIZETERM HAVE_USE_DEFAULT_COLORS
makefile_vars bindir datadir libdir mandir exampledir
diff --git a/mp4.c b/mp4.c
index a26569d..9c5d4f2 100644
--- a/mp4.c
+++ b/mp4.c
@@ -23,7 +23,7 @@
#include "file.h"
#include "config/mp4.h"
-#if MP4_USE_OLD_HEADER
+#if USE_MPEG4IP
#include <mp4.h>
#else
#include <mp4v2/mp4v2.h>
@@ -317,15 +317,20 @@ static int mp4_read_comments(struct input_plugin_data *ip_data,
struct keyval **comments)
{
struct mp4_private *priv;
+#if USE_MPEG4IP
uint16_t meta_num, meta_total;
uint8_t val;
+ char *str;
/*uint8_t *ustr;
uint32_t size;*/
- char *str;
+#else
+ const MP4Tags *tags;
+#endif
GROWING_KEYVALS(c);
priv = ip_data->private;
+#if USE_MPEG4IP
/* MP4GetMetadata* provides malloced pointers, and the data
* is in UTF-8 (or at least it should be). */
if (MP4GetMetadataArtist(priv->mp4.handle, &str))
@@ -374,6 +379,43 @@ static int mp4_read_comments(struct input_plugin_data *ip_data,
comments_add_const(&c, "discnumber", buf);
}
+#else /* !USE_MPEG4IP, new interface */
+
+ tags = MP4TagsAlloc();
+
+ MP4TagsFetch(tags, priv->mp4.handle);
+
+ if (tags->artist)
+ comments_add_const(&c, "artist", tags->artist);
+ if (tags->albumArtist)
+ comments_add_const(&c, "albumartist", tags->albumArtist);
+ if (tags->sortArtist)
+ comments_add_const(&c, "artistsort", tags->sortArtist);
+ if (tags->sortAlbumArtist)
+ comments_add_const(&c, "albumartistsort", tags->sortAlbumArtist);
+ if (tags->album)
+ comments_add_const(&c, "album", tags->album);
+ if (tags->name)
+ comments_add_const(&c, "title", tags->name);
+ if (tags->releaseDate)
+ comments_add_const(&c, "date", tags->releaseDate);
+ if (tags->compilation)
+ comments_add_const(&c, "compilation", *tags->compilation ? "yes" : "no");
+ if (tags->track) {
+ char buf[6];
+ snprintf(buf, 6, "%u", tags->track->index);
+ comments_add_const(&c, "tracknumber", buf);
+ }
+ if (tags->disc) {
+ char buf[6];
+ snprintf(buf, 6, "%u", tags->disc->index);
+ comments_add_const(&c, "discnumber", buf);
+ }
+

It seems the field is disk, not disc.
http://www.google.com/codesearch/p?hl=en#gD9_8o1fFSU/trunk/include/mp4v2/itmf_tags.h&q=disk%20package:http://mp4v2\.googlecode\.com&sa=N&cd=8&ct=rc&d=3

diff --git a/mp4.c b/mp4.c
index 9c5d4f2..07ada77 100644
--- a/mp4.c
+++ b/mp4.c
@@ -406,9 +406,9 @@ static int mp4_read_comments(struct
input_plugin_data *ip_data,
snprintf(buf, 6, "%u", tags->track->index);
comments_add_const(&c, "tracknumber", buf);
}
- if (tags->disc) {
+ if (tags->disk) {
char buf[6];
- snprintf(buf, 6, "%u", tags->disc->index);
+ snprintf(buf, 6, "%u", tags->disk->index);
comments_add_const(&c, "discnumber", buf);
}

Otherwise, it builds and plays fine a mp4 file I just converted, but I
didn't find a proper mp4 file with tags yet.

Gregory Petrosyan

2010-05-24 18:40:17 UTC

Permalink

On Mon, May 24, 2010 at 10:33 PM, Xavier Chantry

Post by Xavier Chantry
On Mon, May 24, 2010 at 7:52 PM, Gregory Petrosyan

Post by Gregory Petrosyan
[gp: clean up a bit, add support for more tags]

It seems the field is disk, not disc.

Thanks a lot, good catch!

Gregory

Gregory Petrosyan

2010-05-24 19:33:38 UTC

Permalink

On Mon, May 24, 2010 at 10:40 PM, Gregory Petrosyan

Post by Gregory Petrosyan
On Mon, May 24, 2010 at 10:33 PM, Xavier Chantry

Post by Xavier Chantry
On Mon, May 24, 2010 at 7:52 PM, Gregory Petrosyan

Post by Gregory Petrosyan
[gp: clean up a bit, add support for more tags]

It seems the field is disk, not disc.

Thanks a lot, good catch!

Merged into -maint.

Gregory

Evan Niessen-Derry

2010-05-24 20:55:24 UTC

Permalink

Works perfectly for all of the m4a files I have (all of which have
tags).

Post by Gregory Petrosyan
On Mon, May 24, 2010 at 10:40 PM, Gregory Petrosyan

Post by Gregory Petrosyan
On Mon, May 24, 2010 at 10:33 PM, Xavier Chantry

Post by Xavier Chantry
On Mon, May 24, 2010 at 7:52 PM, Gregory Petrosyan

Post by Gregory Petrosyan
[gp: clean up a bit, add support for more tags]

It seems the field is disk, not disc.

Thanks a lot, good catch!

Merged into -maint.
Gregory
------------------------------------------------------------------------------

Xavier Chantry

2010-05-24 21:33:58 UTC

Permalink

Post by Evan Niessen-Derry
Works perfectly for all of the m4a files I have (all of which have
tags).

Ahah I was looking for mp4 files. I actually had 2-3 albums in m4a, I
confirm the followings tags were displayed fine :
:set format_current='%a %l %D %n %t %g %y
artist / album / disc number / track number / title / genre / year