Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
443783003:
Turn off AVFoundation field trial (Closed)
|
Created:
6 years, 4 months ago by vrk (LEFT CHROMIUM) Modified:
6 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, tnakamura Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionTurn off AVFoundation field trial
Turning off AVFoundation by default on ToT, Canary, and in field trials
in preparation for M38 branch cut.
BUG=396810, 394315, 356106
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288054
Patch Set 1 #
Total comments: 4
Patch Set 2 : revert glue changes #
Messages
Total messages: 13 (0 generated)
what? i am reading this on my phone and looks very wrong. why not simply change the finch config? On 6 Aug 2014 03:51, <vrk@chromium.org> wrote: > Reviewers: tommi, > > Description: > Turn off AVFoundation field trial > > Turning off AVFoundation by default on ToT, Canary, and in field trials > in preparation for M38 branch cut. > > BUG=396810,394315,356106 > > Please review this at https://codereview.chromium.org/443783003/ > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+1, -17 lines): > M chrome/browser/media/media_capture_devices_dispatcher.cc > M media/video/capture/mac/avfoundation_glue.mm > > > Index: chrome/browser/media/media_capture_devices_dispatcher.cc > diff --git a/chrome/browser/media/media_capture_devices_dispatcher.cc > b/chrome/browser/media/media_capture_devices_dispatcher.cc > index 72f6f2449c9af91fd1d921f47537798b6ea9fa5c.. > 30d3d75907ca135ef6cfd8cdabc637ed10793d2f 100644 > --- a/chrome/browser/media/media_capture_devices_dispatcher.cc > +++ b/chrome/browser/media/media_capture_devices_dispatcher.cc > @@ -274,19 +274,6 @@ MediaCaptureDevicesDispatcher:: > MediaCaptureDevicesDispatcher() > notifications_registrar_.Add( > this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED, > content::NotificationService::AllSources()); > - > - // AVFoundation is used for video/audio device monitoring and video > capture in > - // Mac. Experimentally, connect it in Canary and Unknown (developer > builds). > -#if defined(OS_MACOSX) > - chrome::VersionInfo::Channel channel = chrome::VersionInfo:: > GetChannel(); > - if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kForceQTKit)) > { > - if (channel == chrome::VersionInfo::CHANNEL_CANARY || > - channel == chrome::VersionInfo::CHANNEL_UNKNOWN) { > - CommandLine::ForCurrentProcess()->AppendSwitch( > - switches::kEnableAVFoundation); > - } > - } > -#endif > } > > MediaCaptureDevicesDispatcher::~MediaCaptureDevicesDispatcher() {} > Index: media/video/capture/mac/avfoundation_glue.mm > diff --git a/media/video/capture/mac/avfoundation_glue.mm > b/media/video/capture/mac/avfoundation_glue.mm > index 551f759df59679b5998e2a4583edd7c2a0ad85bc.. > cbcfc6f196e374e2a50a4b172c589ec9ee610fe8 100644 > --- a/media/video/capture/mac/avfoundation_glue.mm > +++ b/media/video/capture/mac/avfoundation_glue.mm > @@ -9,7 +9,6 @@ > #include "base/command_line.h" > #include "base/lazy_instance.h" > #include "base/mac/mac_util.h" > -#include "base/metrics/field_trial.h" > #include "media/base/media_switches.h" > > namespace { > @@ -125,9 +124,7 @@ bool AVFoundationGlue::IsAVFoundationSupported() { > // crbug.com/396764 > // TODO(vrk): Does this really need to be static? > static bool should_enable_avfoundation = > - command_line->HasSwitch(switches::kEnableAVFoundation) || > - base::FieldTrialList::FindFullName("AVFoundationMacVideoCapture") > - == "Enabled"; > + command_line->HasSwitch(switches::kEnableAVFoundation); > // Try to load AVFoundation. Save result in static bool to avoid loading > // AVFoundationBundle every call. > static bool loaded_successfully = [AVFoundationBundle() load]; > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I am doing a CL do push the experiment back to 0 (will CC you two). In the meantime please remove this section that hard-codes forcing the AVFoundation if on Canary or Unknown channel. https://codereview.chromium.org/443783003/diff/1/chrome/browser/media/media_c... File chrome/browser/media/media_capture_devices_dispatcher.cc (left): https://codereview.chromium.org/443783003/diff/1/chrome/browser/media/media_c... chrome/browser/media/media_capture_devices_dispatcher.cc:280: #if defined(OS_MACOSX) I wonder why this was here and not controlled by the finch experiment? Now I understand Victoria concerns... Anyhow this part of the CL LGTM. https://codereview.chromium.org/443783003/diff/1/media/video/capture/mac/avfo... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/443783003/diff/1/media/video/capture/mac/avfo... media/video/capture/mac/avfoundation_glue.mm:127: command_line->HasSwitch(switches::kEnableAVFoundation); Why remove the finch experiment here?
No lgtm until the finch comment is addressed.
Thanks Andre! PTAL. https://codereview.chromium.org/443783003/diff/1/chrome/browser/media/media_c... File chrome/browser/media/media_capture_devices_dispatcher.cc (left): https://codereview.chromium.org/443783003/diff/1/chrome/browser/media/media_c... chrome/browser/media/media_capture_devices_dispatcher.cc:280: #if defined(OS_MACOSX) On 2014/08/06 06:50:33, andresp wrote: > I wonder why this was here and not controlled by the finch experiment? > > Now I understand Victoria concerns... Anyhow this part of the CL LGTM. Acknowledged. https://codereview.chromium.org/443783003/diff/1/media/video/capture/mac/avfo... File media/video/capture/mac/avfoundation_glue.mm (right): https://codereview.chromium.org/443783003/diff/1/media/video/capture/mac/avfo... media/video/capture/mac/avfoundation_glue.mm:127: command_line->HasSwitch(switches::kEnableAVFoundation); On 2014/08/06 06:50:33, andresp wrote: > Why remove the finch experiment here? My thinking was that if we never wanted to turn on the finch experiment in M38 (which we wouldn't if we made M39 the min required version) then might as well take it out of the binary entirely. But I'm fine to revert it - done.
On 2014/08/07 01:25:00, Victoria Kirst wrote: > Thanks Andre! PTAL. > > https://codereview.chromium.org/443783003/diff/1/chrome/browser/media/media_c... > File chrome/browser/media/media_capture_devices_dispatcher.cc (left): > > https://codereview.chromium.org/443783003/diff/1/chrome/browser/media/media_c... > chrome/browser/media/media_capture_devices_dispatcher.cc:280: #if > defined(OS_MACOSX) > On 2014/08/06 06:50:33, andresp wrote: > > I wonder why this was here and not controlled by the finch experiment? > > > > Now I understand Victoria concerns... Anyhow this part of the CL LGTM. > > Acknowledged. > > https://codereview.chromium.org/443783003/diff/1/media/video/capture/mac/avfo... > File media/video/capture/mac/avfoundation_glue.mm (right): > > https://codereview.chromium.org/443783003/diff/1/media/video/capture/mac/avfo... > media/video/capture/mac/avfoundation_glue.mm:127: > command_line->HasSwitch(switches::kEnableAVFoundation); > On 2014/08/06 06:50:33, andresp wrote: > > Why remove the finch experiment here? > > My thinking was that if we never wanted to turn on the finch experiment in M38 > (which we wouldn't if we made M39 the min required version) then might as well > take it out of the binary entirely. But I'm fine to revert it - done. LGTM
The CQ bit was checked by andresp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vrk@chromium.org/443783003/20001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional 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.
lgtm
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Message was sent while issue was closed.
Change committed as 288054 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
