Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIN32] play wma audio with native windows codec #757

Closed
wants to merge 12 commits into from
Closed

[WIN32] play wma audio with native windows codec #757

wants to merge 12 commits into from

Conversation

wsoltys
Copy link

@wsoltys wsoltys commented Mar 8, 2012

this pull request implements a paplayer codec to play wma audio instead with our dvdplayer (ffmpeg) with the native windows codec. This adds support for wma lossless which doesn't work in the current implementation.

I did this primary for learning reasons as a native wmv codec for dvdplayer would make more sense but I guess it could be a nice addition.

One thing I don't understand is that regardless which file I present to paplayer (16 or 24 bit, 44100Hz vs 48000Hz) I need to supply always 16 bit and 44100Hz in the parameters. Either the isyncreader interface just delivers only this format or I'm doing something wrong for paplayer.

@a11599
Copy link

a11599 commented Mar 8, 2012

WMA lossless was added in ffmpeg 0.9?
https://github.com/FFmpeg/FFmpeg/blob/release/0.9/libavcodec/wmalosslessdec.c

@wsoltys
Copy link
Author

wsoltys commented Mar 8, 2012

As said the main purpose was for learning reasons as dvdplayer seems to difficult for me ;)
The second could be why use some reengineered stuff if you can get the original. We do the same with WINFileSMB.
Agreed that wmv would make more sense since we had never proper wmv playback (at least what I get from the user reports since I don't use wma nor wmv).

@a11599
Copy link

a11599 commented Mar 8, 2012

I am not objectioning. :) My comment was just meant as a note, in case someone is interested. I am happy with flac, but I realize there is demand for lossless wma as well. :)

@wsoltys
Copy link
Author

wsoltys commented Mar 8, 2012

Sure for that we have the pull requests and thanks for the info ;)
I still can live with 128kbits mp3 :P

@DDDamian
Copy link
Contributor

DDDamian commented Mar 8, 2012

Haven't tested but all in favor of adding capabilities. Can I suggest this be looked at in the context of the AE branch and compatibility with it?

@wsoltys
Copy link
Author

wsoltys commented Mar 9, 2012

I never had a look at AE but IIRC we just need to deliver float samples rather than int for the current implementation.

@DDDamian
Copy link
Contributor

DDDamian commented Mar 9, 2012

That's correct, or ideal at any rate. Either is acceptable but they'll be converted to float in AE before further processing, so all good. We can easily add the calls to your decoder from AE. Nice addition :)

@CrystalP
Copy link
Contributor

CrystalP commented Mar 9, 2012

I'm all for additional support, however it's delicate so close to release. Because this is new code and may not be safeguarded enough for the weird files out there, it might have a better chance if the codec only accepted wma lossless, so that lossy wma support remains via ffmpeg and can't have regressions.

a11599 is right that when we bump ffmpeg after Eden release, we'll gain wma lossless support and it will be more difficult to justify a native codec.

From a technical POV, wmvcore.dll is not installed by default on all machines - I'm thinking about the European N versions, without media stuff builtin. So static linking won't do and the user should be told to download the media feature pack.

Not having touched these parts of XBMC, take this with a grain of salt. Is it a good idea for XBMCistream to instantiate a CFile? Is that the correct level of abstraction to transparently get network streams, network shares & all?
Or should XBMCistream be class that takes a standard XBMC object and dresses it up as an IStream?

@wsoltys
Copy link
Author

wsoltys commented Mar 9, 2012

This isn't meant for eden ofc. As said why use some "hacked" stuff if you can have the original? After all this was for learning purpose and if nobody wants it we can discard it.
I don't think that the N version is used widely so no really good point for me. wmvcore is delayed load and only needed when playing wma files. If not found CanInit returns false.
Don't get your last question. isyncreader can only handle local files and an istream object. To use our vfs I had to implement cfile in istream which is working fine. to change istream was also mentioned in all infos I found on the net.

@wsoltys
Copy link
Author

wsoltys commented Mar 9, 2012

On a side note what is the difference between using windows native codecs and linux system or external libs. I would suppose we have a lot more hacks to support all the different library versions for the different linux distributions.

@CrystalP
Copy link
Contributor

Isn't the N version mandatory in Europe? And probably most people install the media extensions anyway.

Don't read me wrong, I'm for this but if the wma lossless in ffmpeg is solid, the linuxers may object to an additional dependency for little gain.
It's not exactly the same as linux system libs, as they're still the same library, only with a potentially different version. There may be an equivalent with OSX-provided closed-source libraries for stuff that one of XBMC's dependencies handles.

I'm very unfamiliar with paplayer, if it can only play local files I understand the XBMCistream. If paplayer can play music files from all kinds of urls (smb, network stream, upnp, ...), does the CFile handle that abstraction or does it take other XBMC classes? In which case XBMCistream may have to be reworked.

@wsoltys
Copy link
Author

wsoltys commented Mar 10, 2012

paplayer lets the codecs handle the disc access and several our codecs uses Cfile which handles all kinds of urls fine. Other codecs accesses the disc with their third party lib which is wrapped to use our CFile. Cfile is the access to our filefactory and used at several places.
the XBMCistream was taken from an example out of the windows sdk. I just replaced the createfile statements for local disc access by our Cfile.

@wsoltys
Copy link
Author

wsoltys commented Mar 10, 2012

btw paplayer used dvdplayer to play wma files. You still get the same behavior if you choose "play with dvdplayer" from the context menu.

@jmarshallnz
Copy link
Contributor

Nice work - always good to delve into a new area of XBMC :)

It seems to me that this is best evaluated once ffmpeg has been bumped: If ffmpeg supplies what we need, then we should use it (as it unifies things across platforms) unless there's a very good reason against it (eg ffmpeg doesn't work for some files). If not, I don't see a problem of adding this as long as it falls back gracefully on failure (eg missing dlls or corrupt files) to using ffmpeg.

@wsoltys
Copy link
Author

wsoltys commented Mar 16, 2012

haha, jmarshall you always have a nice way of saying: we don't want this ;)
You know that I always kept the platform idea and tried to think for all platforms but in this particular area I disagree. I think we should use what the platforms offer if it can be implemented without harming the others. But I can live with the decision knowing that I've learned some other parts of XBMC.
I assume the same statements would be valid if I would dive into dvdplayer for implementing wmv? Not that I did this yet as dvdplayer is a beast for me.

@jmarshallnz
Copy link
Contributor

Heh - it's not a complete no - just might not be an advantage if (and that may be a big if) ffmpeg handles it. If ffmpeg's support isn't as good, then no problem at all.

@wsoltys
Copy link
Author

wsoltys commented Mar 16, 2012

IIRC we had more problems seeking and playing wmv than with wma. But as said dvdplayer is a beast for me and wanted to start with a small thing :)

@wsoltys
Copy link
Author

wsoltys commented Apr 5, 2012

bump ;)
Even with our new ffmpeg XBMC won't playback any of my lossless files. If anyone want to test/fix it I've uploaded samples on teamftp in my /wiso folder.
This pr will get a fallback to dvdplayer if the dll wasn't found and then all should be good (assumed it still works).

@DDDamian
Copy link
Contributor

DDDamian commented Apr 8, 2012

Finally got a chance to pull this, after applying the ffmpeg bump. Looks like ffmpeg is still trying to decode, not your patch. Same using DVDPlayer or PAPlayer. Used the 24bit wma sample from FTP.

Didn't dig any deeper, but here's a log: http://pastebin.com/TTWXz2Rn

@wsoltys
Copy link
Author

wsoltys commented Apr 8, 2012

I need to rebase it with latest master. Probably you missed the change in CodecFactory.cpp but I'll look at it to include more log infos and the fall back to dvdplayer.
Just for the others as info as the logfile tells ffmpeg detects it as lossless samples but denies to play it since experimental codecs aren't enabled.

@wsoltys
Copy link
Author

wsoltys commented Apr 8, 2012

The rebase was a mess due to massive changes to the vs project files. Also the wmacodec wasn't returned in CreateCodecDemux. Dunno why it worked before but I think it has to do with the changes in between.
The fall back will follow later. the 24bit file doesn't work atm.
Note: paplayer plays all wma files in 44khz 16bit. Dunno if this is due to paplayer or that probably the syncreader just returns that when we request uncompressed streams.

@wsoltys
Copy link
Author

wsoltys commented Apr 10, 2012

updated with a fallback to dvdplayer when wmvcore.dll isn't available.
If now one objects I'll merge it within the next merge window.

m_Bitrate = inputFormat->nAvgBytesPerSec/8;

if(!m_pcmBuffer.Create(BUFFER_SIZE))
return false;

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@bb10
Copy link
Contributor

bb10 commented May 30, 2012

ffmpeg 0.11 changelog shows the WMA Lossless decoder got some fixes.

@wsoltys
Copy link
Author

wsoltys commented May 30, 2012

and if we wait another few months the wma lossless codec might be not experimental anymore and even activated in ffmpeg (which it isn't yet)

@wsoltys wsoltys closed this May 30, 2012
@wsoltys
Copy link
Author

wsoltys commented May 30, 2012

closed as it would need to be rebased on ae anyway

tru pushed a commit to plexinc/plex-home-theater-public that referenced this pull request Nov 12, 2013
HolgerW1 pushed a commit to HolgerW1/xbmc that referenced this pull request Sep 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature non-breaking change which adds functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants