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?

Jer Noble
Reported 2021-11-15 13:57:06 PST
HTMLMediaElement: adopt Adopt reportExtraMemoryVisited/Allocated
Attachments
Patch (36.62 KB, patch)
2021-11-17 21:22 PST, Jer Noble 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (36.62 KB, patch)
2021-11-17 21:40 PST, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch (36.73 KB, patch)
2021-11-18 09:32 PST, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch (36.73 KB, patch)
2021-11-18 12:59 PST, Jer Noble 		jer.noble: review?
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2021-11-17 20:44:06 PST
<rdar://problem/85533233>
Jer Noble
Comment 2 2021-11-17 21:22:25 PST
Created attachment 444637 [details] Patch
Jer Noble
Comment 3 2021-11-17 21:40:12 PST
Created attachment 444638 [details] Patch
Peng Liu
Comment 4 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?
Jean-Yves Avenard [:jya]
Comment 5 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
Jer Noble
Comment 6 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.
Jer Noble
Comment 7 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().
Jer Noble
Comment 8 2021-11-18 09:32:11 PST
Created attachment 444693 [details] Patch
Jer Noble
Comment 9 2021-11-18 12:59:27 PST
Created attachment 444722 [details] Patch
Note You need to log in before you can comment on or make changes to this bug.