Bug 209919

Summary: Don't call -[AVCapture startRunning] when interruption ends
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: WebRTCAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Eric Carlson 2020-04-02 12:19:47 PDT
Don't call -[AVCapture startRunning] when interruption ends
Comment 1 Eric Carlson 2020-04-02 12:20:19 PDT
<rdar://problem/61090625>
Comment 2 Eric Carlson 2020-04-02 12:44:52 PDT
Created attachment 395288 [details]
Patch
Comment 3 youenn fablet 2020-04-02 13:14:41 PDT
Comment on attachment 395288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395288&action=review

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:623
>      m_interruption = InterruptionReason::None;

Maybe we could switch m_interruption to "boolean m_interrupted"?

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:698
> +    if (!willChange && m_callback->loggerPtr() && m_callback->logger().willLog(m_callback->logChannel(), WTFLogLevel::Always)) {

Why !willChange?
I would think could write it as if (willChange) or maybe if(isChanging).

Should we return early if wilChange is true?
Comment 4 Eric Carlson 2020-04-02 14:52:29 PDT
Created attachment 395305 [details]
Patch for landing
Comment 5 Eric Carlson 2020-04-02 15:30:45 PDT
Comment on attachment 395288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395288&action=review

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:623
>>      m_interruption = InterruptionReason::None;
> 
> Maybe we could switch m_interruption to "boolean m_interrupted"?

Good idea, fixed.

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:698
>> +    if (!willChange && m_callback->loggerPtr() && m_callback->logger().willLog(m_callback->logChannel(), WTFLogLevel::Always)) {
> 
> Why !willChange?
> I would think could write it as if (willChange) or maybe if(isChanging).
> 
> Should we return early if wilChange is true?

Also a good idea, also fixed.
Comment 6 EWS 2020-04-02 17:14:03 PDT
Committed r259430: <https://trac.webkit.org/changeset/259430>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395305 [details].