[webkit-reviews] review denied: [Bug 191004] [GTK] Layout test media/track/track-cue-missing.html is failing : [Attachment 427350] Patch

[bugzilla-daemon at webkit.org]

Alicia Boya García <aboya at igalia.com> has denied Enrique Ocaña
<eocanha at igalia.com>'s request for review:
Bug 191004: [GTK] Layout test media/track/track-cue-missing.html is failing
https://bugs.webkit.org/show_bug.cgi?id=191004

Attachment 427350: Patch

https://bugs.webkit.org/attachment.cgi?id=427350&action=review




--- Comment #3 from Alicia Boya García <aboya at igalia.com> ---
Comment on attachment 427350
  --> https://bugs.webkit.org/attachment.cgi?id=427350
Patch

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

Good job. Frame accuracy is something we need to improve in WebKitGTK.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2849
> +	   RunLoop::main().dispatchAfter(Seconds((time-currentTime).toDouble()
/ double(m_playbackRate)), WTFMove(task));

This is not robust enough. This breaks if the video is paused or a seek occurs.
This is also vulnerable to slight timing differences. Given that a common
application of this API is subtitles, I can imagine how this could cause frames
being shown with the wrong subtitle for a fraction of a second.

Instead, to get a more robust implementation I suggest taking advantage of
MediaPlayerPrivateGStreamer::triggerRepaint(), since that will give you frame
accuracy. You can get the stream time of the PTS of each sample, and if that
time is >= the time previously saved in this function, run the callback.

Looking at the implementation in AVFoundation I see there is no need to have
more than one callback at a time, so you can always clear the previous one. See
performTaskAtMediaTime() in
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm for reference.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:187
> +    bool performTaskAtMediaTime(Function<void()>&&, const MediaTime&);

Please add the `override` keyword to indicate that this method comes from
MediaPlayerPrivateInterface, as opposed to something internal of the GStreamer
backend.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:409
> +    MediaTime playbackPosition(bool useCached = true) const;

Using an enum would be better for readability.