[webkit-reviews] review denied: [Bug 174817] [GStreamer] Refactor media player to use MediaTime consistently : [Attachment 321341] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Enrique Ocaña
<eocanha at igalia.com>'s request for review:
Bug 174817: [GStreamer] Refactor media player to use MediaTime consistently
https://bugs.webkit.org/show_bug.cgi?id=174817

Attachment 321341: Patch

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




--- Comment #13 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 321341
  --> https://bugs.webkit.org/attachment.cgi?id=321341
Patch

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

The patch is mainly ok, there are nits and some other things we are missing.
There are still some places where there are createWith{Float|Double} and we
need to get rid of those as well. And review also all float and double values
to ensure we are not leaving any float or double behind.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:322
> +    MediaTime playbackPosition = MediaTime::zeroTime();

Before, unless we used NaN for double there was no better way of specify an
invalid time. I doubt the spirit of the original like was to return 0
(beginning of playback) if we can't give the playback position. I think it
would be semantically better to write this line as invalidTime instead of
zeroTime. Please, check that tests still pass.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:537
>	   // If we are at beginning of media, start from the end to
>	   // avoid immediate EOS.
>	   if (position < 0)
> -	       endTime = static_cast<gint64>(durationMediaTime().toDouble() *
GST_SECOND);
> +	       endTime = toGstUnsigned64Time(durationMediaTime());
>	   else
>	       endTime = position;
>      }

I'd like to have MediaPlayerPrivateGStreamer::doSeek to get position as
MediaTime. That way you don't even need GstClockTime clockTime variable in
MediaPlayerPrivateGStreamer::seek.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:551
> +    uint64_t currentPosition = toGstUnsigned64Time(playbackPosition());

I guess modifying doSeek will affect updatePlaybackRate as well.

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp:191
> +    MediaTime prevSeekTime = m_seekTime;

Let's name this properly as previousSeekTime.

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp:250
>      GstClockTime position = toGstClockTime(m_seekTime);
> -    MediaTime seekTime = MediaTime::createWithDouble(m_seekTime);
> +    MediaTime seekTime = m_seekTime;

Regarding this, I have a question. We cache here seekTime and change it later,
if needed, for the nearest buffered time to avoid the minigaps but later you
use the cached position as GstClockTime for the seek. Is that right?

Btw, in this function you create the miniGap as double, please, create it as
fraction, 1/10, for example.