[webkit-reviews] review denied: [Bug 174817] [GStreamer] Refactor media player to use MediaTime consistently : [Attachment 321811] 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 321811: Patch

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




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

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

This looks better. Still, there are missing bits to rip floating point from our
code. An example is for example maxTimeSeekable that has been left unchanged on
the regular player. I'll continue in another comment.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:183
> +// Convert a MediaTime in seconds to a GstClockTime.
> +// Note that we can get MediaTime objects with a time scale
> +// that isn't a GST_SECOND, since they can come to us through
> +// the internal testing API, the DOM and internally. It would
> +// be nice to assert the format of the incoming time, but all
> +// the media APIs assume time is passed around in fractional
> +// seconds, so we'll just have to assume the same.

These lines can be longer.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:309
> -	   MediaTime mediaDuration = durationMediaTime();
> -	   if (mediaDuration)
> -	       return mediaDuration.toDouble();
> -	   return 0;
> +	   return durationMediaTime();

It seems in this code you're not honoring your comment:

> Your next comment made me aware of the bug I was introducing here. If
!durationMediaTime().isValid() I should be returning 0 here too.

It seems in some cases, durationMediaTime can be invalid.

Maybe you overlooked this or decided to leave it like this for some reason.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:872
> +	       timeRanges->add(MediaTime(rangeStart *
toGstUnsigned64Time(mediaDuration) / GST_FORMAT_PERCENT_MAX, GST_SECOND),
> +		   MediaTime(rangeStop * toGstUnsigned64Time(mediaDuration) /
GST_FORMAT_PERCENT_MAX, GST_SECOND));

I think you don't need to leave MediaTime, perform the operation and go back to
MediaTime, you can do something like mediaDuration * (rangeStart /
GST_FORMAT_PERCENT_MAX), as MediaTime has an operator*(int32_t).

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1247
> +	       m_maxTimeLoaded = MediaTime(fillStatus *
static_cast<double>(toGstUnsigned64Time(mediaDuration)) / 100, GST_SECOND);

Use operator* here too.