Summary: | Report the memory cost of HTMLMediaElements to GC. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, darin, eric.carlson, esprehn+autocc, glenn | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Jer Noble
2013-06-13 14:08:32 PDT
Created attachment 204642 [details]
Patch
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. 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? (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! (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. (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. Committed r151609: <http://trac.webkit.org/changeset/151609> |