Bug 117608 – Report the memory cost of HTMLMediaElements to GC.

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

Report the memory cost of HTMLMediaElements to GC.

[Jer Noble]

Created attachment 204642 [details]
Patch

[Darin Adler]

Comment on attachment 204642 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:622
> +            double extraMemoryCost = m_player->extraMemoryCost();
> +            double extraMemoryCostDelta = extraMemoryCost - m_reportedExtraMemoryCost;
> +            m_reportedExtraMemoryCost = extraMemoryCost;

Why the mix of double and size_t here? It seems like extraMemoryCost is returning size_t, so this should just be size_t.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:883
> +    return totalBytes() * buffered()->totalDuration() / duration;

Is there something that prevents this from overflowing a size_t?

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:269
> +    virtual size_t extraMemoryCost() const OVERRIDE;

Either this class or this function should be marked FINAL too.

[Eric Carlson]

Comment on attachment 204642 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:209
> +    virtual size_t extraMemoryCost() const { return 0; }

Most (all?) ports will want the simple implementation you added to MediaPlayerPrivateAVFoundation.cpp - why not put it here?

[Jer Noble]

(In reply to comment #2)
> (From update of attachment 204642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204642&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:622
> > +            double extraMemoryCost = m_player->extraMemoryCost();
> > +            double extraMemoryCostDelta = extraMemoryCost - m_reportedExtraMemoryCost;
> > +            m_reportedExtraMemoryCost = extraMemoryCost;
> 
> Why the mix of double and size_t here? It seems like extraMemoryCost is returning size_t, so this should just be size_t.

No good reason; these should indeed be size_t.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:883
> > +    return totalBytes() * buffered()->totalDuration() / duration;
> 
> Is there something that prevents this from overflowing a size_t?

Since totalBytes() is an unsigned, and (totalDuration() / duration) will be in the range [0..1], this can never overflow a size_t.

> > Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:269
> > +    virtual size_t extraMemoryCost() const OVERRIDE;
> 
> Either this class or this function should be marked FINAL too.

Sure thing.  Thanks!

[Jer Noble]

(In reply to comment #3)
> (From update of attachment 204642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204642&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayerPrivate.h:209
> > +    virtual size_t extraMemoryCost() const { return 0; }
> 
> Most (all?) ports will want the simple implementation you added to MediaPlayerPrivateAVFoundation.cpp - why not put it here?

I guess that makes sense.  If any port wants their own implementation, they can just override it.

[Jer Noble]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 204642 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=204642&action=review
> > 
> > > Source/WebCore/platform/graphics/MediaPlayerPrivate.h:209
> > > +    virtual size_t extraMemoryCost() const { return 0; }
> > 
> > Most (all?) ports will want the simple implementation you added to MediaPlayerPrivateAVFoundation.cpp - why not put it here?
> 
> I guess that makes sense.  If any port wants their own implementation, they can just override it.

Ah, it can't go into MediaPlayerPrivate because MediaPlayerPrivate does not have a "totalBytes()" method.

[Jer Noble]

Committed r151609: <http://trac.webkit.org/changeset/151609>