Bug 233146 – HTMLMediaElement: Adopt reportExtraMemoryVisited/Allocated

• Status: NEW
• Component: Media

[Jer Noble]

HTMLMediaElement: adopt Adopt reportExtraMemoryVisited/Allocated

[Radar WebKit Bug Importer]

<rdar://problem/85533233>

[Jer Noble]

Created attachment 444637 [details]
Patch

[Jer Noble]

Created attachment 444638 [details]
Patch

[Peng Liu]

Comment on attachment 444638 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1001
> +    if (m_private && !m_private->supportsMemoryCostChanged())

When m_private does not support MemoryCostChanged?

[Jean-Yves Avenard [:jya]]

Comment on attachment 444638 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:866
> +    if (newMemoryCost == m_reportedExtraMemoryCost)

on which thread could this be running that makes a requirement that m_reportedExtraMemoruCost be atomic?

Could a note be added on the member declaration explaining on why this requirement?

It would be ideal if this method had an assert like Assert(isOnMainthread() || isOnTheThreadWeExpect())
unfortunately, there's very little infrastructure for that

[Jer Noble]

(In reply to Peng Liu from comment #4)
> Comment on attachment 444638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444638&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:1001
> > +    if (m_private && !m_private->supportsMemoryCostChanged())
> 
> When m_private does not support MemoryCostChanged?

Every MediaPlayerPrivate that isn't MediaPlayerPrivateAVFoundation just uses the default implementation of memoryCost(), and never explicitly pushes memory cost information upwards.

[Jer Noble]

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> Comment on attachment 444638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444638&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:866
> > +    if (newMemoryCost == m_reportedExtraMemoryCost)
> 
> on which thread could this be running that makes a requirement that
> m_reportedExtraMemoruCost be atomic?

memoryCost() can be called from a GC thread, through the JSHTMLMediaElement wrapper object, by JSC.

> Could a note be added on the member declaration explaining on why this
> requirement?

I'll add a note in ::memoryCost().

[Jer Noble]

Created attachment 444693 [details]
Patch

[Jer Noble]

Created attachment 444722 [details]
Patch