Bug 233146

Summary:

HTMLMediaElement: Adopt reportExtraMemoryVisited/Allocated

Product:

WebKit

Reporter:

Jer Noble <jer.noble>

Component:

Media

Assignee:

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:

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-	Details Formatted Diff Diff
Patch (36.62 KB, patch) 2021-11-17 21:40 PST, Jer Noble	no flags	Details Formatted Diff Diff
Patch (36.73 KB, patch) 2021-11-18 09:32 PST, Jer Noble	no flags	Details Formatted Diff Diff
Patch (36.73 KB, patch) 2021-11-18 12:59 PST, Jer Noble	jer.noble: review?	Details Formatted Diff Diff
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.