Bug 132077

Summary: [Mac, iOS] Stop buffering media when on an inactive tab
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: MediaAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, philipj, sergio
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch eric.carlson: review+

Description Brent Fulgham 2014-04-23 14:23:56 PDT
We shouldn't waste bandwidth and energy downloading and decoding media if the user isn't playing the media, and has switched to a different tab.
Comment 1 Brent Fulgham 2014-04-23 14:24:36 PDT
<rdar://problem/15349921>
Comment 2 Brent Fulgham 2014-04-23 14:33:12 PDT
Created attachment 230011 [details]
Patch
Comment 3 Eric Carlson 2014-04-23 15:03:50 PDT
Comment on attachment 230011 [details]
Patch

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

> Source/WebCore/platform/audio/MediaSession.h:105
> +    bool m_elementIsHidden;

This is never initialized, but I don't know how it can be. Maybe it makes more sense to not cache this state but to always ask the client?
Comment 4 Brent Fulgham 2014-04-23 16:19:45 PDT
Created attachment 230018 [details]
Patch
Comment 5 Eric Carlson 2014-04-23 16:30:27 PDT
Comment on attachment 230018 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:6147
> +void HTMLMediaElement::shouldBufferData(bool shouldBuffer)

I should have noticed this before, but "shouldBufferData" sounds like it is a getter. Maybe "setShouldBufferData"?

> Source/WebCore/html/HTMLMediaElement.cpp:6150
> +        return m_player->shouldBufferData(shouldBuffer);

Ditto.

> Source/WebCore/platform/audio/MediaSession.cpp:64
> +    , m_elementIsHidden(false)

This isn't used.

> Source/WebCore/platform/audio/MediaSession.cpp:111
> +    clientShouldBufferDataIfNotHiddenOrPlaying();

Nit: this name is a mouthful!

> Source/WebCore/platform/audio/MediaSession.cpp:175
> +void MediaSession::clientShouldBufferDataIfNotHiddenOrPlaying()
> +{
> +    if (m_client.elementIsHidden() && m_state != Playing)
> +        m_client.shouldBufferData(false);
> +    else
> +        m_client.shouldBufferData(true);
> +}

I think part of the reason the name is so long is because it is used by the client and internally by the session. I would prefer something like "visibilityChanged()" for the client and "updateClientDataBuffering" for use within the session.
Comment 6 Brent Fulgham 2014-04-23 17:10:13 PDT
Created attachment 230024 [details]
Patch
Comment 7 Brent Fulgham 2014-04-23 17:17:02 PDT
(In reply to comment #5)
> (From update of attachment 230018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230018&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:6147
> > +void HTMLMediaElement::shouldBufferData(bool shouldBuffer)
> 
> I should have noticed this before, but "shouldBufferData" sounds like it is a getter. Maybe "setShouldBufferData"?
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:6150
> > +        return m_player->shouldBufferData(shouldBuffer);
> 
> Ditto.
> 
> > Source/WebCore/platform/audio/MediaSession.cpp:64
> > +    , m_elementIsHidden(false)
> 
> This isn't used.
> 
> > Source/WebCore/platform/audio/MediaSession.cpp:111
> > +    clientShouldBufferDataIfNotHiddenOrPlaying();
> 
> Nit: this name is a mouthful!
> 
> > Source/WebCore/platform/audio/MediaSession.cpp:175
> > +void MediaSession::clientShouldBufferDataIfNotHiddenOrPlaying()
> > +{
> > +    if (m_client.elementIsHidden() && m_state != Playing)
> > +        m_client.shouldBufferData(false);
> > +    else
> > +        m_client.shouldBufferData(true);
> > +}
> 
> I think part of the reason the name is so long is because it is used by the client and internally by the session. I would prefer something like "visibilityChanged()" for the client and "updateClientDataBuffering" for use within the session.

Agreed. I've changed it.
Comment 8 Eric Carlson 2014-04-23 18:08:56 PDT
Comment on attachment 230024 [details]
Patch

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

Thanks!

> Source/WebCore/platform/audio/MediaSession.h:130
> +#if ENABLE(PAGE_VISIBILITY_API)
> +    virtual bool elementIsHidden() const { return false; }
> +#endif

I don't think this is necessary.
Comment 9 Brent Fulgham 2014-04-23 18:24:26 PDT
Committed r167734: <http://trac.webkit.org/changeset/167734>