Bug 303253 - File browser of amarok does not display m4a files
Summary: File browser of amarok does not display m4a files
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Unclassified
Component: File Browser (show other bugs)
Version: 2.5.0
Platform: Fedora RPMs Linux
: NOR normal (vote)
Target Milestone: 2.6
Assignee: Amarok Developers
URL:
Keywords:
: 268896 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-07-09 19:30 UTC by Andrea
Modified: 2012-07-24 10:16 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.6


Attachments
Add both aliases for audio/x-m4a and audio/mp4 (712 bytes, patch)
2012-07-14 20:54 UTC, Andrea
Details
0001-FileBrowser-sequel-to-display-all-audio-video-files-.patch (13.21 KB, patch)
2012-07-23 10:20 UTC, Matěj Laitl
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrea 2012-07-09 19:30:20 UTC
+++ This bug was initially created as a clone of Bug #241660 +++

Version:           2.5.0 (using KDE 4.8.4) 
OS:                Fedora 17

If you use the file browser of amarok to go into a directory which contains m4a files, file browser display nothing.
But if I drag drop this folder to the playlist, all m4a files are added and play properly (but the length is not updated properly, it is always 00:00, contrary to other file types).
So I think I have all codecs installed.

Reproducible: Always

Steps to Reproduce:
1/Open amarok
2/Go to Files item
3/Browse to directory with m4a files
4/Take note that browser display nothing

Actual Results:  
Amarok file browser does not display m4a files

Expected Results:  
Amarok file browser should display m4a files
Comment 1 Myriam Schweingruber 2012-07-10 20:07:48 UTC
Sounds like a MIME type problem. Is it available in your MIME types (Systemsettings -> Fille associations -> Audio) ?
Comment 2 Andrea 2012-07-11 06:18:21 UTC
I have a group called mp4 which associates

*.aac
*.f4a
*.m4a

to Amarok with description MPEG-4 audio.
In Dolphin they show as such and a double click there starts amarok which plays them properly.

It is only Amarok browser which misses them.
Comment 3 Andrea 2012-07-11 20:09:48 UTC
I've done some more research and read a bit of the code.

FileBrowser calls EngineController::supportedMimeTypes() which returns for sure a list containing audio/x-m4a (I've recompiled this function separately and run it)

in /usr/share/mime/audio there is a file for this mime type

andrea@thinkpad:/usr/share/mime/audio$ grep m4a *
mp4.xml:  <alias type="audio/x-m4a"/>
mp4.xml:  <glob pattern="*.m4a"/>

I guess the next step is the debugger....
Comment 4 Andrea 2012-07-11 20:38:47 UTC
I've tried to rename (not convert) an .m4a file to .mp3 and it is displayed.
On top of that, once it goes into the playlist the correct length is displayed.

when the m4a goes to the playlist, the length is 00:00.
Comment 5 Myriam Schweingruber 2012-07-12 07:00:44 UTC
Which phonon backend do you use? The lack of lenght displayed depends on the phonon backend, and in your case, I suspect gstreamer to be the culprit.

You shouldn't rename mp4 files to mp3, those are totally different containers. You can rename a m4a to mp4 eventually, but not mp3
Comment 6 Andrea 2012-07-12 07:54:39 UTC
I am using gstreamer backend. I can try xine backend. 
Is the backend used in the file browser as well?

I renamed it, just to rule out the fact that maybe it is a bad file and if amarok looked inside, finding a problem, it skipped it from the file browser. But since it works as .mp3, I suspect as you said the mime type to be off. But again, Dolphin identifies it properly....

Maybe the 00:00 length (happening in the right panel) and the missing file in the browser are not related. I dont care about the length too much, but since my collection is mostly .m4a, it is very annoying..

I tried to debug, but the fedora package is optimised and gdb jumps around and gets lost.
I need to recompile it. Which will take time.

Do you have any idea where to debug this issue? I can extract the part of the code filtering the files and create a little test app.
Comment 7 Myriam Schweingruber 2012-07-12 17:05:24 UTC
Please do not use the xine backend, it is deprecated and unmaintained since more than a year. Please use the phonon-backend-vlc instead if you want to try another one. Don't forget to restart KDE to make the backend change effective.

Do you have the taglib 1.7 package with the mp4 and asf flags activated? That could be another reason for these files not showing up. See also http://taglib.github.com/
Comment 8 Andrea 2012-07-12 20:37:13 UTC
Found!

It is to do with mime aliases.

I've added some debug print in

bool
MimeTypeFilterProxyModel::filterAcceptsRow( int source_row, const QModelIndex& source_parent ) const

and this is what I see

ElTren.flv: video/x-flv = 1
ARamZamZam.ogg: audio/x-vorbis+ogg = 1
LaDenseDesCanards.flv: video/x-flv = 1
ElTren.m4a: audio/mp4 = 0
ARamZamZam.m4a: audio/mp4 = 0
ARamZamZam.flv: video/x-flv = 1
ElTren.ogg: audio/x-vorbis+ogg = 1

so for each file I print the name, mime and if it accepted or not by the filter.
amarok expects audio/x-mp4, but the mime is audio/mp4.

If I read the function 

QStringList
EngineController::supportedMimeTypes();

I see already an attempt to deal with something similar.

    if( mimeTable.contains( "audio/x-flac" ) && !mimeTable.contains( "audio/flac" ) )
        mimeTable << "audio/flac";

maybe we could extend it to mp4?

the audio/mp4 come from

/usr/share/aliases

which contains

audio/x-m4a audio/mp4

I am not an expert, but the check should take into account the aliases, either at KDE level or amarok parsing this file.

What do you think?
Comment 9 Andrea 2012-07-12 20:39:18 UTC
of course there was a typo in my comment.

the item's mime type is audio/mp4, but amarok expects audio/x-m4a.
Comment 10 Andrea 2012-07-14 20:54:50 UTC
Created attachment 72524 [details]
Add both aliases for audio/x-m4a and audio/mp4

It seems the official mime type is audio/mp4, which is the same as the type returned for the file.

But Phonon (at least in my case) says it supports audio/x-m4a which is an alias.
There is already a similar check to support both audio/x-flac and audio/flac. This patch extends the same logic to mp4.
Comment 11 Myriam Schweingruber 2012-07-16 22:39:03 UTC
Strange as I can't reproduce this problem at all with the phonon-backend-vlc 0.5.0, and normally this should not be solved in Amarok anyway but in the phonon backend and by a correct mime type list in the system.  Not sure the patch belongs here...

Please try the latest phonon-backend-gstreamer 4.6.1 or the phonon-backend-vlc 0.5.0 first, as suggested in my last comment.
Comment 12 Harald Sitter 2012-07-17 08:18:36 UTC
Bug 291438
Comment 13 Harald Sitter 2012-07-17 08:21:41 UTC
Oh also... What phonon may claim to support and what phonon can actually support are two entirely different things these days as VLC can play just about everything unless linux distros break it and GST can install support for just about everything on the fly. So as a matter of fact simply allowing all mimetypes of audio/* would probably be the best thing to do.
Comment 14 Andrea 2012-07-17 12:42:00 UTC
I read a bit more the code and I think it would work in phonon-backend-vlc since, there is a hardcoded list of mime types supported by vlc which includes:

audio/mp4 *and* audio/x-m4a

(lines  163 to 249 of https://projects.kde.org/projects/kdesupport/phonon/phonon-vlc/repository/revisions/master/entry/src/backend.cpp)

the same thing in gstreamer is done via plugins

(lines 218 of
https://projects.kde.org/projects/kdesupport/phonon/phonon-gstreamer/repository/revisions/master/entry/gstreamer/backend.cpp)
plus some extra logic around lines 273)

Then, in gstreamer-plugins-good the decoder for mp4,
http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/isomp4/qtdemux.c

at line 367 they mention audio/x-m4a and not the official type audio/mp4.

I have not run all that, so I might be wrong.
I believe the patch belongs to phonon-gstreamer or gstreamer-plugins.

But, as Harald mentioned, maybe it is simlper to show all audio files even if they dont play. I feel it is better to get an error (or no sound) for an unsupported type, rather than hiding from the list (even if it does actually play).
Comment 15 Andrea 2012-07-17 13:50:18 UTC
But looking again at what happens for flac:

It is amarok that adds the 2 aliases since both vlc and gstreamer backends claim to support audio/x-flac, while /usr/share/mime/aliases lists audio/flac as the official name (which is what KDE returns).

Let me know if you believe I should open a bug in gstreamer or phonon-backend-gstreamer.
Comment 16 Matěj Laitl 2012-07-19 13:26:14 UTC
Andrea, patch from https://git.reviewboard.kde.org/r/105524/ should fix this, please test it.
Comment 17 Matěj Laitl 2012-07-19 14:20:19 UTC
Git commit 712873769dda415c6a21737428bd1a5305018271 by Matěj Laitl.
Committed on 13/07/2012 at 13:05.
Pushed by laitl into branch 'master'.

EngineController: ditch canDecode()

MetaFile::Track::isTrack() is partial replacement. Existing 3 calls to
canDecode() were in fact all related to MetaFile classes, so move the
method there, simplify it not to query phonon at all (and document it
can return false positives).

As a consequence, we show all audio and video files in file browser and
in other places, even if they wouldn't be playable by the current
phonon back-end. This is arguably a cleaner approach and at least lets
users discover where the error is.

Works quite well for me and prevents failures in many tests. This
change is propelled by Bart's and Ralf's legitimate comments on
http://git.reviewboard.kde.org/r/105524/
FIXED-IN: 2.6

M  +1    -0    ChangeLog
M  +0    -42   src/EngineController.cpp
M  +0    -5    src/EngineController.h
M  +2    -1    src/browsers/filebrowser/FileBrowser.cpp
M  +3    -1    src/browsers/filebrowser/FileView.cpp
M  +5    -8    src/core-impl/collections/support/CollectionManager.cpp
M  +33   -6    src/core-impl/meta/file/File.cpp
M  +9    -0    src/core-impl/meta/file/File.h

http://commits.kde.org/amarok/712873769dda415c6a21737428bd1a5305018271
Comment 18 Andrea 2012-07-20 19:42:59 UTC
Hi guys,

this does not seem to change anything.
The files still dont show up in the browser.
Looking at the files patched it seems they refer to the playlist/collection.

The browser does not seem to be affected.
For me the problem is still in the interaction between

MimeTypeFilterProxyModel::filterAcceptsRow( int source_row, const QModelIndex& source_parent ) const

and

EngineController::supportedMimeTypes()

Am I misreading your comment?
I don't think canDecode is ever called, the files are filtered beforehand (by MimeTypeFilterProxyModel).
I've always been able to see m4a files in the playlist if

1) dragging a folder from amarok's browser
2) double clicking in dolphin

but when I dig into the folder in amarok browser, m4a files don't show up
Comment 19 Matěj Laitl 2012-07-23 09:56:46 UTC
(In reply to comment #18)
> Hi guys,
> 
> this does not seem to change anything.
> The files still dont show up in the browser.
> Looking at the files patched it seems they refer to the playlist/collection.
> 
> The browser does not seem to be affected.
> For me the problem is still in the interaction between
> 
> MimeTypeFilterProxyModel::filterAcceptsRow( int source_row, const
> QModelIndex& source_parent ) const
> 
> and
> 
> EngineController::supportedMimeTypes()
> 
> Am I misreading your comment?
> I don't think canDecode is ever called, the files are filtered beforehand
> (by MimeTypeFilterProxyModel).
> I've always been able to see m4a files in the playlist if
> 
> 1) dragging a folder from amarok's browser
> 2) double clicking in dolphin
> 
> but when I dig into the folder in amarok browser, m4a files don't show up

Oh sorry, you're right. I removed most cases where shown files are dependent on what Phonon currently uses and most of them called canDecode(). But now I see I didn't remove the one that calls supportedMimeTypes() and which is essentially this bug.
Comment 20 Matěj Laitl 2012-07-23 10:20:21 UTC
Created attachment 72696 [details]
0001-FileBrowser-sequel-to-display-all-audio-video-files-.patch
Comment 21 Matěj Laitl 2012-07-23 10:23:00 UTC
Andrea,
please test above patch and report whether it fixes the problem for you. The file browser should now display all audio and video files + directories, but nothing more. Your patch should still be applied to Amarok master, but only after the 0001-FileBrowser-sequel-to-display-all-audio-video-files-.patch to ensure the latter works.
Comment 22 Andrea 2012-07-23 20:07:41 UTC
Much better now.
I can see them in the browser.

My patch look superfluous now.

Thanks
Comment 23 Matěj Laitl 2012-07-24 09:14:39 UTC
*** Bug 268896 has been marked as a duplicate of this bug. ***
Comment 24 Matěj Laitl 2012-07-24 09:48:51 UTC
(In reply to comment #22)
> Much better now.
> I can see them in the browser.

Great.

> My patch look superfluous now.

It isn't, we export supportedMimeTypes() for example in MPRIS2 and we should really incpude audio/mp4 if we have audio/x-mp4.
Comment 25 Matěj Laitl 2012-07-24 10:00:45 UTC
Git commit e80db0e2a349708465491635fdfe157f112b7ae2 by Matěj Laitl.
Committed on 23/07/2012 at 12:16.
Pushed by laitl into branch 'master'.

FileBrowser: sequel to display all audio/video files, don't use Phonon

This is the real fix to the bug 303253, previous commit 712873769dda415
that claimed it fixed that fixed something else.
FIXED-IN: 2.6

M  +1    -1    src/CMakeLists.txt
R  +13   -8    src/browsers/filebrowser/DirPlaylistTrackFilterProxyModel.cpp [from: src/browsers/filebrowser/MimeTypeFilterProxyModel.cpp - 080% similarity]
R  +11   -20   src/browsers/filebrowser/DirPlaylistTrackFilterProxyModel.h [from: src/browsers/filebrowser/MimeTypeFilterProxyModel.h - 065% similarity]
M  +2    -3    src/browsers/filebrowser/FileBrowser.cpp
M  +2    -2    src/browsers/filebrowser/FileBrowser_p.h

http://commits.kde.org/amarok/e80db0e2a349708465491635fdfe157f112b7ae2
Comment 26 Matěj Laitl 2012-07-24 10:16:25 UTC
Git commit 22460625656fb9cc092edfe9aa0fd1609088c615 by Matěj Laitl.
Committed on 24/07/2012 at 12:08.
Pushed by laitl into branch 'master'.

EngineController: if supportedMimeTypes contains x-m4a, add mp4 too

So that we announce correct mimetypes in MPRIS2. Patch from Andrea on
CCed bug.

M  +4    -0    src/EngineController.cpp

http://commits.kde.org/amarok/22460625656fb9cc092edfe9aa0fd1609088c615