Bug 170771

Created attachment 306920 [details] Patch

Comment on attachment 306920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306920&action=review > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:418 > + err = setupAudioUnits(); If err is not null, I would guess m_ioUnit to no longer be null. If err is null, is m_ioUnit null? Should we clean it? How about setupAudioUnits be returning a m_ioUnit instead of an err? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:423 > + err = AudioOutputUnitStart(m_ioUnit); If err, do we need to do some clean-up related to m_ioUnit? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:-428 > - m_ioUnitStarted = true; Should we do ASSERT(m_ioUnit) here? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:438 > + if (m_ioUnit) { If there is no m_ioUnit, do we still want to call setMuted(true)? If not, we could return early if !m_ioUnit.

Comment on attachment 306920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306920&action=review >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:418 >> + err = setupAudioUnits(); > > If err is not null, I would guess m_ioUnit to no longer be null. > If err is null, is m_ioUnit null? Should we clean it? > How about setupAudioUnits be returning a m_ioUnit instead of an err? Sounds good, but that is outside the scope of this change. >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:423 >> + err = AudioOutputUnitStart(m_ioUnit); > > If err, do we need to do some clean-up related to m_ioUnit? Maybe. The unit will be cleaned-up in the destructor. In the mean-time no audio will be produced. >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:-428 >> - m_ioUnitStarted = true; > > Should we do ASSERT(m_ioUnit) here? I can add it even earlier, right after setupAudioUnits succeeds. >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:438 >> + if (m_ioUnit) { > > If there is no m_ioUnit, do we still want to call setMuted(true)? > If not, we could return early if !m_ioUnit. Yes, we still want to call setMuted even if there is no audio unit so that the state is still correct when the unit is later created.

Created attachment 307015 [details] Patch

(In reply to Jeremy Jones from comment #4) > Comment on attachment 306920 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306920&action=review > > >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:418 > >> + err = setupAudioUnits(); > > > > If err is not null, I would guess m_ioUnit to no longer be null. > > If err is null, is m_ioUnit null? Should we clean it? It is probably error prone to have an error but have a non null m_ioUnit. Can we clean it here or ensure it is null if there is an error? > > How about setupAudioUnits be returning a m_ioUnit instead of an err? > > Sounds good, but that is outside the scope of this change. OK > >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:423 > >> + err = AudioOutputUnitStart(m_ioUnit); > > > > If err, do we need to do some clean-up related to m_ioUnit? > > Maybe. The unit will be cleaned-up in the destructor. In the mean-time no > audio will be produced. It seems error-prone to leave the class in an unstable state (muted, m_ioUnit not null, error occurred). Can we do the clean-up in this patch if that is not too difficult? > > If there is no m_ioUnit, do we still want to call setMuted(true)? > > If not, we could return early if !m_ioUnit. > > Yes, we still want to call setMuted even if there is no audio unit so that > the state is still correct when the unit is later created. If there is no m_ioUnit, muted should already be false and setMuted will be a no-op, right?

(In reply to youenn fablet from comment #6) > (In reply to Jeremy Jones from comment #4) > > Comment on attachment 306920 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=306920&action=review > > > > >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:418 > > >> + err = setupAudioUnits(); > > > > > > If err is not null, I would guess m_ioUnit to no longer be null. > > > If err is null, is m_ioUnit null? Should we clean it? > > It is probably error prone to have an error but have a non null m_ioUnit. > Can we clean it here or ensure it is null if there is an error? I now clean up everything in case of an error here. > > > > How about setupAudioUnits be returning a m_ioUnit instead of an err? > > > > Sounds good, but that is outside the scope of this change. > > OK > > > >> Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:423 > > >> + err = AudioOutputUnitStart(m_ioUnit); > > > > > > If err, do we need to do some clean-up related to m_ioUnit? > > > > Maybe. The unit will be cleaned-up in the destructor. In the mean-time no > > audio will be produced. > > It seems error-prone to leave the class in an unstable state (muted, > m_ioUnit not null, error occurred). Can we do the clean-up in this patch if > that is not too difficult? Added cleanupAudioUnits() after setupAudioUnits() fails. > > > > If there is no m_ioUnit, do we still want to call setMuted(true)? > > > If not, we could return early if !m_ioUnit. > > > > Yes, we still want to call setMuted even if there is no audio unit so that > > the state is still correct when the unit is later created. > > If there is no m_ioUnit, muted should already be false and setMuted will be > a no-op, right? Setting m_muted directly in the constructor instead of calling setMuted().

Created attachment 307287 [details] Patch

Created attachment 307291 [details] Patch

Created attachment 307303 [details] Patch for landing.

Comment on attachment 307303 [details] Patch for landing. Attachment 307303 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3553540 New failing tests: webrtc/multi-video.html

Created attachment 307315 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 307303 [details] Patch for landing. Attachment 307303 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3553809 New failing tests: webrtc/no-port-zero-in-upd-candidates.html

Created attachment 307324 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 307303 [details] Patch for landing. Clearing flags on attachment: 307303 Committed r215485: <http://trac.webkit.org/changeset/215485>