Bug 208080 – Support in-band generic cues when loading media in the GPU Process

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Support in-band generic cues when loading media in the GPU Process

[Radar WebKit Bug Importer]

<rdar://problem/59687943>

[Eric Carlson]

Created attachment 391433 [details]
Patch

[Eric Carlson]

Created attachment 391442 [details]
Patch

[Eric Carlson]

Created attachment 391446 [details]
Patch

[Jer Noble]

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?

[Eric Carlson]

Created attachment 391848 [details]
Patch

[Eric Carlson]

Created attachment 391851 [details]
Patch

[youenn fablet]

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!

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 391946 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 391946 [details]
Patch for landing

Clearing flags on attachment: 391946

Committed r257612: <https://trac.webkit.org/changeset/257612>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Aakash Jain]

> 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