Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1613863002:
Upgrade to Opus 1.1.2 (Closed)
|
Created:
4 years, 11 months ago by flim-chromium Modified:
4 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpgrade to Opus 1.1.2
BUG=580524
Committed: https://crrev.com/c6076f2213725d472f17191c54ce0e3232a07537
Cr-Commit-Position: refs/heads/master@{#372974}
Patch Set 1 #Patch Set 2 : Whitelist Opus license as its autogen.sh file is missing a license header #
Total comments: 2
Patch Set 3 : Remove '-mfpu=vfpv3-d16' when compiling with '-mfpu=neon' #
Total comments: 2
Patch Set 4 : Narrow opus license exception to just its autogen.sh file #
Messages
Total messages: 51 (12 generated)
kjellander@chromium.org changed reviewers: + kjellander@chromium.org
I think you need to add an exception for opus in https://code.google.com/p/chromium/codesearch#chromium/src/tools/checklicense... for third_party/opus. I assume that at the time when the previous roll of opus happened, this license check was probably not mandatory. You can have a look at https://codereview.chromium.org/1106973002 to see how it's done. I think you'll need to file a bug against the opus project to add license headers for all their sources (even the src/autogen.sh file that seems to fail the license check). Then reference that bug in your checklicenses.py entry.
Description was changed from ========== Upgrade to Opus 1.1.2 BUG= ========== to ========== Upgrade to Opus 1.1.2 BUG=580524 ==========
flim@chromium.org changed reviewers: + jansson@chromium.org
Can you please help to review this? I've updated checklicenses.py as well now.
On 2016/01/26 15:26:29, flim-chromium wrote: > Can you please help to review this? I've updated checklicenses.py as well now. I'm not super familiar with this but LGTM. One question though, why hasn't Opus been part of checklicenses.py before?
On 2016/01/27 09:30:49, jansson wrote: > On 2016/01/26 15:26:29, flim-chromium wrote: > > Can you please help to review this? I've updated checklicenses.py as well now. > > I'm not super familiar with this but LGTM. One question though, why hasn't Opus > been part of checklicenses.py before? Thanks. Turns out that the new file autogen.sh doesn't have a license header and was causing the test failure. Other files have the correct headers so Opus didn't need to be whitelisted in checklicenses.py before.
kjellander@chromium.org changed reviewers: + phajdan.jr@chromium.org
lgtm with a comment. +phajdan for OWNERS in tools/checklicenses/ https://codereview.chromium.org/1613863002/diff/20001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/1613863002/diff/20001/third_party/opus/opus.g... third_party/opus/opus.gyp:159: 'cflags!': [ '-mfpu=vfpv3-d16' ], This is not needed for the GN build?
LGTM w/comment https://codereview.chromium.org/1613863002/diff/40001/tools/checklicenses/che... File tools/checklicenses/checklicenses.py (right): https://codereview.chromium.org/1613863002/diff/40001/tools/checklicenses/che... tools/checklicenses/checklicenses.py:429: 'third_party/opus' : [ # https://trac.xiph.org/ticket/2253#ticket Is this limited to just autogen.sh? Please narrow the exception just to that one file then. Fine otherwise.
flim@chromium.org changed reviewers: + henrika@chromium.org, minyue@chromium.org
+henrika for OWNERS and +minyue https://codereview.chromium.org/1613863002/diff/20001/third_party/opus/opus.gyp File third_party/opus/opus.gyp (right): https://codereview.chromium.org/1613863002/diff/20001/third_party/opus/opus.g... third_party/opus/opus.gyp:159: 'cflags!': [ '-mfpu=vfpv3-d16' ], On 2016/01/28 08:06:07, kjellander-cr (OOO til Jan 29) wrote: > This is not needed for the GN build? Fixed. https://codereview.chromium.org/1613863002/diff/40001/tools/checklicenses/che... File tools/checklicenses/checklicenses.py (right): https://codereview.chromium.org/1613863002/diff/40001/tools/checklicenses/che... tools/checklicenses/checklicenses.py:429: 'third_party/opus' : [ # https://trac.xiph.org/ticket/2253#ticket On 2016/01/28 16:51:55, Paweł Hajdan Jr. wrote: > Is this limited to just autogen.sh? Please narrow the exception just to that one > file then. Fine otherwise. Done.
I am only and "admin" owner. Will give OK when minyue says OK.
good work. I see some new files e.g., celt/x86/*.*, are we not using them?
On 2016/02/02 08:37:57, minyue wrote: > good work. I see some new files e.g., celt/x86/*.*, are we not using them? You're right, we're not using them here. The focus was on improving performance for ARM so I didn't investigate/test the new SSE optimizations.
On 2016/02/02 09:13:46, flim-chromium wrote: > On 2016/02/02 08:37:57, minyue wrote: > > good work. I see some new files e.g., celt/x86/*.*, are we not using them? > > You're right, we're not using them here. The focus was on improving performance > for ARM so I didn't investigate/test the new SSE optimizations. Ok. LGTM
LGTM
The CQ bit was checked by flim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@chromium.org, jansson@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/1613863002/#ps60001 (title: "Narrow opus license exception to just its autogen.sh file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1613863002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1613863002/60001
The CQ bit was unchecked by commit-bot@chromium.org
The author flim@chromium.org has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by flim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1613863002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1613863002/60001
Message was sent while issue was closed.
Description was changed from ========== Upgrade to Opus 1.1.2 BUG=580524 ========== to ========== Upgrade to Opus 1.1.2 BUG=580524 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Upgrade to Opus 1.1.2 BUG=580524 ========== to ========== Upgrade to Opus 1.1.2 BUG=580524 Committed: https://crrev.com/c6076f2213725d472f17191c54ce0e3232a07537 Cr-Commit-Position: refs/heads/master@{#372974} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c6076f2213725d472f17191c54ce0e3232a07537 Cr-Commit-Position: refs/heads/master@{#372974}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
It looks like this brings in some undefined behavior:
../../third_party/opus/src/silk/macros.h:98:5: error: macro expansion producing
'defined' has undefined behavior [-Werror,-Wexpansion-to-defined]
#if OPUS_FAST_INT64
^
../../third_party/opus/src/silk/macros.h:47:26: note: expanded from macro
'OPUS_FAST_INT64'
#define OPUS_FAST_INT64 (defined(__x86_64__) || defined(__LP64__) ||
defined(_WIN64))
^
Can you change
#define OPUS_FAST_INT64 (defined(__x86_64__) || defined(__LP64__) ||
defined(_WIN64))
to
#if defined(__x86_64__) || defined(__LP64__) || defined(_WIN64)
#define OPUS_FAST_INT64 1
#else
#define OPUS_FAST_INT64 0
#endif
upstream and roll in that fix? As-is, this blocks a compiler update that flags
this issue. (Which I also wanted to land today, so this is a bit of an
unfortunate collision)
Message was sent while issue was closed.
On 2016/02/02 19:14:20, Nico wrote: > It looks like this brings in some undefined behavior: > > ../../third_party/opus/src/silk/macros.h:98:5: error: macro expansion producing > 'defined' has undefined behavior [-Werror,-Wexpansion-to-defined] > #if OPUS_FAST_INT64 > ^ > ../../third_party/opus/src/silk/macros.h:47:26: note: expanded from macro > 'OPUS_FAST_INT64' > #define OPUS_FAST_INT64 (defined(__x86_64__) || defined(__LP64__) || > defined(_WIN64)) > ^ > > > Can you change > > #define OPUS_FAST_INT64 (defined(__x86_64__) || defined(__LP64__) || > defined(_WIN64)) > > > to > > #if defined(__x86_64__) || defined(__LP64__) || defined(_WIN64) > #define OPUS_FAST_INT64 1 > #else > #define OPUS_FAST_INT64 0 > #endif > > upstream and roll in that fix? As-is, this blocks a compiler update that flags > this issue. (Which I also wanted to land today, so this is a bit of an > unfortunate collision) Since this is external code, we shouldn't change the source code. Can you disable the -Wexpansion-to-defined warning in opus.gyp to unblock your work? Then I suggest flim@ files a bug against Opus to get it fixed upstream (https://wiki.xiph.org/OpusFAQ#How_do_I_report_a_bug.3F).
Message was sent while issue was closed.
On 2016/02/02 20:13:15, kjellander (chromium) wrote: > On 2016/02/02 19:14:20, Nico wrote: > > It looks like this brings in some undefined behavior: > > > > ../../third_party/opus/src/silk/macros.h:98:5: error: macro expansion > producing > > 'defined' has undefined behavior [-Werror,-Wexpansion-to-defined] > > #if OPUS_FAST_INT64 > > ^ > > ../../third_party/opus/src/silk/macros.h:47:26: note: expanded from macro > > 'OPUS_FAST_INT64' > > #define OPUS_FAST_INT64 (defined(__x86_64__) || defined(__LP64__) || > > defined(_WIN64)) > > ^ > > > > > > Can you change > > > > #define OPUS_FAST_INT64 (defined(__x86_64__) || defined(__LP64__) || > > defined(_WIN64)) > > > > > > to > > > > #if defined(__x86_64__) || defined(__LP64__) || defined(_WIN64) > > #define OPUS_FAST_INT64 1 > > #else > > #define OPUS_FAST_INT64 0 > > #endif > > > > upstream and roll in that fix? As-is, this blocks a compiler update that flags > > this issue. (Which I also wanted to land today, so this is a bit of an > > unfortunate collision) > > Since this is external code, we shouldn't change the source code. > Can you disable the -Wexpansion-to-defined warning in opus.gyp to unblock your > work? Yes, that's what I'm doing. > Then I suggest flim@ files a bug against Opus to get it fixed upstream > (https://wiki.xiph.org/OpusFAQ#How_do_I_report_a_bug.3F). flim, this text might be useful for your bugreport: http://llvm.org/viewvc/llvm-project?view=revision&revision=258128
Message was sent while issue was closed.
On 2016/02/02 20:14:54, Nico wrote: > On 2016/02/02 20:13:15, kjellander (chromium) wrote: > > On 2016/02/02 19:14:20, Nico wrote: > > > It looks like this brings in some undefined behavior: > > > > > > ../../third_party/opus/src/silk/macros.h:98:5: error: macro expansion > > producing > > > 'defined' has undefined behavior [-Werror,-Wexpansion-to-defined] > > > #if OPUS_FAST_INT64 > > > ^ > > > ../../third_party/opus/src/silk/macros.h:47:26: note: expanded from macro > > > 'OPUS_FAST_INT64' > > > #define OPUS_FAST_INT64 (defined(__x86_64__) || defined(__LP64__) || > > > defined(_WIN64)) > > > ^ > > > > > > > > > Can you change > > > > > > #define OPUS_FAST_INT64 (defined(__x86_64__) || defined(__LP64__) || > > > defined(_WIN64)) > > > > > > > > > to > > > > > > #if defined(__x86_64__) || defined(__LP64__) || defined(_WIN64) > > > #define OPUS_FAST_INT64 1 > > > #else > > > #define OPUS_FAST_INT64 0 > > > #endif > > > > > > upstream and roll in that fix? As-is, this blocks a compiler update that > flags > > > this issue. (Which I also wanted to land today, so this is a bit of an > > > unfortunate collision) > > > > Since this is external code, we shouldn't change the source code. > > Can you disable the -Wexpansion-to-defined warning in opus.gyp to unblock your > > work? > > Yes, that's what I'm doing. (Or what I'm trying to do -- opus is also built by pnacl, which doesn't know this new warning flag yet, which makes things a bit annoying)
Message was sent while issue was closed.
(Also note that silk/macros.h likely now does the wrong thing with msvc, so if this code is used on Windows you want to get this addressed very quickly. See the llvm commit message I linked to above.)
Message was sent while issue was closed.
On 2016/02/02 20:27:54, Nico wrote: > (Also note that silk/macros.h likely now does the wrong thing with msvc, so if > this code is used on Windows you want to get this addressed very quickly. See > the llvm commit message I linked to above.) Sure I will get this fixed upstream. Thanks for the link to the llvm commit message.
Message was sent while issue was closed.
On 2016/02/03 09:05:10, flim-chromium wrote: > On 2016/02/02 20:27:54, Nico wrote: > > (Also note that silk/macros.h likely now does the wrong thing with msvc, so if > > this code is used on Windows you want to get this addressed very quickly. See > > the llvm commit message I linked to above.) > > Sure I will get this fixed upstream. Thanks for the link to the llvm commit > message. Any luck?
Message was sent while issue was closed.
On 2016/02/08 15:51:59, Nico wrote: > On 2016/02/03 09:05:10, flim-chromium wrote: > > On 2016/02/02 20:27:54, Nico wrote: > > > (Also note that silk/macros.h likely now does the wrong thing with msvc, so > if > > > this code is used on Windows you want to get this addressed very quickly. > See > > > the llvm commit message I linked to above.) > > > > Sure I will get this fixed upstream. Thanks for the link to the llvm commit > > message. > > Any luck? It has now been fixed upstream. Since this only affects a small optimization decision, and doesn't affect output quality, are you happy for us to wait until the next Opus release to pull it in?
Message was sent while issue was closed.
On 2016/02/09 15:45:31, flim-chromium wrote: > On 2016/02/08 15:51:59, Nico wrote: > > On 2016/02/03 09:05:10, flim-chromium wrote: > > > On 2016/02/02 20:27:54, Nico wrote: > > > > (Also note that silk/macros.h likely now does the wrong thing with msvc, > so > > if > > > > this code is used on Windows you want to get this addressed very quickly. > > See > > > > the llvm commit message I linked to above.) > > > > > > Sure I will get this fixed upstream. Thanks for the link to the llvm commit > > > message. > > > > Any luck? > > It has now been fixed upstream. Since this only affects a small optimization > decision, and doesn't affect output quality, are you happy for us to wait until > the next Opus release to pull it in? Unless it's a lot of work, I think it'd be nice to cherry-pick that change into our 1.1.2 opus in chromium deps - that way, we can remove the warning suppression and trybots can then make sure that new opus rolls don't bring in new instances of this problem.
Message was sent while issue was closed.
On 2016/02/09 15:47:53, Nico wrote: > On 2016/02/09 15:45:31, flim-chromium wrote: > > On 2016/02/08 15:51:59, Nico wrote: > > > On 2016/02/03 09:05:10, flim-chromium wrote: > > > > On 2016/02/02 20:27:54, Nico wrote: > > > > > (Also note that silk/macros.h likely now does the wrong thing with msvc, > > so > > > if > > > > > this code is used on Windows you want to get this addressed very > quickly. > > > See > > > > > the llvm commit message I linked to above.) > > > > > > > > Sure I will get this fixed upstream. Thanks for the link to the llvm > commit > > > > message. > > > > > > Any luck? > > > > It has now been fixed upstream. Since this only affects a small optimization > > decision, and doesn't affect output quality, are you happy for us to wait > until > > the next Opus release to pull it in? > > Unless it's a lot of work, I think it'd be nice to cherry-pick that change into > our 1.1.2 opus in chromium deps - that way, we can remove the warning > suppression and trybots can then make sure that new opus rolls don't bring in > new instances of this problem. Since we mirror the upstream repo, cherry picking would involve a lot of work for this small change. At the next opus release, I'll remove the warning suppression so that any new instances of this problem should be flagged.
Message was sent while issue was closed.
On 2016/02/10 11:12:07, flim-chromium wrote: > On 2016/02/09 15:47:53, Nico wrote: > > On 2016/02/09 15:45:31, flim-chromium wrote: > > > On 2016/02/08 15:51:59, Nico wrote: > > > > On 2016/02/03 09:05:10, flim-chromium wrote: > > > > > On 2016/02/02 20:27:54, Nico wrote: > > > > > > (Also note that silk/macros.h likely now does the wrong thing with > msvc, > > > so > > > > if > > > > > > this code is used on Windows you want to get this addressed very > > quickly. > > > > See > > > > > > the llvm commit message I linked to above.) > > > > > > > > > > Sure I will get this fixed upstream. Thanks for the link to the llvm > > commit > > > > > message. > > > > > > > > Any luck? > > > > > > It has now been fixed upstream. Since this only affects a small optimization > > > decision, and doesn't affect output quality, are you happy for us to wait > > until > > > the next Opus release to pull it in? > > > > Unless it's a lot of work, I think it'd be nice to cherry-pick that change > into > > our 1.1.2 opus in chromium deps - that way, we can remove the warning > > suppression and trybots can then make sure that new opus rolls don't bring in > > new instances of this problem. > > Since we mirror the upstream repo, cherry picking would involve a lot of work Isn't it just * create new branch in our git repo off the 1.1.2 branch * cherry-pick to that * change chromium DEPS to pull that revision ? > for this small change. At the next opus release, I'll remove the warning > suppression so that any new instances of this problem should be flagged. How often do we roll opus usually? If it's about once every month or two, I'm fine with waiting for that.
Message was sent while issue was closed.
On 2016/02/10 12:34:22, Nico wrote: > On 2016/02/10 11:12:07, flim-chromium wrote: > > On 2016/02/09 15:47:53, Nico wrote: > > > On 2016/02/09 15:45:31, flim-chromium wrote: > > > > On 2016/02/08 15:51:59, Nico wrote: > > > > > On 2016/02/03 09:05:10, flim-chromium wrote: > > > > > > On 2016/02/02 20:27:54, Nico wrote: > > > > > > > (Also note that silk/macros.h likely now does the wrong thing with > > msvc, > > > > so > > > > > if > > > > > > > this code is used on Windows you want to get this addressed very > > > quickly. > > > > > See > > > > > > > the llvm commit message I linked to above.) > > > > > > > > > > > > Sure I will get this fixed upstream. Thanks for the link to the llvm > > > commit > > > > > > message. > > > > > > > > > > Any luck? > > > > > > > > It has now been fixed upstream. Since this only affects a small > optimization > > > > decision, and doesn't affect output quality, are you happy for us to wait > > > until > > > > the next Opus release to pull it in? > > > > > > Unless it's a lot of work, I think it'd be nice to cherry-pick that change > > into > > > our 1.1.2 opus in chromium deps - that way, we can remove the warning > > > suppression and trybots can then make sure that new opus rolls don't bring > in > > > new instances of this problem. > > > > Since we mirror the upstream repo, cherry picking would involve a lot of work > > Isn't it just > > * create new branch in our git repo off the 1.1.2 branch > * cherry-pick to that > * change chromium DEPS to pull that revision > ? I assumed it was not possible to do local changes in a repo that is auto-mirrored from another repo, especially as this mirror is also mirroring all the branches from upstream. I mean, how would the git_updater service know how to handle a conflicting branch name etc? That's what I told flim@ when she asked me. If you really feel we should do/try this, we should check with the infra team first if that approach is recommended. > > for this small change. At the next opus release, I'll remove the warning > > suppression so that any new instances of this problem should be flagged. > > How often do we roll opus usually? If it's about once every month or two, I'm > fine with waiting for that. flim@ said the next release is scheduled for Q2, I think. So it won't be as long as this time (where it was a longer period).
Message was sent while issue was closed.
> I assumed it was not possible to do local changes in a repo that is > auto-mirrored from another repo, especially as this mirror is also mirroring all > the branches from upstream. > I mean, how would the git_updater service know how to handle a conflicting > branch name etc? That's what I told flim@ when she asked me. If you really feel > we should do/try this, we should check with the infra team first if that > approach is recommended. I think we have cherrypick branches on many deps – how else would you e.g. security fixes on stable if some bug is found? If creating a local branch doesn't Just Work, I'm fine with not spending a lot of time on this, but I would've expected this to be both possible and easy. (on conflicting branch names, I guess the tool could just complain – that's more a theoretical problem than an actual one imo if you give your branch and "obviously downstream"-looking name) > > > > for this small change. At the next opus release, I'll remove the warning > > > suppression so that any new instances of this problem should be flagged. > > > > How often do we roll opus usually? If it's about once every month or two, I'm > > fine with waiting for that. > > flim@ said the next release is scheduled for Q2, I think. So it won't be as long > as this time (where it was a longer period).
Message was sent while issue was closed.
On 2016/02/10 13:26:49, Nico wrote: > > I assumed it was not possible to do local changes in a repo that is > > auto-mirrored from another repo, especially as this mirror is also mirroring > all > > the branches from upstream. > > I mean, how would the git_updater service know how to handle a conflicting > > branch name etc? That's what I told flim@ when she asked me. If you really > feel > > we should do/try this, we should check with the infra team first if that > > approach is recommended. > > I think we have cherrypick branches on many deps – how else would you e.g. > security fixes on stable if some bug is found? > > If creating a local branch doesn't Just Work, I'm fine with not spending a lot > of time on this, but I would've expected this to be both possible and easy. (on > conflicting branch names, I guess the tool could just complain – that's more a > theoretical problem than an actual one imo if you give your branch and > "obviously downstream"-looking name) I've just tried creating a local branch and pushing it, but get "fatal: remote error: Access denied to flim@chromium.org". > > > > > > > for this small change. At the next opus release, I'll remove the warning > > > > suppression so that any new instances of this problem should be flagged. > > > > > > How often do we roll opus usually? If it's about once every month or two, > I'm > > > fine with waiting for that. > > > > flim@ said the next release is scheduled for Q2, I think. So it won't be as > long > > as this time (where it was a longer period).
Message was sent while issue was closed.
On 2016/02/10 13:35:00, flim-chromium wrote: > On 2016/02/10 13:26:49, Nico wrote: > > > I assumed it was not possible to do local changes in a repo that is > > > auto-mirrored from another repo, especially as this mirror is also mirroring > > all > > > the branches from upstream. > > > I mean, how would the git_updater service know how to handle a conflicting > > > branch name etc? That's what I told flim@ when she asked me. If you really > > feel > > > we should do/try this, we should check with the infra team first if that > > > approach is recommended. > > > > I think we have cherrypick branches on many deps – how else would you e.g. > > security fixes on stable if some bug is found? These Git repos are different. They're created and updated only by the git_updater service that's run by Chrome infra. The source of truth is somewhere else than at chromium.googlesource.com. That's not the case with Chromium's own repos, where we certainly create a lot of branches and do cherry-picking between them all the time. > > > > If creating a local branch doesn't Just Work, I'm fine with not spending a lot > > of time on this, but I would've expected this to be both possible and easy. > (on > > conflicting branch names, I guess the tool could just complain – that's more a > > theoretical problem than an actual one imo if you give your branch and > > "obviously downstream"-looking name) > > I've just tried creating a local branch and pushing it, but get "fatal: remote > error: Access denied to mailto:flim@chromium.org". > > > > > > > > > > > for this small change. At the next opus release, I'll remove the warning > > > > > suppression so that any new instances of this problem should be flagged. > > > > > > > > How often do we roll opus usually? If it's about once every month or two, > > I'm > > > > fine with waiting for that. > > > > > > flim@ said the next release is scheduled for Q2, I think. So it won't be as > > long > > > as this time (where it was a longer period).
Message was sent while issue was closed.
On 2016/02/10 13:38:03, kjellander (chromium) wrote: > On 2016/02/10 13:35:00, flim-chromium wrote: > > On 2016/02/10 13:26:49, Nico wrote: > > > > I assumed it was not possible to do local changes in a repo that is > > > > auto-mirrored from another repo, especially as this mirror is also > mirroring > > > all > > > > the branches from upstream. > > > > I mean, how would the git_updater service know how to handle a conflicting > > > > branch name etc? That's what I told flim@ when she asked me. If you really > > > feel > > > > we should do/try this, we should check with the infra team first if that > > > > approach is recommended. > > > > > > I think we have cherrypick branches on many deps – how else would you e.g. > > > security fixes on stable if some bug is found? > > These Git repos are different. They're created and updated only by the > git_updater service that's run by Chrome infra. The source of truth is somewhere > else than at http://chromium.googlesource.com. That's not the case with Chromium's own > repos, where we certainly create a lot of branches and do cherry-picking between > them all the time. Sorry I didn't read you referred to "deps". To be honest I don't know. I can only speak for deps like V8 and WebRTC - for them the source of truth is actually on chromium.googlesource.com, and we cut release branches when the Chromium cut happens. > > > > > > > If creating a local branch doesn't Just Work, I'm fine with not spending a > lot > > > of time on this, but I would've expected this to be both possible and easy. > > (on > > > conflicting branch names, I guess the tool could just complain – that's more > a > > > theoretical problem than an actual one imo if you give your branch and > > > "obviously downstream"-looking name) > > > > I've just tried creating a local branch and pushing it, but get "fatal: remote > > error: Access denied to mailto:flim@chromium.org". > > > > > > > > > > > > > > > for this small change. At the next opus release, I'll remove the > warning > > > > > > suppression so that any new instances of this problem should be > flagged. > > > > > > > > > > How often do we roll opus usually? If it's about once every month or > two, > > > I'm > > > > > fine with waiting for that. > > > > > > > > flim@ said the next release is scheduled for Q2, I think. So it won't be > as > > > long > > > > as this time (where it was a longer period).
Message was sent while issue was closed.
> I've just tried creating a local branch and pushing it, but get "fatal: remote > error: Access denied to mailto:flim@chromium.org". Ok, let's wait for the next regular update then. Thanks for trying! Figuring out how to have a cherry-picked local branch is something you might want to figure out anyways though, before it becomes a firedrill 'cause someone finds a security in opus on the stable channel :-) Maybe opus would be fine with having release branches for chromium in their upstream repo?
Message was sent while issue was closed.
On 2016/02/10 13:47:52, Nico wrote: > > I've just tried creating a local branch and pushing it, but get "fatal: remote > > error: Access denied to mailto:flim@chromium.org". > > Ok, let's wait for the next regular update then. Thanks for trying! > > > Figuring out how to have a cherry-picked local branch is something you might > want to figure out anyways though, before it becomes a firedrill 'cause someone > finds a security in opus on the stable channel :-) Maybe opus would be fine with > having release branches for chromium in their upstream repo? Yeah, I believe that would be fine considering the large amount of branches already: https://chromium.googlesource.com/chromium/deps/opus.git/+refs It could be useful for flim@ to reach out to the owners just to get access to creating such branches ahead of time, to save some time in case such a security exploit is found. Good suggestion Nico!
Message was sent while issue was closed.
On 2016/02/10 13:59:15, kjellander (chromium) wrote: > On 2016/02/10 13:47:52, Nico wrote: > > > I've just tried creating a local branch and pushing it, but get "fatal: > remote > > > error: Access denied to mailto:flim@chromium.org". > > > > Ok, let's wait for the next regular update then. Thanks for trying! > > > > > > Figuring out how to have a cherry-picked local branch is something you might > > want to figure out anyways though, before it becomes a firedrill 'cause > someone > > finds a security in opus on the stable channel :-) Maybe opus would be fine > with > > having release branches for chromium in their upstream repo? > > Yeah, I believe that would be fine considering the large amount of branches > already: > https://chromium.googlesource.com/chromium/deps/opus.git/+refs > > It could be useful for flim@ to reach out to the owners just to get access to > creating such branches ahead of time, to save some time in case such a security > exploit is found. Good suggestion Nico! Sounds good, I'll look into this.
Message was sent while issue was closed.
How's the update to the next opus with a fix for this coming along?
Message was sent while issue was closed.
On 2016/06/24 17:43:55, Nico wrote: > How's the update to the next opus with a fix for this coming along? We're still waiting for the next minor release which will include the fix for this. We're hoping it will be released soon.
Message was sent while issue was closed.
On 2016/06/27 08:42:08, flim-chromium wrote: > On 2016/06/24 17:43:55, Nico wrote: > > How's the update to the next opus with a fix for this coming along? > > We're still waiting for the next minor release which will include the fix for > this. We're hoping it will be released soon. flim removed the warning suppression in https://codereview.chromium.org/2184993002/ after updating Opus. Thank you, much appreciated! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
