| Summary: | Improve some media code | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||
| Component: | Media | Assignee: | Darin Adler <darin> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | achristensen, andersca, benjamin, calvaris, cdumez, cmarcelo, eric.carlson, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, japhet, jer.noble, kondapallykalyan, philipj, sergio, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Darin Adler
2020-02-27 09:05:29 PST
Created attachment 392068 [details]
Patch
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 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 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 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 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 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. Created attachment 392673 [details]
Patch
OK, new version up for review uses WeakPtr. Committed r257997: <https://trac.webkit.org/changeset/257997> |