Open freaktechnik opened 10 years ago
I agree that we should look towards refactoring this. Since we're handing over responsibility for playback to gstreamer, we really should rely on gstreamer to tell us what it can handle. Refactoring everything, however, would be a ton of work, and we should probably look towards targeting the 1.0 version if we are rebuilding these components.
@thebecwar I updated the mediacore to support gstreamer 1.0+, but it isn't in the main branch, since it's not backwards compatible AFAIK. Here's the branch: nightingale-hacking/gstreamer-1.0. I'm sure this still needs to be refactored, as I only made minimal changes to avoid breaking things moving between API levels. That branch builds and functions correctly though. Maybe we can make additional changes in that branch and merge it into the trunk as a large mediacore update?
@johnmurrayvi My vote would be to make these changes to the new xulr branch instead (or a branch from it). That way we have less fires to put out when it comes time to migrate our efforts to that branch. Plus, any major changes to the media core would need to compile and work with that new version anyway.
@thebecwar the goal for 2.0 is to use XULR 2+, GStreamer 1.0 and native SQLite, as documented on the roadmap: http://wiki.getnightingale.com/doku.php?id=roadmap In order to be able to shift to gstreamer 1.0 we also need to have the option to bundle it on linuxe, because of long term support releases (Debian/Ubuntu), as we have to do for taglib.
@thebecwar Here's an initial cherry-picking of the GStreamer 1.0 stuff into the new xul branch. Once I make sure nothing extra got changed, added, or removed, I'll probably just merge it into the master-xul-9.0.1 branch.
Re-reading this... What did you think we should do? Change the audio extension list to a pref? It might be possible to get gstreamer to tell us what the file is? I'm not sure to be honest. I wanted to poke the issue see what the main idea was before it drifts down the issues list too far
I think we should generate the video extension list dynamically too. Even though this gives possible playback issues it's better than restricting the extensions to a preset imo. On the other hand with a preference we can ensure that you can play what you can add to your library.
What about the blacklist? There are several video extensions blacklisted that seem to be playable. Here's the output I get (with bundled gstreamer-1.0) from the logging I have in that commit: http://pastebin.com/raw.php?i=gB7HuJ9w
@johnmurray There should be no video extension in the blacklist, as the blacklist was not used for the video extensions before. If there are any in the blacklist, then they probably served the purpose of not adding a video extension as an audio extension. From your output mainly images and misc. filetypes are blacklisted.
I tweaked the output a bit. Here's a list of the blacklisted extensions which factories could provide: http://pastebin.com/raw.php?i=1c8vCSfd
The (most) relevant ones are
factory: video/x-cdxa
in the blacklist (video): 'dat'
factory: video/x-vcd
in the blacklist (video): 'dat'
and
factory: audio/x-musepack
in the blacklist (audio): 'mpc'
in the blacklist (audio): 'mpp'
in the blacklist (audio): 'mp+'
factory: audio/x-ircam
in the blacklist (audio): 'sf'
factory: audio/x-mod
in the blacklist (audio): 'far'
in the blacklist (audio): 'stm'
factory: audio/x-wavpack-correction
in the blacklist (audio): 'wvc'
Well, I can see why we wouldn't import .dat files, as these are quite generic and can be anything. We would need to do mime sniffing in order to decide if we can actually play them.
I never heard of the audio extensions that are blacklisted, we would have to research what exactly they do (could be filters etc., everything that you can apply to a gstreamer pipeline basically).
That's true. I don't think .dat are as important to handle. I haven't heard of them either, but just wanted to check.
After I put in the dynamic generation of the video extensions, these are the ones that got registered:
VIDEO:
263, 264, asf, avi, dif, dv,
flc, fli, flv, h263, h264, ivf,
m4v, mj2, mk3d, mka, mkv, mng,
mov, mpe, mpeg, mpeg, mpg, mpg,
mpv, mts, mve, nuv, pva, ts,
viv, webm, wm, wma, wmv, x264
Should wma be blacklisted from the video extensions?
So I've looked up the blacklisted extensions:
wavpack-correction is a file to make a lossy file lossless, I can see why we wouldn't import that.
The hybrid mode provides all the advantages of lossless compression with an additional bonus. Instead of creating a single file, this mode creates both a relatively small, high-quality lossy file that can be used all by itself, and a "correction" file that (when combined with the lossy file) provides full lossless restoration. For some users this means never having to choose between lossless and lossy compression!
Further I agree that we should blacklist wma from the video formats, as its extension is windows media audio, so clearly no video. But I guess it's listed under video too as it uses the same container as wmv?
Note to self: need to come back and make sure I got all of this into master-xul-9.0.1.
I see https://github.com/nightingale-media-player/nightingale-hacking/commit/f5ec124164a391667babe2330bb8aaa4aafb3078 and https://github.com/nightingale-media-player/nightingale-hacking/commit/0fc3123d226dfcc8f5585f19aacf1ce0e047d89f so I think I may only need to blacklist the wma extension from video as mentioned above?
Currently our mediacore determines which audio and video formats it suports in two idfferent ways: For audioformats there is a blacklist and an additional list of extensions, that need to be in the list. Those are then matched with GStreamers capabilities. Video is a strict whitelist, aka hardcoded, even if we could play more files. Further, video import is not handled through the MediacoreFactory, but with the pref songbird.mediacore.gstreamer.videoExtensions. There is a code from Songbird in the concerning method: components/mediacore/gstreamer/src/sbGStreamerMediacoreFactory.cpp#L171:
@mook seems to hate the blacklist for audio extensions, according to a comment in there...