Opus: Ensure that NLSF cannot be negative when computing a min distance between them

Opus: Ensure that NLSF cannot be negative when computing a min distance between them

Ref:
https://git.xiph.org/?p=opus.git;a=commit;h=79e8f527b0344b0897a65be35e77f7885bd99409

BUG=632124
Committed: https://crrev.com/7b9d8ea29454181f7210e06deff1df2d89347e80
Cr-Commit-Position: refs/heads/master@{#410621}

[flim-chromium, 2016-08-04 15:52:23]

flim@chromium.org changed reviewers:
+ henrika@chromium.org, minyue@chromium.org

[flim-chromium, 2016-08-04 15:52:24]

This change was merged in the upstream master branch and here's applying it
locally. Could you PTAL? Thanks :)

[minyue, 2016-08-04 15:58:32]

https://codereview.chromium.org/2214813002/diff/1/third_party/opus/README.chr...
File third_party/opus/README.chromium (right):

https://codereview.chromium.org/2214813002/diff/1/third_party/opus/README.chr...
third_party/opus/README.chromium:2: URL:
http://downloads.xiph.org/releases/opus/opus-1.1.3.tar.gz
will divert from this, right?

[flim-chromium, 2016-08-04 16:46:53]

https://codereview.chromium.org/2214813002/diff/1/third_party/opus/README.chr...
File third_party/opus/README.chromium (right):

https://codereview.chromium.org/2214813002/diff/1/third_party/opus/README.chr...
third_party/opus/README.chromium:2: URL:
http://downloads.xiph.org/releases/opus/opus-1.1.3.tar.gz
On 2016/08/04 15:58:32, minyue wrote:
> will divert from this, right?

Right, the local change is detailed below. Not sure if there is something else
more sensible to change this to though.

[minyue, 2016-08-05 11:50:25]

lgtm

https://codereview.chromium.org/2214813002/diff/1/third_party/opus/README.chr...
File third_party/opus/README.chromium (right):

https://codereview.chromium.org/2214813002/diff/1/third_party/opus/README.chr...
third_party/opus/README.chromium:2: URL:
http://downloads.xiph.org/releases/opus/opus-1.1.3.tar.gz
On 2016/08/04 16:46:53, flim-chromium wrote:
> On 2016/08/04 15:58:32, minyue wrote:
> > will divert from this, right?
> 
> Right, the local change is detailed below. Not sure if there is something else
> more sensible to change this to though. 

Acknowledged.

[flim-chromium, 2016-08-05 11:53:28]

The CQ bit was checked by flim@chromium.org

[commit-bot: I haz the power, 2016-08-05 11:53:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 11:53:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 11:53:44]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[flim-chromium, 2016-08-05 12:01:47]

flim@chromium.org changed reviewers:
+ sergeyu@chromium.org

[flim-chromium, 2016-08-05 12:01:48]

Hi sergeyu, could you PTAL? thanks :)

[Sergey Ulanov, 2016-08-05 22:44:06]

What's the reason to cherry-pick one change instead of just rolling opus to the
latest version?

[Sergey Ulanov, 2016-08-05 22:52:16]

opus is pulled from https://chromium.googlesource.com/chromium/deps/opus.git
(see src/DEPS ). The upstream change is already there:
https://chromium.googlesource.com/chromium/deps/opus.git/+/79e8f527b0344b0897...
. You just need to roll past that change by updating DEPS.

[flim-chromium, 2016-08-05 23:52:16]

On 2016/08/05 22:44:06, Sergey Ulanov wrote:
> What's the reason to cherry-pick one change instead of just rolling opus to
the
> latest version?

We are currently taking just the tagged releases of Opus, the latest of which
was released a couple of weeks ago and has since been updated here.

[Sergey Ulanov, 2016-08-08 17:41:19]

On 2016/08/05 23:52:16, flim-chromium wrote:
> On 2016/08/05 22:44:06, Sergey Ulanov wrote:
> > What's the reason to cherry-pick one change instead of just rolling opus to
> the
> > latest version?
> 
> We are currently taking just the tagged releases of Opus, the latest of which
> was released a couple of weeks ago and has since been updated here.

In this case you need to cherry-pick that change in
https://chromium.googlesource.com/chromium/deps/opus.git on a new branch and
then update src/DEPS to pull from that branch. I'm not sure how you managed to
create this CL because third_party/opus/src/silk/NLSF_stabilize.c is not in
src.git repo.

[flim-chromium, 2016-08-08 20:59:01]

On 2016/08/08 17:41:19, Sergey Ulanov wrote:
> On 2016/08/05 23:52:16, flim-chromium wrote:
> > On 2016/08/05 22:44:06, Sergey Ulanov wrote:
> > > What's the reason to cherry-pick one change instead of just rolling opus
to
> > the
> > > latest version?
> > 
> > We are currently taking just the tagged releases of Opus, the latest of
which
> > was released a couple of weeks ago and has since been updated here.
> 
> In this case you need to cherry-pick that change in
> https://chromium.googlesource.com/chromium/deps/opus.git on a new branch and
> then update src/DEPS to pull from that branch. I'm not sure how you managed to
> create this CL because third_party/opus/src/silk/NLSF_stabilize.c is not in
> src.git repo.

Sorry I should have mentioned that I'd recently moved Opus off DEPS and imported
it as a local copy (https://codereview.chromium.org/2195313002/) for such
changes. There was a discussion previously regarding cherry picking on a
separate branch at https://codereview.chromium.org/1613863002/, where it didn't
seem possible because it was mirroring the upstream repo.

[Sergey Ulanov, 2016-08-08 23:01:46]

On 2016/08/08 20:59:01, flim-chromium wrote:
> On 2016/08/08 17:41:19, Sergey Ulanov wrote:
> > On 2016/08/05 23:52:16, flim-chromium wrote:
> > > On 2016/08/05 22:44:06, Sergey Ulanov wrote:
> > > > What's the reason to cherry-pick one change instead of just rolling opus
> to
> > > the
> > > > latest version?
> > > 
> > > We are currently taking just the tagged releases of Opus, the latest of
> which
> > > was released a couple of weeks ago and has since been updated here.
> > 
> > In this case you need to cherry-pick that change in
> > https://chromium.googlesource.com/chromium/deps/opus.git on a new branch and
> > then update src/DEPS to pull from that branch. I'm not sure how you managed
to
> > create this CL because third_party/opus/src/silk/NLSF_stabilize.c is not in
> > src.git repo.
> 
> Sorry I should have mentioned that I'd recently moved Opus off DEPS and
imported
> it as a local copy (https://codereview.chromium.org/2195313002/) for such
> changes. 

I see. LGTM.

> There was a discussion previously regarding cherry picking on a
> separate branch at https://codereview.chromium.org/1613863002/, where it
didn't
> seem possible because it was mirroring the upstream repo.

Looks like there was also discussion about having branches upstream. Did this go
anywhere?

[flim-chromium, 2016-08-09 08:38:51]

On 2016/08/08 23:01:46, Sergey Ulanov wrote:
> On 2016/08/08 20:59:01, flim-chromium wrote:
> > On 2016/08/08 17:41:19, Sergey Ulanov wrote:
> > > On 2016/08/05 23:52:16, flim-chromium wrote:
> > > > On 2016/08/05 22:44:06, Sergey Ulanov wrote:
> > > > > What's the reason to cherry-pick one change instead of just rolling
opus
> > to
> > > > the
> > > > > latest version?
> > > > 
> > > > We are currently taking just the tagged releases of Opus, the latest of
> > which
> > > > was released a couple of weeks ago and has since been updated here.
> > > 
> > > In this case you need to cherry-pick that change in
> > > https://chromium.googlesource.com/chromium/deps/opus.git on a new branch
and
> > > then update src/DEPS to pull from that branch. I'm not sure how you
managed
> to
> > > create this CL because third_party/opus/src/silk/NLSF_stabilize.c is not
in
> > > src.git repo.
> > 
> > Sorry I should have mentioned that I'd recently moved Opus off DEPS and
> imported
> > it as a local copy (https://codereview.chromium.org/2195313002/) for such
> > changes. 
> 
> I see. LGTM.

Thanks for the review :)

> 
> > There was a discussion previously regarding cherry picking on a
> > separate branch at https://codereview.chromium.org/1613863002/, where it
> didn't
> > seem possible because it was mirroring the upstream repo.
> 
> Looks like there was also discussion about having branches upstream. Did this
go
> anywhere?

We did start a discussion but it was unclear what the advantage was of having an
upstream branch vs a locally hosted copy.

[flim-chromium, 2016-08-09 08:38:56]

The CQ bit was checked by flim@chromium.org

[commit-bot: I haz the power, 2016-08-09 08:39:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 09:27:06]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-09 09:29:07]

Description was changed from

==========
Opus: Ensure that NLSF cannot be negative when computing a min distance between
them

Ref:
https://git.xiph.org/?p=opus.git;a=commit;h=79e8f527b0344b0897a65be35e77f7885...

BUG=632124
==========

to

==========
Opus: Ensure that NLSF cannot be negative when computing a min distance between
them

Ref:
https://git.xiph.org/?p=opus.git;a=commit;h=79e8f527b0344b0897a65be35e77f7885...

BUG=632124

Committed: https://crrev.com/7b9d8ea29454181f7210e06deff1df2d89347e80
Cr-Commit-Position: refs/heads/master@{#410621}
==========

[commit-bot: I haz the power, 2016-08-09 09:29:08]

Patchset 1 (id:??) landed as
https://crrev.com/7b9d8ea29454181f7210e06deff1df2d89347e80
Cr-Commit-Position: refs/heads/master@{#410621}