Hey everyone,
Uwe and Daniel's work is awesome and much needed. SoundSource is the dark
corner of Mixxx that is on average where our lowest-quality code lives. It
hasn't been touched significantly in years and it will be great to fix the
long standing bugs associated with it. The flip side of not having been
touched much in years is that we have 5+ years worth of evidence that there
are no serious decoding (i.e. Critical priority) bugs in SoundSource code.
I'm with Owen on this one. The benefits of merging this branch pale in
comparison to the risks. This release will be our biggest in years and it
*will* make headlines. This is not the time for an epic refactor to fix
some -- frankly minor -- decoding glitches. It is not possible to get
sufficient testing of this code on the diverse libraries that exist in the
wild in the beta period for this release. There will surely be crash bugs
as there are with any refactor of complicated code (touches 5+ libraries)
that deals with corrupt and untrusted input (user audio files). Merging
this now will both delay the release with bugs we find during the beta
period and hurt Mixxx's reputation (at a critical time for gaining users)
with the crash bugs that we *don't* find during the beta period.
Finally, I don't believe Uwe has been operating under the impression that
this refactor would be part of this release. The PR milestone is clearly
marked 1.13 and IIRC we discussed it explicitly when he was working on it.
This was why he spit off multiple pieces of it into smaller refactors that
have merged already.
Cheers,
RJ
On Fri, Mar 20, 2015 at 6:48 PM, Owen Williams <owilli...@mixxx.org> wrote:
> I simply disagree that we have to apply the whole PR to get one bug fix.
> If there are are other similar issues there should be bugs filed for
> them so we can judge their severity. There is no reason to rush this
> patch in. I am not saying we will never merge this PR -- the audio
> subsystem does need help, and I really appreciate the work.
>
> The beta will have bugs. The release will have bugs. But I want to
> *have* a release, and if we merge this giant patch now that will set the
> release back by *months*.
>
> I don't feel any responsibility to find specific bugs with this PR or
> "prove" where there are hot spots. The risk is too high.
>
> owen
>
> On Fri, 2015-03-20 at 22:39 +0100, Daniel Schürmann wrote:
> > I have really reviewed every single line and it took me quite long, (see
> > the discussion at GitHub)
> > Yes it is really big, but it has received the same attention as every
> > small patch got, devided by the number of lines.
> >
> > Can you identify a hotspot of risky changes in the PR? Maybe we can
> > exude just this.
> >
> > Th PR Fixes a lot of issues in every sound-source. We have just the bug
> > reports for mp3, but Uwe and me have identified
> > a lot of more similar issues in the other sound-sources.
> > It is a good idea to have them all fixed in the upcomming release.
> >
> >
> > Am 20.03.2015 um 22:24 schrieb Owen Williams:
> > > I'm sorry but I can't just take your word for it that this 7000 line
> > > patch is safe to merge. I don't even trust myself to review it, it's
> > > too big. If it's the day after a big release then we can risk the
> > > breakage, but not right now. My experience has been that these reworks
> > > always introduce new problems.
> > >
> > > If there's a way to just fix the mp3 issues, let's do that.
> Backporting
> > > is annoying, but it's a pretty standard procedure on a major project.
> > >
> > > On Fri, 2015-03-20 at 22:15 +0100, Daniel Schürmann wrote:
> > >> Sure it is possible, but it feels stupid to do that.
> > >> Taking effort to exclude useful changes from a patch that is already
> > >> reviewed and from a high quality.
> > >> The result will be un-reviewed code introduced into a sound source
> > >> environment from a poor quality,
> > >> without additional tests.
> > >>
> > >>
> > >>
> > >> Am 20.03.2015 um 18:30 schrieb Owen Williams:
> > >>> How hard would it be to backport the mp3 fix to the current
> > >>> architecture? I would much prefer that.
> > >>>
> > >>> On Fri, 2015-03-20 at 17:18 +0100, Daniel Schürmann wrote:
> > >>>> Here are the bug:
> > >>>> https://bugs.launchpad.net/mixxx/+bug/1407394
> > >>>> https://bugs.launchpad.net/mixxx/+bug/1281654
> > >>>>
> > >>>> Uwe and I made significant progress with this beast:
> > >>>> https://github.com/mixxxdj/mixxx/pull/411
> > >>>> It was continuously improved and testes since 30 Nov 2014
> > >>>> Some important QM tasks are already finished:
> > >>>> * It is reviewed by a second pair of eyes.
> > >>>> * It has Unit-tests for the most difficult parts
> > >>>>
> > >>>> What is missing:
> > >>>> a) Fire ugly files against it
> > >>>> b) test in the wild bay a broad amount of users
> > >>>>
> > >>>> a) can't take long and b) is ideal done in a beat phase, we will
> never
> > >>>> have such an attention for the next two years.
> > >>>>
> > >>>> For me this advantages have an higher value than a diffuse change of
> > >>>> introducing new bugs.
> > >>>> And yes new code introduces always new bugs (not a big problem in a
> > >>>> beta)
> > >>>>
> > >>>>
> > >>>>
> > >>>> Am 20.03.2015 um 15:49 schrieb Owen Williams:
> > >>>>> I still need a pointer to what the mp3 bug even is.
> > >>>>>
> > >>>>> On Fri, 2015-03-20 at 07:43 -0700, Gavin Swanson wrote:
> > >>>>>> I exclusively use mp3s with mixxx. I would imagine there is a
> large
> > >>>>>> portion of the mixxx user base that does.
> > >>>>>> I do not agree with pushing a beta with this kind of issue, if the
> > >>>>>> intent is not to fix it for this release. If the intent is to get
> the
> > >>>>>> beta with all its other features out, and then work this issue
> through
> > >>>>>> the beta cycle so it is fixed for release, I could get behind
> that.
> > >>>>>> At the same time if the fix is a major-rework of the base of mixxx
> > >>>>>> then it doesn't make sense to do it during a beta cycle and we
> should
> > >>>>>> push beta until its ready. If it's a 1000 line fix with 6000
> lines of
> > >>>>>> unit tests (unlikely), that's a different story.
> > >>>>>>
> > >>>>>>
> > >>>>>> my ¢2
> > >>>>>>
> > >>>>>>
> > >>>>>> -Gavin S
> > >>>>>>
> > >>>>>> Gavin S
> > >>>>>>
> > >>>>>> On Fri, Mar 20, 2015 at 7:02 AM, Owen Williams <
> owilli...@mixxx.org>
> > >>>>>> wrote:
> > >>>>>> I can't in good conscience approve of merging a 7000-line
> > >>>>>> delta change
> > >>>>>> to the bedrock of the Mixxx engine, even if it fixes an
> issue
> > >>>>>> with mp3
> > >>>>>> seeking. (I also hate how mp3s skip on the first beat,
> so I
> > >>>>>> can
> > >>>>>> sympathize). I understand there are new tests, and I'm
> really
> > >>>>>> thankful
> > >>>>>> for those. But it's going to take a long time to go
> through
> > >>>>>> that patch
> > >>>>>> and then test to find all the issues with bad tags, weird
> > >>>>>> files and
> > >>>>>> formats, etc etc etc. I think RJ has a collection of
> horrible
> > >>>>>> mp3s that
> > >>>>>> he'll want to test.
> > >>>>>>
> > >>>>>> To me, sitting two years after the last release, I can't
> > >>>>>> justify the
> > >>>>>> delay that this merge will inevitably cause -- I think
> we're
> > >>>>>> ready for a
> > >>>>>> beta and can tolerate the mp3 issues we have. (by the
> way, is
> > >>>>>> there a
> > >>>>>> bug for the mp3 issue you're talking about? I can't find
> it.)
> > >>>>>>
> > >>>>>> owen
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>> On Fri, 2015-03-20 at 09:03 +0100, Daniel Schürmann wrote:
> > >>>>>> > The only remaining blocker for me is the seek offset
> issue
> > >>>>>> for mp3s.
> > >>>>>> >
> > >>>>>> >
> > >>>>>> > Since we have a fully reviewed solution in
> > >>>>>> > https://github.com/mixxxdj/mixxx/pull/411 of really
> high
> > >>>>>> code quality,
> > >>>>>> > verified by a new set of unittests,
> > >>>>>> >
> > >>>>>> > we should just merge it and release the beta.
> > >>>>>> >
> > >>>>>> > And yes there is a risk of introducing issues and it
> surely
> > >>>>>> will, but
> > >>>>>> > by now it fixes much more issues than it might
> introduce.
> > >>>>>> >
> > >>>>>> >
> > >>>>>> > Of cause I would be happy about any pending PR that can
> be
> > >>>>>> review and
> > >>>>>> > merged before.
> > >>>>>> >
> > >>>>>> >
> > >>>>>> > 2015-03-20 0:35 GMT+01:00 Owen Williams
> > >>>>>> <owilli...@mixxx.org>:
> > >>>>>> > So how about that release? Do we still have
> > >>>>>> crashing problems
> > >>>>>> > on
> > >>>>>> > windows? is anyone looking in to that? Any
> other
> > >>>>>> blockers?
> > >>>>>> >
> > >>>>>> > owen
> > >>>>>> >
> > >>>>>> >
> > >>>>>> >
> > >>>>>>
> ------------------------------------------------------------------------------
> > >>>>>> > Dive into the World of Parallel Programming The
> Go
> > >>>>>> Parallel
> > >>>>>> > Website, sponsored
> > >>>>>> > by Intel and developed in partnership with
> Slashdot
> > >>>>>> Media, is
> > >>>>>> > your hub for all
> > >>>>>> > things parallel software development, from
> weekly
> > >>>>>> thought
> > >>>>>> > leadership blogs to
> > >>>>>> > news, videos, case studies, tutorials and more.
> Take
> > >>>>>> a look
> > >>>>>> > and join the
> > >>>>>> > conversation now.
> http://goparallel.sourceforge.net/
> > >>>>>> > _______________________________________________
> > >>>>>> > Get Mixxx, the #1 Free MP3 DJ Mixing software
> Today
> > >>>>>> > http://mixxx.org
> > >>>>>> >
> > >>>>>> >
> > >>>>>> > Mixxx-devel mailing list
> > >>>>>> > Mixxx-devel@lists.sourceforge.net
> > >>>>>> >
> > >>>>>> https://lists.sourceforge.net/lists/listinfo/mixxx-devel
> > >>>>>> >
> > >>>>>> >
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>>
>
> ------------------------------------------------------------------------------
> > >>>>>> Dive into the World of Parallel Programming The Go
> Parallel
> > >>>>>> Website, sponsored
> > >>>>>> by Intel and developed in partnership with Slashdot
> Media, is
> > >>>>>> your hub for all
> > >>>>>> things parallel software development, from weekly thought
> > >>>>>> leadership blogs to
> > >>>>>> news, videos, case studies, tutorials and more. Take a
> look
> > >>>>>> and join the
> > >>>>>> conversation now. http://goparallel.sourceforge.net/
> > >>>>>> _______________________________________________
> > >>>>>> Get Mixxx, the #1 Free MP3 DJ Mixing software Today
> > >>>>>> http://mixxx.org
> > >>>>>>
> > >>>>>>
> > >>>>>> Mixxx-devel mailing list
> > >>>>>> Mixxx-devel@lists.sourceforge.net
> > >>>>>> https://lists.sourceforge.net/lists/listinfo/mixxx-devel
> > >>>>>>
> > >>>>>>
> > >>>>>>
> ------------------------------------------------------------------------------
> > >>>>>> Dive into the World of Parallel Programming The Go Parallel
> Website, sponsored
> > >>>>>> by Intel and developed in partnership with Slashdot Media, is
> your hub for all
> > >>>>>> things parallel software development, from weekly thought
> leadership blogs to
> > >>>>>> news, videos, case studies, tutorials and more. Take a look and
> join the
> > >>>>>> conversation now. http://goparallel.sourceforge.net/
> > >>>>>> _______________________________________________
> > >>>>>> Get Mixxx, the #1 Free MP3 DJ Mixing software Today
> > >>>>>> http://mixxx.org
> > >>>>>>
> > >>>>>>
> > >>>>>> Mixxx-devel mailing list
> > >>>>>> Mixxx-devel@lists.sourceforge.net
> > >>>>>> https://lists.sourceforge.net/lists/listinfo/mixxx-devel
> > >>>>>
> ------------------------------------------------------------------------------
> > >>>>> Dive into the World of Parallel Programming The Go Parallel
> Website, sponsored
> > >>>>> by Intel and developed in partnership with Slashdot Media, is your
> hub for all
> > >>>>> things parallel software development, from weekly thought
> leadership blogs to
> > >>>>> news, videos, case studies, tutorials and more. Take a look and
> join the
> > >>>>> conversation now. http://goparallel.sourceforge.net/
> > >>>>> _______________________________________________
> > >>>>> Get Mixxx, the #1 Free MP3 DJ Mixing software Today
> > >>>>> http://mixxx.org
> > >>>>>
> > >>>>>
> > >>>>> Mixxx-devel mailing list
> > >>>>> Mixxx-devel@lists.sourceforge.net
> > >>>>> https://lists.sourceforge.net/lists/listinfo/mixxx-devel
> > >>>>
> ------------------------------------------------------------------------------
> > >>>> Dive into the World of Parallel Programming The Go Parallel
> Website, sponsored
> > >>>> by Intel and developed in partnership with Slashdot Media, is your
> hub for all
> > >>>> things parallel software development, from weekly thought
> leadership blogs to
> > >>>> news, videos, case studies, tutorials and more. Take a look and
> join the
> > >>>> conversation now. http://goparallel.sourceforge.net/
> > >>>> _______________________________________________
> > >>>> Get Mixxx, the #1 Free MP3 DJ Mixing software Today
> > >>>> http://mixxx.org
> > >>>>
> > >>>>
> > >>>> Mixxx-devel mailing list
> > >>>> Mixxx-devel@lists.sourceforge.net
> > >>>> https://lists.sourceforge.net/lists/listinfo/mixxx-devel
> > >>
> > >>
> > >
> >
> >
> >
>
>
>
>
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming The Go Parallel Website,
> sponsored
> by Intel and developed in partnership with Slashdot Media, is your hub for
> all
> things parallel software development, from weekly thought leadership blogs
> to
> news, videos, case studies, tutorials and more. Take a look and join the
> conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> Get Mixxx, the #1 Free MP3 DJ Mixing software Today
> http://mixxx.org
>
>
> Mixxx-devel mailing list
> Mixxx-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/mixxx-devel
>
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Get Mixxx, the #1 Free MP3 DJ Mixing software Today
http://mixxx.org
Mixxx-devel mailing list
Mixxx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mixxx-devel
