Comment 1 Andre Klapper 2009-12-29 12:34:05 UTC

This is not a forum but a bugtracker. I tend to close this as INVALID as I
cannot see a bug report here...

(In reply to comment #1)
> This is not a forum but a bugtracker. I tend to close this as INVALID as I
> cannot see a bug report here...

You're absolutely right.

I don't know if it is a bug or not, because the user guide doesn't mention it
(page 72-77), and I haven't been able to locate any other documentation that
describes the Media player.

I would have looked at the code - as I have with the email application - but I
can't find the repository for the Media player.

Feel free to close this bug (preferably INVALID if it isn't supposed to work
with cover.jpg and WORKSFORME if it should and does for you - thanks).

Please note that the forum on maemo.org has at least two threads with
contradictory information about this.

(Sorry about the discussionlike nature of the first attempt at this bug report;
here is a hopefully better try:)

SOFTWARE VERSION:
(Settings > General > About product)
1.2009.42-11

EXACT STEPS LEADING TO PROBLEM: 
(Explain in detail what you do (e.g. tap on OK) and what you see (e.g. message
Connection Failed appears))
1. Transfer an album in Ogg Vorbis format to the N900
2. Open the Media player
3. Browse to the new album

EXPECTED OUTCOME:
Album cover art is shown, either from picture embedded via tags (marked
Cover.front, or not), or from the file cover.jpg in the directory.

ACTUAL OUTCOME:
No cover art is shown, just the default turntable icon.

REPRODUCIBILITY:
(always, less than 1/10, 5/10, 9/10)
always (for Ogg Vorbis; embedded cover art works for MP3)

EXTRA SOFTWARE INSTALLED:
~ $ dpkg -l '*ogg*' | grep -v log | grep ^ii
ii  gstreamer0.10-ogg   0.10.23-0maemo4+0m5-tk1  GStreamer ogg plugin from the
"base" set
ii  libogg0             1:1.1.3-2.tk2            Ogg Bitstream Library
ii  ogg-support         1.0.5                    Ogg support for n900

OTHER COMMENTS:
Any tips on how to debug/contribute to fixing this is much appreciated.

User-Agent:       Mozilla/5.0 (X11; U; Linux armv7l; en-GB; rv:1.9.2a1pre)
Gecko/20090928 Firefox/3.5 Maemo Browser 1.4.1.21 RX-51 N900

Comment 4 Andre Klapper 2009-12-30 21:59:35 UTC

CC'ing Tuomas.
Tuomas, do you know if this is something that ogg-support should/could handle,
or a tracker issue?

(In reply to comment #4)
> CC'ing Tuomas.
> Tuomas, do you know if this is something that ogg-support should/could handle,
> or a tracker issue?

Unfortunately I don't know how this should work. My wild guess is that the
tracker-extractor-vorbis (installed as a dependency for ogg-support) should
somehow read the cover art from the meta information of the vorbis file. The
Ogg/Vorbis tags are in the Vorbis bitstream instead of the Ogg container but I
don't know where the album art should be in Ogg/Vorbis case.

Tracker is open source so somebody eager enough could dig out the information
from the Tracker's mp3 extractor[1] and see how it should work. Of course it
would be nice, if that would be already documented somewhere.

In any case, I think this should be moved to Extras / Ogg Support instead of
resolving as invalid.

[1] cd /tmp && dget
http://repository.maemo.org/pool/maemo5.0/free/t/tracker/tracker_0.6.95-20maemo1+0m5.dsc
; dpkg-source -x tracker_0.6.95-20maemo1+0m5.dsc && cd tracker-0.6.95 && vi
src/tracker-extract/tracker-extract-mp3.c

(In reply to comment #5)

> Unfortunately I don't know how this should work. My wild guess is that the
> tracker-extractor-vorbis (installed as a dependency for ogg-support) should
> somehow read the cover art from the meta information of the vorbis file.

Your wild guess seems quite correct.

tracker-extractor-vorbis seems to be the part of the tracker package that
handles ogg vorbis files (which isn't included in the fremantle tracker
package).

The short term solution would be to expand tracker-extractor-vorbis to handle
the coverart (base64 encoded image) and coverartmime (mime type) fields from
Ogg Vorbis files.

I guess the long term solution would be to include the same Ogg Vorbis support
directly in the tracker package (which would then depend on libvorbis).

> The
> Tracker is open source so somebody eager enough could dig out the information
> from the Tracker's mp3 extractor[1] and see how it should work. Of course it
> would be nice, if that would be already documented somewhere.

Thanks for the pointer - very helpful!

I will try to put together a patch if nobody beats me to it.

> In any case, I think this should be moved to Extras / Ogg Support instead of
> resolving as invalid.

As you point out, it it a tracker-thing, not a Media player thing.

Thanks again for the pointers!

Best regards,
Adam

(In reply to comment #6)
> tracker-extractor-vorbis seems to be the part of the tracker package that
> handles ogg vorbis files (which isn't included in the fremantle tracker
> package).

> I guess the long term solution would be to include the same Ogg Vorbis support
> directly in the tracker package (which would then depend on libvorbis).

Nokia doesn't provide Ogg support and therefore they can't compile Tracker so
that the Ogg/Vorbis support would be included. That's why
tracker-extractor-vorbis is provided now as a separate package although it's
pretty much just a copy of the code in the Tracker.

Upstream GStreamer is evolving and will include proper support for Vorbis tags
via oggdemux and after that's included in Maemo, the Tracker's generic
GStreamer extractor can extract vorbis tags without extra components. But that
may take quite a while.

If somebody patches the tracker-extractor-vorbis to have album support, it's
easy for me to integrate to Ogg Support.

Andre, is it possible to move this to Ogg Support product?

Comment 8 Andre Klapper 2010-01-01 18:54:45 UTC

Sure. :)

I've been looking at how to do this, so *might* beat Adam to creating a patch.
Unfortunately I haven't been able to figure out how to do it without
duplicating the code from the MP3 extractor. (And even if I did, that would be
relying on the MP3 extractor internals, which could potentially change with the
next update.)

So the solution is going to involve copying tracker-extract-albumart.c from the
tracker package to the tracker-extractor-vorbis package.

*** This bug has been confirmed by popular vote. ***

Created an attachment (id=1915) [details]
Add album art support to Ivan Frade's tracker-extractor-vorbis 0.1-2maemo2

This patch works quite well for me - I have around 2100 songs with cover art
extracted from Ogg Vorbis files on my N900 now.

There are one or two glitches that I haven't had time to work out yet (Sigur
Rós album "()" gets an image of Beck's song "MTV Makes Me Wanna Smoke Crack",
but I think that may be because of the parens, and not the plugin?)

Some notes on the patch:

 * The package now includes the files tracker-extract-albumart.{c,h} from the
tracker 0.6.95-20maemo1+0m5 package, which is also used in the MP3 extractor.

 * The package now links against libtracker-common, but the header files for
that library isn't packaged by the tracker source package, so I have simply
copied them into tracker-extractor-vorbis.

 * Linking against libtracker-common also means that the tracker package is now
a build dependency.

 * Album art is extracted from the COVERART and COVERARTMIME comments.

 * To decode the base64-encoded COVERART, I have included cdecode.{c,h} from
libb64, which is in the Public Domain. I'm sure it won't be difficult to use
Simon Josefssons GPL code from GNU coreutils instead, if need be.

 * The patch also comments out the setting of a number of metadata fields that
makes tracker-indexer warn about a field not described in the ontology. They
were annoying while debugging.

 * I'm not sure who is responsible for freeing the albumartdata* memory; if I
do it (similarly to the MP3 extractor), tracker-extract crashes for me.

Many thanks to Tuomas Kulve for the pointer on where to look.

I think the correct solution would be to support Ogg Vorbis out of the box
(i.e. fold equivalent changes into the tracker package proper), but this does
it for me until such a change can be made. Let me know if you want a patch for
the tracker package for that.

Ivan, any comments to comment #11?

Created an attachment (id=1918) [details]
Updated patch, uses g_base64_decode() instead of cdecode.{c,h}

This update to the patch replaces the base64 code from libb64 with simply
calling g_base64_decode(), as suggested by John Steele Scott (thanks!)

Also, I forgot to mention that if you want, you can retrieve the resulting
package from my .deb repository if you want/dare; more info on
http://koldfront.dk/debian/

Comment 14 Ivan Frade 2010-01-05 11:12:55 UTC

The patch imports almost all libtracker-common which is wrong. It should take
out of the library only the album-art part of the code, removing all the extra
unneeded dependencies.

Also, please don't include debian/changelog or .in (generated) files in the
patch. I would mark this patch as "needs work". 

Thanks for the effort, definitely album-art in the vorbis extractor is needed.

(In reply to comment #14)
> The patch imports almost all libtracker-common which is wrong. It should take
> out of the library only the album-art part of the code, removing all the extra
> unneeded dependencies.

It only includes the header-files of libtracker-common so the code can compile
and be linked against it.

I could have weeded the ones that aren't included by each other out, but I was
a little lazy there.

The correct solution would be for the tracker source package to generate a
libtracker-common-dev package which contains only the header-files, which
tracker-extractor-vorbis (and other packages linking to libtracker-common)
could build-depend on, no?

(The correcter, err :-), still solution would be for the tracker package to
simply support Ogg Vorbis - then we didn't need the ugly "ripped-functions" nor
the copied files, at all :-))

> Also, please don't include debian/changelog or .in (generated) files in the
> patch. I would mark this patch as "needs work". 

Uhm, what .in files does the patch include?

Thanks for the comments; I will try to generate a corrected patch this evening
(Danish time) and attach it.

> Thanks for the effort, definitely album-art in the vorbis extractor is needed.

Sounds great; I hope it can be integrated.

Created an attachment (id=1925) [details]
Patch updated as per comments from Ivan Frade.

This iteration of the patch:

 * Does not include any changes to debian/* except for the addition of
'tracker' to Build-Depends in debian/control.

 * Does not include libtracker-common/tracker-dbus.h (all the other headers in
librtracker-common/ is included by the code (directly or indirectly)).

Thanks again for the comments.

Created an attachment (id=2017) [details]
Updated patch works with non-embedded covers, fixes some style issues.

This attachment is based on Adam's attachment 1925 [details], with the following
modifications to tracker-extract-vorbis.c. No other files in the package were
changed.

* Don't comment out the "not described in ontology" fields. They don't have
anything to do with this bug.

* Better conform to GNOME coding style, which is what tracker seems to use. (As
an aside, I find it weird that tracker code uses a mix of tabs and spaces.
Yuck.)

* Call tracker_process_albumart() even if there is no embedded album art. This
allows a cover.jpg file in the same directory as an album to be used for art.

I still don't think it's good for this package to link against
libtracker-common.so, which seems to be an internal tracker library and
therefore not necessarily a stable API.

Could someone point me to some documentation about how to see the tracker debug
log messages? During my testing it seemed to me that the first part of
tracker_albumart_heuristic() (where it copies art from .mediaartlocal) was not
working as it should. I couldn't figure out how to debug it, but this means
that if you delete your ~/.cache/media-album-art directory, but do not delete
all your various .mediaartlocal directories, your album art will not appear in
Media Player (since that program only looks in ~/.cache/media-album-art).

(In reply to comment #17)

> * Don't comment out the "not described in ontology" fields. They don't have
> anything to do with this bug.

Yeah, I left them in because it was really annoying to have the log-files fill
up with warnings about those; just being lazy.

> I still don't think it's good for this package to link against
> libtracker-common.so, which seems to be an internal tracker library and
> therefore not necessarily a stable API.

Feel free to fix it :-)

> Could someone point me to some documentation about how to see the tracker debug
> log messages?

I don't know where the documentation is, but the log files are in
~/.local/share/tracker/tracker{-extract,-indexer,d}.log

> During my testing it seemed to me that the first part of
> tracker_albumart_heuristic() (where it copies art from .mediaartlocal) was not
> working as it should. I couldn't figure out how to debug it, but this means
> that if you delete your ~/.cache/media-album-art directory, but do not delete
> all your various .mediaartlocal directories, your album art will not appear in
> Media Player (since that program only looks in ~/.cache/media-album-art).

I read this in a thread in Talk and made a script to "clean up", so I could
test.

P.S. Working further on this patch is probably futile, since a) upstream
tracker has moved quite a bit, and b) the status of bug #176 and bug #2527 has
changed for harmattan.

(In reply to comment #19)
> P.S. Working further on this patch is probably futile, since a) upstream
> tracker has moved quite a bit, and b) the status of bug #176 and bug #2527 has
> changed for harmattan.

Yes, that's one reason why I haven't done anything about the libtracker-common
issue. On the other hand, harmattan is what, a year away? And it probably won't
be released by Nokia for the N900, so I imagine I'll be using this fix on Maemo
5 for at least a couple of years.

Tuomas, Ivan, what do you think?

Is it worth doing more work on this patch for a future release of ogg-support
or tracker-extractor-vorbis?

(In reply to comment #20)

> Yes, that's one reason why I haven't done anything about the libtracker-common
> issue. On the other hand, harmattan is what, a year away? And it probably won't
> be released by Nokia for the N900, so I imagine I'll be using this fix on Maemo
> 5 for at least a couple of years.
> 
> Tuomas, Ivan, what do you think?
> 
> Is it worth doing more work on this patch for a future release of ogg-support
> or tracker-extractor-vorbis?

I think it's worth doing. Like said, it will take quite a while before
Harmattan is here.

Comment 22 Ivan Frade 2010-01-19 12:51:59 UTC

(In reply to comment #21)
> (In reply to comment #20)
> 
> > Yes, that's one reason why I haven't done anything about the libtracker-common
> > issue. On the other hand, harmattan is what, a year away? And it probably won't
> > be released by Nokia for the N900, so I imagine I'll be using this fix on Maemo
> > 5 for at least a couple of years.
> > 
> > Tuomas, Ivan, what do you think?
> > 
> > Is it worth doing more work on this patch for a future release of ogg-support
> > or tracker-extractor-vorbis?
> 
> I think it's worth doing. Like said, it will take quite a while before
> Harmattan is here.

Fremantle is Fremantle, whatever (and whenever) harmattan does. 

Few considerations:
* Fremantle (0.6) is not correctly packaged for third party extractors
developement. I doubt we can fix it, so this vorbis extractor will always be a
dirty hack (lets minimize the dirty part, but still...)
* Harmattan (0.7) will use the same extractors code! so the work done here is
not wasted. 
* In Harmattan we will fix the -dev packaging. At that point we will just
remove the dirty hacks done for fremantle.

So, please continue the work on this patch! It is a nice feature and your are
not wasting your time.

(In reply to comment #22)

> * Harmattan (0.7) will use the same extractors code! so the work done here is
> not wasted. 

It isn't going to use a more up to date tracker?

> * In Harmattan we will fix the -dev packaging. At that point we will just
> remove the dirty hacks done for fremantle.

Cool!

> So, please continue the work on this patch! It is a nice feature and your are
> not wasting your time.

Ok, great. (The patch has already allowed me to have artwork on my N900, so for
that alone it wasn't wasted, for me, but it would of course be nice if there
were more than two N900s where Ogg artwork works :-)).

So, what do you consider to be missing from the current incarnation of the
patch?

Comment 24 Andre Klapper 2010-02-17 21:29:29 UTC

Please do not close bugs as WONTFIX.

Adam, why were you suggesting this as a WONTFIX?

(In reply to comment #25)
> Adam, why were you suggesting this as a WONTFIX?

I didn't know that one isn't supposed to until Andre Klapper's reply (I suggest
removing that option from the drop-down).

It seemed to be the option least wrong, when looking at the other choices (it
isn't fixed, it is a valid request and it only works for me because I wrote a
patch).

I'm not going to work any more on this and nobody had time to comment on the
remaining shortcomings of the attached patches, so the bug is a waste of
everybodys time and just takes up space in the non-resolved list.

Sorry for the noise; I really just intended to clean up after myself.

Just an addendum, it;d be handy if this could be updated to also support the
METADATA_BLOCK_PICTURE tag, as described here:
http://wiki.xiph.org/VorbisComment#Cover_art

Ogg Support 1.0.7 is in extras-devel and it now depends on gst-av instead of
gstreamer-vorbis/tracker-extractor-vorbis and the oggdemux has been updated has
well.

According to another bug report, embedded album art works in Flac so it might
be worth to check what's the situation now with Ogg Vorbis.

(In reply to comment #28)
> Ogg Support 1.0.7 is in extras-devel and it now depends on gst-av instead of
> gstreamer-vorbis/tracker-extractor-vorbis and the oggdemux has been updated has
> well.

> According to another bug report, embedded album art works in Flac so it might
> be worth to check what's the situation now with Ogg Vorbis.

I tried removing tracker-extractor-vorbis on my N900 and reset the database
(tracker-processes -k; remove .mediaartlocal files from MyDocs/; remove
.cache/media-art; tracker-processes -r). Rebooted the device.

No album art appeared on my 2700 Ogg Vorbis files.

Reinstalled tracker-extractor-vorbis and reset the database again. Now album
art is shown on Ogg Vorbis files again.

This is with ogg-support 1.0.7 and gstreamer0.10-av 0.5-6.

(In reply to comment #29)
> (In reply to comment #28)
> I tried removing tracker-extractor-vorbis on my N900 and reset the database
> (tracker-processes -k; remove .mediaartlocal files from MyDocs/; remove
> .cache/media-art; tracker-processes -r). Rebooted the device.
> 
> No album art appeared on my 2700 Ogg Vorbis files.
> 
> Reinstalled tracker-extractor-vorbis and reset the database again. Now album
> art is shown on Ogg Vorbis files again.
> 
> This is with ogg-support 1.0.7 and gstreamer0.10-av 0.5-6.

Can you post some of these files somewhere?

(In reply to comment #30)

> Can you post some of these files somewhere?

The packages? They are in the maemo repository.

The music? I don't have license to redistribute the music on my phone.

It is quite easy to add a cover to an Ogg Vorbis file though - EasyTAG
(http://packages.debian.org/easytag) is one tool, for instance.

An example easily found on the net could be the Ubuntu UK Podcast, say:
http://podcast.ubuntu-uk.org/download/uupc_s04e07_low.ogg

Have fun!

(In reply to comment #31)
> The music? I don't have license to redistribute the music on my phone.
> 
> It is quite easy to add a cover to an Ogg Vorbis file though - EasyTAG
> (http://packages.debian.org/easytag) is one tool, for instance.

I did that, the result works fine.

> An example easily found on the net could be the Ubuntu UK Podcast, say:
> http://podcast.ubuntu-uk.org/download/uupc_s04e07_low.ogg

This also works fine.

Perhaps you didn't wait long enough to get tracker to work on them.

(In reply to comment #32)

>> It is quite easy to add a cover to an Ogg Vorbis file though - EasyTAG
>> (http://packages.debian.org/easytag) is one tool, for instance.

> I did that, the result works fine.

>> An example easily found on the net could be the Ubuntu UK Podcast, say:
>> http://podcast.ubuntu-uk.org/download/uupc_s04e07_low.ogg

> This also works fine.

Sounds great. What versions did you use?

> Perhaps you didn't wait long enough to get tracker to work on them.

Perhaps I didn't.

(In reply to comment #33)
> (In reply to comment #32)
> 
> >> It is quite easy to add a cover to an Ogg Vorbis file though - EasyTAG
> >> (http://packages.debian.org/easytag) is one tool, for instance.
> 
> > I did that, the result works fine.
> 
> >> An example easily found on the net could be the Ubuntu UK Podcast, say:
> >> http://podcast.ubuntu-uk.org/download/uupc_s04e07_low.ogg
> 
> > This also works fine.
> 
> Sounds great. What versions did you use?

Latest and greatest:
gstreamer0.10-av 0.5.2-2
gstreamer0.10-maemo-xiph 0.5-3

(In reply to comment #34)

> > Sounds great. What versions did you use?

> Latest and greatest:
> gstreamer0.10-av 0.5.2-2
> gstreamer0.10-maemo-xiph 0.5-3

What version of the tracker* and ogg-support-packages?

Oh, it started working on my phone after I deinstalled the (original)
tracker-extractor-vorbis package.

So I guess this bug can be closed - let's party like it is 2009!

Does Application Manager remove automatically the packages that are not needed
anymore at some point? I.e. will the tracker-extractor-vorbis be autoremoved
eventually or does the user need to manually do that?

Anyway, resolving as FIXED. Thanks to Felipe again.

Created an attachment (id=3389) [details]
Extends the previous patch to support the current way of embedding coverart in
ogg files

This extends the previous patch to support the standard method of embedding
cover art in OGG files as I linked to in my previous comment, falling back to
the deprecated method the previous patch added.

Note: It will only extract artwork identified as front cover art.

I've tested this quite a bit and it works fine for me.

(In reply to comment #35)
> (In reply to comment #34)
> > > Sounds great. What versions did you use?
> 
> > Latest and greatest:
> > gstreamer0.10-av 0.5.2-2
> > gstreamer0.10-maemo-xiph 0.5-3
> 
> What version of the tracker* and ogg-support-packages?

The tracker-extractor packages are not needed any more.

...well, I have the latest gstreamer packages installed, and my cover art
wasn't being extracted. I couldn't for the life of me work out where the old
extraction was happening in the gstreamer code, so I just added it to this
package as a quick fix...and now I have working cover art extraction.

All my files have their cover art embedded correctly as METADATA_BLOCK_PICTURE
comments, as mentioned previously.

(In reply to comment #39)
> ...well, I have the latest gstreamer packages installed, and my cover art
> wasn't being extracted. I couldn't for the life of me work out where the old
> extraction was happening in the gstreamer code, so I just added it to this
> package as a quick fix...and now I have working cover art extraction.
> 
> All my files have their cover art embedded correctly as METADATA_BLOCK_PICTURE
> comments, as mentioned previously.

Can you post one of them somewhere? Also, I would remove all the packages and
start afresh. Or you could also install first the old one, and then upgrade to
make sure normal users would not notice any problems.

Created an attachment (id=3390) [details]
30 seconds of silence with cover artwork

I can't provide any of my *actual* audio files due to copyright issues, but I
added all my Ogg tags using the latest version of the software mp3tag
(http://www.mp3tag.de/en/). What I can do however is generate an audio file,
and I've attached a 30 silent ogg with some artwork using
METADATA_BLOCK_PICTURE - the image is a PD piece from wikipedia.

As I mentioned previously, Xiph documented the spec very clearly:
http://wiki.xiph.org/VorbisComment#Cover_art

...and linked from that document they provide a couple of sample files:
http://www.audioranger.com/coverart_mk.ogg (image set as another image, aka can
be ignored)
http://www.audioranger.com/coverart_im.ogg (image set as front cover, aka the
one we want)

(In reply to comment #41)
> Created an attachment (id=3390) [details] [details]
> 30 seconds of silence with cover artwork
> 
> I can't provide any of my *actual* audio files due to copyright issues, but I
> added all my Ogg tags using the latest version of the software mp3tag
> (http://www.mp3tag.de/en/). What I can do however is generate an audio file,
> and I've attached a 30 silent ogg with some artwork using
> METADATA_BLOCK_PICTURE - the image is a PD piece from wikipedia.
> 
> As I mentioned previously, Xiph documented the spec very clearly:
> http://wiki.xiph.org/VorbisComment#Cover_art

Ok, found the problem. It turns out the problem is in gst-plugins-good, in
libgsttab; the Fremantle version doesn't check for METADATA_BLOCK_PICTURE. I
have made a hack to copy the latest version in gst-maemo-xiph. Now it works
fine :)

Fixed in gst-maemo-xiph 0.5-4.

Now I wonder if ogg-support should trigger a manual regeneration of the
vorbis/flac tags in order to compensate for this regression. There must be a
way through tracker-extractor distutils I guess.

(In reply to comment #42)

> Fixed in gst-maemo-xiph 0.5-4.

I missed the mail notifications about this bug report and now the ogg-support
1.1.1 in testing doesn't depend on 0.5-4 of this package :/

I'm not sure how the Maemo repository magic work, does it take the newest
version from Extras-devel/testing to Extras even if nothing requires that new
package?

(In reply to comment #43)
> (In reply to comment #42)
> 
> > Fixed in gst-maemo-xiph 0.5-4.
> 
> I missed the mail notifications about this bug report and now the ogg-support
> 1.1.1 in testing doesn't depend on 0.5-4 of this package :/

Crap :/

> I'm not sure how the Maemo repository magic work, does it take the newest
> version from Extras-devel/testing to Extras even if nothing requires that new
> package?

Nope it doesn't. Or at least not the last time I tested that.