Bug 208080 - Support in-band generic cues when loading media in the GPU Process
Summary: Support in-band generic cues when loading media in the GPU Process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-21 16:34 PST by Eric Carlson
Modified: 2020-02-28 04:55 PST (History)
23 users (show)

See Also:


Attachments
Patch (60.53 KB, patch)
2020-02-21 16:38 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (60.53 KB, patch)
2020-02-21 18:53 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (61.98 KB, patch)
2020-02-21 19:49 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (63.20 KB, patch)
2020-02-27 04:16 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (63.23 KB, patch)
2020-02-27 05:49 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (81.19 KB, patch)
2020-02-27 17:40 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2020-02-21 16:34:23 PST
Support in-band generic cues when loading media in the GPU Process
Comment 1 Radar WebKit Bug Importer 2020-02-21 16:34:36 PST
<rdar://problem/59687943>
Comment 2 Eric Carlson 2020-02-21 16:38:38 PST
Created attachment 391433 [details]
Patch
Comment 3 Eric Carlson 2020-02-21 18:53:11 PST
Created attachment 391442 [details]
Patch
Comment 4 Eric Carlson 2020-02-21 19:49:11 PST
Created attachment 391446 [details]
Patch
Comment 5 Jer Noble 2020-02-22 10:29:24 PST
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?
Comment 6 Eric Carlson 2020-02-27 04:16:54 PST
Created attachment 391848 [details]
Patch
Comment 7 Eric Carlson 2020-02-27 05:49:31 PST
Created attachment 391851 [details]
Patch
Comment 8 youenn fablet 2020-02-27 08:37:19 PST
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 9 Eric Carlson 2020-02-27 17:39:15 PST
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.
Comment 10 Eric Carlson 2020-02-27 17:40:26 PST
Created attachment 391946 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2020-02-27 19:06:05 PST
Comment on attachment 391946 [details]
Patch for landing

Clearing flags on attachment: 391946

Committed r257612: <https://trac.webkit.org/changeset/257612>
Comment 12 WebKit Commit Bot 2020-02-27 19:06:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Aakash Jain 2020-02-28 04:55:43 PST
> 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