Bug 180666 – Playing WebRTC video tracks should prevent from display to going to sleep

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[youenn fablet]

Playing webrtc video tracks should prevent from display to got to sleep.
Current strategy is based on video elements having both audio and video.
For webrtc, the audio might be rendered outside the video element (in another audio element or with web audio for instance).

[youenn fablet]

Created attachment 329021 [details]
Patch

[Darin Adler]

Comment on attachment 329021 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:6634
> +    // In case of remote media stream video tracks, audio might be rendered outside the corresponding video element.

This comment is mysterious. The code below says that if there is a non-capture, non-canvas video track it should prevent sleep. Is the reason it prevents sleep is that it *might* be playing audio? That really doesn’t explain things well to me.

The comment here should explain why the code below implements the correct rule, and I don’t think the comment does that. Generally speaking it seems we have risky behavior here, where we prevent sleep just because something *might* be audible or visible. That’s not so great, and hard to understand.

> Source/WebCore/html/HTMLMediaElement.cpp:6638
> +    bool hasRemoteMediaStreamVideoTrack = m_mediaStreamSrcObject && WTF::anyOf(m_mediaStreamSrcObject->getTracks(), [] (auto& track) {
> +        return track->privateTrack().type() == RealtimeMediaSource::Type::Video && !track->isCaptureTrack() && !track->isCanvas();
> +    });
> +    shouldBeAbleToSleep = shouldBeAbleToSleep && !hasRemoteMediaStreamVideoTrack;

Seems wasteful to do this computation if shouldBeAbleToSleep is already false. Making this a lambda could fix that.

    shouldBeAbleToSleep = shouldBeAbleToSleep && !([m_mediaStreamSrcObject] {
        return m_mediaStreamSrcObject && WTF::anyOf(m_mediaStreamSrcObject->getTracks(), [] (auto& track) {
            return track->privateTrack().type() == RealtimeMediaSource::Type::Video && !track->isCaptureTrack() && !track->isCanvas();
        });
    }());

[youenn fablet]

Created attachment 329174 [details]
Patch for landing

[youenn fablet]

(In reply to Darin Adler from comment #2)
> Comment on attachment 329021 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329021&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:6634
> > +    // In case of remote media stream video tracks, audio might be rendered outside the corresponding video element.
> 
> This comment is mysterious. The code below says that if there is a
> non-capture, non-canvas video track it should prevent sleep. Is the reason
> it prevents sleep is that it *might* be playing audio? That really doesn’t
> explain things well to me.

I tried to beef up the comment here.
I also added a isRemoteMediaStreamVideoTrack helper function to make the intent clearer.

In WebRTC land, we may have multiple video streams coming in, some only video, some only audio. Ideally, we would reason about all of these as a whole with something like: this web page is playing video in a visible element and audio, so let's not idle the screen. But this is prone to regression.
Hence the minimal fix here: dedicated to webrtc and based on a single media element.

> 
> The comment here should explain why the code below implements the correct
> rule, and I don’t think the comment does that. Generally speaking it seems
> we have risky behavior here, where we prevent sleep just because something
> *might* be audible or visible. That’s not so great, and hard to understand.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:6638
> > +    bool hasRemoteMediaStreamVideoTrack = m_mediaStreamSrcObject && WTF::anyOf(m_mediaStreamSrcObject->getTracks(), [] (auto& track) {
> > +        return track->privateTrack().type() == RealtimeMediaSource::Type::Video && !track->isCaptureTrack() && !track->isCanvas();
> > +    });
> > +    shouldBeAbleToSleep = shouldBeAbleToSleep && !hasRemoteMediaStreamVideoTrack;
> 
> Seems wasteful to do this computation if shouldBeAbleToSleep is already
> false. Making this a lambda could fix that.
> 
>     shouldBeAbleToSleep = shouldBeAbleToSleep && !([m_mediaStreamSrcObject] {
>         return m_mediaStreamSrcObject &&
> WTF::anyOf(m_mediaStreamSrcObject->getTracks(), [] (auto& track) {
>             return track->privateTrack().type() ==
> RealtimeMediaSource::Type::Video && !track->isCaptureTrack() &&
> !track->isCanvas();
>         });
>     }());


I updated the code by &&ing shouldBeAbleToSleep upfront instead of after the computation of hasRemoteMediaStreamVideoTrack.

[youenn fablet]

Created attachment 329175 [details]
Fixing WPE

[WebKit Commit Bot]

Comment on attachment 329175 [details]
Fixing WPE

Clearing flags on attachment: 329175

Committed r225822: <https://trac.webkit.org/changeset/225822>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36008948>

[Ryan Haddad]

This change broke the Apple Windows build:
https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/6539

C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6616): error C2065: 'MediaStreamTrack': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6616): error C2923: 'WTF::RefPtr': 'MediaStreamTrack' is not a valid template type argument for parameter 'T' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6618): error C2662: 'T *WTF::RefPtr<T>::get(void) const': cannot convert 'this' pointer from 'WTF::RefPtr' to 'const WTF::RefPtr<T> &' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6618): note: Reason: cannot convert from 'WTF::RefPtr' to 'const WTF::RefPtr<T>'
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6618): note: Conversion requires a second user-defined-conversion operator or constructor
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): error C3536: 'track': cannot be used before it is initialized [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): error C2227: left of '->privateTrack' must point to class/struct/union/generic type [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): note: type is 'int'
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): error C2228: left of '.type' must have class/struct/union [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): error C2653: 'RealtimeMediaSource': is not a class or namespace name [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): error C2065: 'Video': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): error C2227: left of '->isCaptureTrack' must point to class/struct/union/generic type [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): note: type is 'int'
C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): error C2227: left of '->isCanvas' must point to class/struct/union/generic type [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\html/HTMLMediaElement.cpp(6619): note: type is 'int'

[Michael Catanzaro]

Committed r225830: <https://trac.webkit.org/changeset/225830>

[Michael Catanzaro]

(In reply to Ryan Haddad from comment #9)
> This change broke the Apple Windows build:
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 6539

Should be fixed now.

[Michael Catanzaro]

BTW Youenn, you might want to look at bug #180197. The HTMLMediaElement sleep disabling code definitely does not work properly for GTK, though I suppose it's possible there are platform-specific differences that could be at play.

[youenn fablet]

(In reply to Michael Catanzaro from comment #11)
> (In reply to Ryan Haddad from comment #9)
> > This change broke the Apple Windows build:
> > https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> > 6539
> 
> Should be fixed now.

Thanks!