Mixxx

Merge lp://qastaging/~mixxxdevelopers/mixxx/features_m4a_win7_plugin into lp://qastaging/mixxx/1.10

  1. features_m4a_win7_plugin
  2. Merge into release-1.10.x
Proposed by William Good
Status: Merged
Merged at revision: 2881
Proposed branch: lp://qastaging/~mixxxdevelopers/mixxx/features_m4a_win7_plugin
Merge into: lp://qastaging/mixxx/1.10
Prerequisite: lp://qastaging/~mixxxdevelopers/mixxx/fixes-plugins-mempassing
Diff against target: 800 lines (+725/-2)
7 files modified
mixxx/SConstruct (+2/-0)
mixxx/build/depends.py (+1/-2)
mixxx/build/features.py (+32/-0)
mixxx/plugins/SConscript (+6/-0)
mixxx/plugins/soundsourcemediafoundation/SConscript (+22/-0)
mixxx/plugins/soundsourcemediafoundation/soundsourcemediafoundation.cpp (+551/-0)
mixxx/plugins/soundsourcemediafoundation/soundsourcemediafoundation.h (+111/-0)
To merge this branch: bzr merge lp://qastaging/~mixxxdevelopers/mixxx/features_m4a_win7_plugin
Reviewer Review Type Date Requested Status
RJ Skerry-Ryan Approve
William Good Needs Fixing
Review via email: mp+78777@code.qastaging.launchpad.net

Description of the change

Replacing https://code.launchpad.net/~mixxxdevelopers/mixxx/features_m4a_win7/+merge/67111 because the only way to have this functionality built in to Mixxx would be to either a) distribute multiple executables which is a PITA or b) load the necessary COM interfaces dynamically which would be somewhere between a huge PITA and impossible.

I was initially against the plugin thing but with the fixes in lp:~mixxxdevelopers/mixxx/fixes-plugins-mempassing it should be stable for production.

Anyway, so this way we can package a plugin DLL with the normal installers, and if the requisite interfaces/libraries aren't available at runtime, the plugin DLL will simply fail to load and nothing will crash and burn. This plugin is 110% legal to distribute because the AAC patent applies to distributing decoders, not code calling someone else's decoder (like how our binaries with CoreAudio-based m4a decoding are legal to distribute, except on Windows).

Proposed for 1.10 because it works, and m4a decoding was targeted for like 2 major versions ago so it'd be way cool to have in 1.10. It does feel a bit slower than most of our decoders (to me), but that's Media Foundation's fault, as far as I can tell. CachingReader really mitigates the issue for all intents and purposes, though.

To post a comment you must log in.
Revision history for this message
William Good (bkgood) wrote :

I screwed up a merge somewhere on this, I'll sort it out later today.

review: Needs Fixing
lp://qastaging/~mixxxdevelopers/mixxx/features_m4a_win7_plugin updated
2850. By William Good

Merged from lp:~mixxxdevelopers/mixxx/fixes-plugins-mempassing. Updated SoundSourceMediaFoundation to new plugin API.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Hey Bill,

What's the status of the branch? Do you think it's stable enough to be in 1.10?

If so, I'd like to figure out how to get it packaged for the beta so we can work out the kinks. It only works for Vista onwards because of the WMF dependency right? We may need Sean's help updating the NSIS to only install the plugin on >=Vista.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

Also, just to make sure I'm right here -- is the mempassing branch now fully merged into this one or should I also review that branch?

Revision history for this message
William Good (bkgood) wrote :

Yeah, I think it's good to go. We really could be sloppy with it and
install it anywhere as if Mixxx can't load the DLL (due to unresolved
symbols) it'll happily continue on. It's also possible that a user
could install Mixxx, and then install the necessary update that gives
Vista the AAC decoder, and they ought not have to reinstall Mixxx if
possible.

On Wed, Oct 19, 2011 at 11:57 AM, RJ Ryan <email address hidden> wrote:
> Hey Bill,
>
> What's the status of the branch? Do you think it's stable enough to be in 1.10?
>
> If so, I'd like to figure out how to get it packaged for the beta so we can work out the kinks. It only works for Vista onwards because of the WMF dependency right? We may need Sean's help updating the NSIS to only install the plugin on >=Vista.
> --
> https://code.launchpad.net/~mixxxdevelopers/mixxx/features_m4a_win7_plugin/+merge/78777
> You proposed lp:~mixxxdevelopers/mixxx/features_m4a_win7_plugin for merging.
>

Revision history for this message
William Good (bkgood) wrote :

mempassing hasn't been merged and needs review, it needs to be merged
before this branch is.

On Wed, Oct 19, 2011 at 11:58 AM, RJ Ryan <email address hidden> wrote:
> Also, just to make sure I'm right here -- is the mempassing branch now fully merged into this one or should I also review that branch?
> --
> https://code.launchpad.net/~mixxxdevelopers/mixxx/features_m4a_win7_plugin/+merge/78777
> You proposed lp:~mixxxdevelopers/mixxx/features_m4a_win7_plugin for merging.
>

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

In seek(): If the original setCurrentPosition fails, in other SoundSources I think we generally return the current position (i.e. the current location of the reader) to indicate that the seek failed.

Also, could you define a "const static bool sDebug" at the top of the soundsourcemediafoundation.cpp and then guard all qDebug()'s with that? It could be useful to be able to turn all of those debugs on with a single flag if something needs debugging in the future.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

It all looks pretty good to me. I didn't delve much into the API and its edge cases since I assume you and Albert both did that. In favor of getting quick feedback I'm merging this in before the 1.10 beta. We'll fix any problems that crop up on the fly. Thanks Bill!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: