Support in-band generic cues when loading media in the GPU Process
<rdar://problem/59687943>
Created attachment 391433 [details] Patch
Created attachment 391442 [details] Patch
Created attachment 391446 [details] Patch
Comment on attachment 391446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391446&action=review > Source/WebCore/html/track/InbandGenericTextTrack.cpp:88 > -TextTrackCueGeneric* GenericTextTrackCueMap::find(GenericCueData& cueData) > +TextTrackCueGeneric* GenericTextTrackCueMap::find(InbandGenericCue& cueData) > { > - return m_dataToCueMap.get(&cueData); > + auto index = findIndexOfPair(cueData); > + if (index == notFound) > + return nullptr; > + > + return m_cues[index].m_cue.get(); > } > > -GenericCueData* GenericTextTrackCueMap::find(TextTrackCue& cue) > +size_t GenericTextTrackCueMap::findIndexOfPair(TextTrackCue& cue) > { > - return m_cueToDataMap.get(&cue); > + return m_cues.findMatching([&cue](const auto& cuePair) { > + if (!cue.hasEquivalentStartTime(*cuePair.m_cue)) > + return false; > + > + return cue.isEqual(*cuePair.m_cue, TextTrackCue::IgnoreDuration); > + }); > +} > + > +InbandGenericCue* GenericTextTrackCueMap::find(TextTrackCue& cue) > +{ > + auto index = findIndexOfPair(cue); > + if (index == notFound) > + return nullptr; > + > + return m_cues[index].m_cueData.get(); These methods seem to now be much slower, doing a long walk of the entire vector. And a long movie could have thousands of cues. Is there a way to speed this up? I see that the cues are inserted in startTime order; could we do a binary search here?
Created attachment 391848 [details] Patch
Created attachment 391851 [details] Patch
Comment on attachment 391851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391851&action=review > Source/WebCore/html/track/InbandGenericTextTrack.cpp:49 > +TextTrackCueGeneric* GenericTextTrackCueMap::find(InbandGenericCue& inbandCue) We could pass for all these methods the cue ID instead of InbandGenericCue object. > Source/WebCore/html/track/InbandGenericTextTrack.h:46 > + using CueToDataMap = HashMap<RefPtr<TextTrackCue>, uint64_t>; Can we use ObjectIdentifier? > Source/WebCore/html/track/InbandGenericTextTrack.h:47 > + using CueDataToCueMap = HashMap<uint64_t, RefPtr<TextTrackCueGeneric>>; Ditto. Also, do we need to keep ownership of the keys? Should it be TextTrackCue*? > Source/WebCore/platform/graphics/InbandGenericCue.cpp:79 > + switch (m_cueData.m_align) { Can this switch be useful elsewhere? Should it be made as a helper function? > Source/WebCore/platform/graphics/InbandGenericCue.cpp:111 > + if (!m_cueData.m_fontName.isEmpty()) empty or null? > Source/WebCore/platform/graphics/InbandGenericCue.cpp:144 > + return true; Would be more compact as return m_relativeFontSize == other.m_relativeFontSize && m_.... == ... > Source/WebCore/platform/graphics/InbandGenericCue.h:93 > + return WTF::nullopt; Should we serialise/deserialize invalid IDs? > Source/WebCore/platform/graphics/InbandGenericCue.h:228 > + explicit InbandGenericCue(); No need for explicit. Constructor as private would be better. > Source/WebCore/platform/graphics/InbandGenericCue.h:284 > + static uint64_t s_nextUniqueId; Needed? ObjectIdentifier as well > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:87 > +void InbandTextTrackPrivateAVF::processCueAttributes(CFAttributedStringRef attributedString, InbandGenericCue& cueData) It seems processCueAttributes is called in one place where cureData is directly created. It seems we could then change it to something like Ref<InbandGenericCue> InbandTextTrackPrivateAVF::processCueAttributes(CFAttributedStringRef) Or maybe RefPtr< InbandGenericCue > if there are error cases. > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:99 > + Vector<RefPtr<InbandGenericCue>> m_cues; Vector<Ref<>>? > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:531 > + if (const auto& track = m_textTracks.get(identifier)) s/const// Seems strange that a const auto& track could be calling addGenericCue. Ditto below. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:532 > + track->addGenericCue(WTFMove(InbandGenericCue::create(WTFMove(cueData)).get())); Maybe addGenericCue should take a Ref<>. WTFMove not needed probably. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:540 > + track->updateGenericCue(WTFMove(InbandGenericCue::create(WTFMove(cueData)).get())); WTFMove not needed probably. > Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:548 > + track->removeGenericCue(WTFMove(InbandGenericCue::create(WTFMove(cueData)).get())); WTFMove not needed probably. > Source/WebKit/WebProcess/GPU/media/TextTrackPrivateRemote.cpp:91 > +void TextTrackPrivateRemote::addGenericCue(InbandGenericCue&& cue) Do we really needed a &&? Since it is refcounted, I would expect to either pass a Ref<>&& or a InbandGenericCue&. Ditto for the others. > LayoutTests/gpu-process/TestExpectations:240 > +media/track/track-in-band-metadata-display-order.html [ Pass ] Nice!
Comment on attachment 391851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391851&action=review >> Source/WebCore/html/track/InbandGenericTextTrack.cpp:49 >> +TextTrackCueGeneric* GenericTextTrackCueMap::find(InbandGenericCue& inbandCue) > > We could pass for all these methods the cue ID instead of InbandGenericCue object. Good idea, fixed. >> Source/WebCore/html/track/InbandGenericTextTrack.h:46 >> + using CueToDataMap = HashMap<RefPtr<TextTrackCue>, uint64_t>; > > Can we use ObjectIdentifier? Good idea, fixed. >> Source/WebCore/html/track/InbandGenericTextTrack.h:47 >> + using CueDataToCueMap = HashMap<uint64_t, RefPtr<TextTrackCueGeneric>>; > > Ditto. > > Also, do we need to keep ownership of the keys? > Should it be TextTrackCue*? Fixed. >> Source/WebCore/platform/graphics/InbandGenericCue.cpp:79 >> + switch (m_cueData.m_align) { > > Can this switch be useful elsewhere? > Should it be made as a helper function? This is the only place it is used so far. >> Source/WebCore/platform/graphics/InbandGenericCue.cpp:111 >> + if (!m_cueData.m_fontName.isEmpty()) > > empty or null? .isEmpty() checks for NULL and empty: bool isEmpty() const { return !m_impl || m_impl->isEmpty(); } >> Source/WebCore/platform/graphics/InbandGenericCue.cpp:144 >> + return true; > > Would be more compact as > return m_relativeFontSize == other.m_relativeFontSize > && m_.... == ... Fixed. >> Source/WebCore/platform/graphics/InbandGenericCue.h:93 >> + return WTF::nullopt; > > Should we serialise/deserialize invalid IDs? I changed it to an ObjectIdentifier, so we get whatever its behavior with invalid values is. >> Source/WebCore/platform/graphics/InbandGenericCue.h:228 >> + explicit InbandGenericCue(); > > No need for explicit. > Constructor as private would be better. Fixed. >> Source/WebCore/platform/graphics/InbandGenericCue.h:284 >> + static uint64_t s_nextUniqueId; > > Needed? ObjectIdentifier as well Not needed with ObjectIdentifier >> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:87 >> +void InbandTextTrackPrivateAVF::processCueAttributes(CFAttributedStringRef attributedString, InbandGenericCue& cueData) > > It seems processCueAttributes is called in one place where cureData is directly created. > It seems we could then change it to something like > Ref<InbandGenericCue> InbandTextTrackPrivateAVF::processCueAttributes(CFAttributedStringRef) > Or maybe RefPtr< InbandGenericCue > if there are error cases. Good idea, changed. >> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:99 >> + Vector<RefPtr<InbandGenericCue>> m_cues; > > Vector<Ref<>>? Fixed. >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:531 >> + if (const auto& track = m_textTracks.get(identifier)) > > s/const// > Seems strange that a const auto& track could be calling addGenericCue. > Ditto below. Fixed >> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:548 >> + track->removeGenericCue(WTFMove(InbandGenericCue::create(WTFMove(cueData)).get())); > > WTFMove not needed probably. All fixed.
Created attachment 391946 [details] Patch for landing
Comment on attachment 391946 [details] Patch for landing Clearing flags on attachment: 391946 Committed r257612: <https://trac.webkit.org/changeset/257612>
All reviewed patches have been landed. Closing bug.
> Committed r257612: <https://trac.webkit.org/changeset/257612> This seems to have broken Windows build. Tracked in https://bugs.webkit.org/show_bug.cgi?id=208372 EWS also indicated this failure: https://ews-build.webkit.org/#/builders/10/builds/8439