Bug 132077 – [Mac, iOS] Stop buffering media when on an inactive tab

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/15349921>

[Brent Fulgham]

Created attachment 230011 [details]
Patch

[Eric Carlson]

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?

[Brent Fulgham]

Created attachment 230018 [details]
Patch

[Eric Carlson]

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.

[Brent Fulgham]

Created attachment 230024 [details]
Patch

[Brent Fulgham]

(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.

[Eric Carlson]

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.

[Brent Fulgham]

Committed r167734: <http://trac.webkit.org/changeset/167734>