Bug 233146

Summary: HTMLMediaElement: Adopt reportExtraMemoryVisited/Allocated
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: MediaAssignee: Jer Noble <jer.noble>
Status: NEW ---    
Severity: Normal CC: calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, jean-yves.avenard, kondapallykalyan, peng.liu6, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch jer.noble: review?

Description Jer Noble 2021-11-15 13:57:06 PST
HTMLMediaElement: adopt Adopt reportExtraMemoryVisited/Allocated
Comment 1 Radar WebKit Bug Importer 2021-11-17 20:44:06 PST
<rdar://problem/85533233>
Comment 2 Jer Noble 2021-11-17 21:22:25 PST
Created attachment 444637 [details]
Patch
Comment 3 Jer Noble 2021-11-17 21:40:12 PST
Created attachment 444638 [details]
Patch
Comment 4 Peng Liu 2021-11-17 21:56:34 PST
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?
Comment 5 Jean-Yves Avenard [:jya] 2021-11-18 02:51:41 PST
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
Comment 6 Jer Noble 2021-11-18 09:03:13 PST
(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.
Comment 7 Jer Noble 2021-11-18 09:07:28 PST
(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().
Comment 8 Jer Noble 2021-11-18 09:32:11 PST
Created attachment 444693 [details]
Patch
Comment 9 Jer Noble 2021-11-18 12:59:27 PST
Created attachment 444722 [details]
Patch