Bug 233146 - HTMLMediaElement: Adopt reportExtraMemoryVisited/Allocated
Summary: HTMLMediaElement: Adopt reportExtraMemoryVisited/Allocated
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-15 13:57 PST by Jer Noble
Modified: 2021-11-18 12:59 PST (History)
15 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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