Bug 208322 - Improve some media code
Summary: Improve some media code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-27 09:05 PST by Darin Adler
Modified: 2020-03-06 08:09 PST (History)
18 users (show)

See Also:


Attachments
Patch (98.07 KB, patch)
2020-02-29 11:56 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (99.47 KB, patch)
2020-03-05 21:37 PST, Darin Adler
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-02-27 09:05:29 PST
Fir issues noticed in audit of media code
Comment 1 Darin Adler 2020-02-29 11:56:23 PST
Created attachment 392068 [details]
Patch
Comment 2 Darin Adler 2020-02-29 12:03:48 PST
Comment on attachment 392068 [details]
Patch

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

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:77
> +void MediaControlsHost::ref()

The alternative to having MediaControlsHost having its lifetime tied completely to the HTMLMediaElement and forwarding ref/deref calls would be to use a WeakPtr and then add null checks to every function. I think this is a better solution, but I suppose there is some risk of a reference cycle if I don’t understand the ownership relationships correctly. Like what can hold a reference to the media controls host object and is there a chance of a cycle.

> Source/WebCore/loader/TextTrackLoader.h:66
>      Vector<Ref<VTTCue>> getNewCues();
> -    void getNewRegions(Vector<RefPtr<VTTRegion>>& outputRegions);
> +    Vector<Ref<VTTRegion>> getNewRegions();
>      Vector<String> getNewStyleSheets();

We could also consider renaming these three functions to take or takeNew instead of getNew, since they transfer ownership to the caller.
Comment 3 Alex Christensen 2020-03-02 08:21:16 PST
Comment on attachment 392068 [details]
Patch

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

r=me except one problem.

> Source/WebCore/html/track/WebVTTParser.cpp:342
> +                m_regionList.remove(i);

This is a O(n^2) algorithm that mutates the collection while iterating, so it would miss regions right after the removed region.  removeAllMatching would be faster and more correct.
Comment 4 Eric Carlson 2020-03-02 09:24:33 PST
Comment on attachment 392068 [details]
Patch

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

>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:77
>> +void MediaControlsHost::ref()
> 
> The alternative to having MediaControlsHost having its lifetime tied completely to the HTMLMediaElement and forwarding ref/deref calls would be to use a WeakPtr and then add null checks to every function. I think this is a better solution, but I suppose there is some risk of a reference cycle if I don’t understand the ownership relationships correctly. Like what can hold a reference to the media controls host object and is there a chance of a cycle.

HTMLMediaElement creates MediaControlsHost and exposes it to the JavaScript that implements controls in the shadow DOM. The controls JS puts the MediaControlsHost into a global variable. 

HTMLMediaElement owns the JS, and the JS now refs HTMLMediaElement, so doesn't this create a retain cycle?
Comment 5 Darin Adler 2020-03-02 10:01:31 PST
Comment on attachment 392068 [details]
Patch

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

>> Source/WebCore/html/track/WebVTTParser.cpp:342
>> +                m_regionList.remove(i);
> 
> This is a O(n^2) algorithm that mutates the collection while iterating, so it would miss regions right after the removed region.  removeAllMatching would be faster and more correct.

Agreed that it’s O(n^2), but note that this breaks out of the loop when it finds a region that matches; so there are no "missed regions" because this removes only the first. All I did here was get rid of the second unnecessary loop over the vector inside the removeFirst function. While it’s OK for someone to change this to removeAllMatching I don’t think that change is necessary as part of this patch. I also don’t think removeAllMatching would be significantly faster.
Comment 6 Darin Adler 2020-03-02 10:03:24 PST
Comment on attachment 392068 [details]
Patch

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

>>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:77
>>> +void MediaControlsHost::ref()
>> 
>> The alternative to having MediaControlsHost having its lifetime tied completely to the HTMLMediaElement and forwarding ref/deref calls would be to use a WeakPtr and then add null checks to every function. I think this is a better solution, but I suppose there is some risk of a reference cycle if I don’t understand the ownership relationships correctly. Like what can hold a reference to the media controls host object and is there a chance of a cycle.
> 
> HTMLMediaElement creates MediaControlsHost and exposes it to the JavaScript that implements controls in the shadow DOM. The controls JS puts the MediaControlsHost into a global variable. 
> 
> HTMLMediaElement owns the JS, and the JS now refs HTMLMediaElement, so doesn't this create a retain cycle?

Sounds like it probably does create a retain cycle. I need to find some other solution, because the old code has a potentially dangling pointer that I can’t just leave as-is. I suppose I could use a WeakPtr and add a null check to every use of m_mediaElement. Any other ideas about how to handle this back-pointer?
Comment 7 Alex Christensen 2020-03-02 11:07:02 PST
Comment on attachment 392068 [details]
Patch

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

>>>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:77
>>>> +void MediaControlsHost::ref()
>>> 
>>> The alternative to having MediaControlsHost having its lifetime tied completely to the HTMLMediaElement and forwarding ref/deref calls would be to use a WeakPtr and then add null checks to every function. I think this is a better solution, but I suppose there is some risk of a reference cycle if I don’t understand the ownership relationships correctly. Like what can hold a reference to the media controls host object and is there a chance of a cycle.
>> 
>> HTMLMediaElement creates MediaControlsHost and exposes it to the JavaScript that implements controls in the shadow DOM. The controls JS puts the MediaControlsHost into a global variable. 
>> 
>> HTMLMediaElement owns the JS, and the JS now refs HTMLMediaElement, so doesn't this create a retain cycle?
> 
> Sounds like it probably does create a retain cycle. I need to find some other solution, because the old code has a potentially dangling pointer that I can’t just leave as-is. I suppose I could use a WeakPtr and add a null check to every use of m_mediaElement. Any other ideas about how to handle this back-pointer?

Sounds like this needs a WeakPtr and a bunch of null checks then.

>>> Source/WebCore/html/track/WebVTTParser.cpp:342
>>> +                m_regionList.remove(i);
>> 
>> This is a O(n^2) algorithm that mutates the collection while iterating, so it would miss regions right after the removed region.  removeAllMatching would be faster and more correct.
> 
> Agreed that it’s O(n^2), but note that this breaks out of the loop when it finds a region that matches; so there are no "missed regions" because this removes only the first. All I did here was get rid of the second unnecessary loop over the vector inside the removeFirst function. While it’s OK for someone to change this to removeAllMatching I don’t think that change is necessary as part of this patch. I also don’t think removeAllMatching would be significantly faster.

Ah, I didn't read all the way to the break. This is fine.
Comment 8 Darin Adler 2020-03-05 21:37:36 PST
Created attachment 392673 [details]
Patch
Comment 9 Darin Adler 2020-03-05 21:38:01 PST
OK, new version up for review uses WeakPtr.
Comment 10 Darin Adler 2020-03-06 08:08:39 PST
Committed r257997: <https://trac.webkit.org/changeset/257997>
Comment 11 Radar WebKit Bug Importer 2020-03-06 08:09:20 PST
<rdar://problem/60151665>