Bug 174817 – [GStreamer] Refactor media player to use MediaTime consistently

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Charlie Turner]

WebCore moved to using a rational class for working with media positions called MediaTime a while back, I suspect because Apple's media backend uses a rational class internally. GStreamer just uses a 64 bit integer for timestamps so we don't benefit from much of the information/safety MediaTime supports. However, the existing code was a bit weird in that there were many places of curiosity that were converting between floats and doubles into MediaTimes just to support the new APIs, which was making parts of the code quite hard to read.

This patch makes more consistent use of MediaTime and cleans up these areas.

[Charlie Turner]

Created attachment 316365 [details]
Patch

[Charlie Turner]

Created attachment 316366 [details]
Patch

[Charlie Turner]

Created attachment 316763 [details]
Patch

Rebasing my patch over current master

[Build Bot]

Comment on attachment 316763 [details]
Patch

Attachment 316763 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4228666

New failing tests:
fast/images/large-size-image-crash.html

[Build Bot]

Created attachment 316767 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Charlie Turner]

Adding some more potential reviewers.

[Xabier Rodríguez Calvar]

Comment on attachment 316763 [details]
Patch

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

I really love this patch happening. Using MediaTime should save us from many nasty rounding bugs in the long term (though it is not guaranteed that we won't introduce some with this patch).

> Source/WebCore/ChangeLog:41
> +        * platform/graphics/gstreamer/GStreamerUtilities.cpp:
> +        (WebCore::toGstClockTime):
> +        * platform/graphics/gstreamer/GStreamerUtilities.h:
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
> +        (WebCore::MediaPlayerPrivateGStreamer::MediaPlayerPrivateGStreamer):
> +        (WebCore::MediaPlayerPrivateGStreamer::load):
> +        (WebCore::MediaPlayerPrivateGStreamer::playbackPosition):
> +        (WebCore::MediaPlayerPrivateGStreamer::durationMediaTime):
> +        (WebCore::MediaPlayerPrivateGStreamer::currentMediaTime):
> +        (WebCore::MediaPlayerPrivateGStreamer::seek):
> +        (WebCore::MediaPlayerPrivateGStreamer::doSeek):
> +        (WebCore::MediaPlayerPrivateGStreamer::updatePlaybackRate):
> +        (WebCore::MediaPlayerPrivateGStreamer::asyncStateChangeDone):
> +        (WebCore::MediaPlayerPrivateGStreamer::updateStates):
> +        (WebCore::MediaPlayerPrivateGStreamer::didEnd):
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:
> +        * platform/graphics/gstreamer/mse/GStreamerMediaSample.cpp:
> +        (WebCore::GStreamerMediaSample::offsetTimestampsBy):
> +        * platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::seek):
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::notifySeekNeedsDataForTime):
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::doSeek):
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::isTimeBuffered):
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::currentMediaTime):
> +        * platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:
> +        * platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:
> +        (WebCore::PlaybackPipeline::enqueueSample):
> +        * platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:
> +        (webKitMediaSrcQueryWithParent):

You might be more verbose here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:323
> +    GstClockTime positionGst = static_cast<GstClockTime>(position);

gstreamerPosition

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:432
> +    bool failure = !gst_element_query_duration(m_pipeline.get(), timeFormat, &timeLength);
> +    failure |= !GST_CLOCK_TIME_IS_VALID(timeLength);

Please keep this in one line.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:543
> +            endTime = durationMediaTime().timeValue();

In these kind of cases, you might end up getting invalid times or maybe (I don't know if it's possible or not but unless you're 100% sure we should follow the safe path) times with a scale that is not GST_SECOND, meaning that timeValue could return something that is not nanoseconds. We need to ensure that and here, I think we should use toGstClockTime. Maybe it would be interesting to rename the functions at utilities to have toGstUnsigned64Time and inline toGstClockTime to call the former and static_cast.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:560
> +    int64_t currentPosition = playbackPosition().timeValue();

ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:575
> -    GST_INFO("Need to mute audio?: %d", (int) mute);
> +    GST_INFO(mute ? "Need to mute audio" : "Do not need to mute audio");

No strong opinion about this specific case but I landed yesterday a patch to help with this kind of debug messages. Very dumb function WTF::boolForPrinting that takes a bool and converts it to "true" or "false". This could end up as:
GST_INFO("Need to mute audio: %s", boolForPrinting(mute));
But if you think it is better this way, be my guest to leave it like this.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:200
> +    GST_DEBUG("m_seeking=%s, m_seekTime=%s", m_seeking ? "true" : "false", toString(m_seekTime).utf8().data());

Keep boolForPrinting

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:669
> +    GST_DEBUG("Time %s buffered? %s", toString(time).utf8().data(), result ? "Yes" : "No");

Use boolForPrinting.

[Charlie Turner]

Comment on attachment 316763 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:432
>> +    failure |= !GST_CLOCK_TIME_IS_VALID(timeLength);
> 
> Please keep this in one line.

I really object to code that relies on the order-of-eval rules in C++ like this. It turns out this is technically valid by the rules (exception 6 out of 21 for the "order of evaluation is undefined" rule in C++...), but having the timeLength check depend on the previous side-effect just does not compute for me.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:543
>> +            endTime = durationMediaTime().timeValue();
> 
> In these kind of cases, you might end up getting invalid times or maybe (I don't know if it's possible or not but unless you're 100% sure we should follow the safe path) times with a scale that is not GST_SECOND, meaning that timeValue could return something that is not nanoseconds. We need to ensure that and here, I think we should use toGstClockTime. Maybe it would be interesting to rename the functions at utilities to have toGstUnsigned64Time and inline toGstClockTime to call the former and static_cast.

There is the assumption in this code that all values are scaled to GST_SECOND and sent around in seconds.microseconds format. Nothing really enforces that, and it I suppose switching to the utilities and asserting the time scale is in the right format might be the best approach to avoiding future mistakes.

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #8)
> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:432
> >> +    failure |= !GST_CLOCK_TIME_IS_VALID(timeLength);
> > 
> > Please keep this in one line.
> 
> I really object to code that relies on the order-of-eval rules in C++ like
> this. It turns out this is technically valid by the rules (exception 6 out
> of 21 for the "order of evaluation is undefined" rule in C++...), but having
> the timeLength check depend on the previous side-effect just does not
> compute for me.

Spec is clear here and with your case you're always evaluating the second clause because there is no shortcircuit, which is unnecessary.

> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:543
> >> +            endTime = durationMediaTime().timeValue();
> > 
> > In these kind of cases, you might end up getting invalid times or maybe (I don't know if it's possible or not but unless you're 100% sure we should follow the safe path) times with a scale that is not GST_SECOND, meaning that timeValue could return something that is not nanoseconds. We need to ensure that and here, I think we should use toGstClockTime. Maybe it would be interesting to rename the functions at utilities to have toGstUnsigned64Time and inline toGstClockTime to call the former and static_cast.
> 
> There is the assumption in this code that all values are scaled to
> GST_SECOND and sent around in seconds.microseconds format. Nothing really
> enforces that, and it I suppose switching to the utilities and asserting the
> time scale is in the right format might be the best approach to avoiding
> future mistakes.

Yep. I think things would be less under control if we don't do this, possibly leading to weird timing errors.

[Charlie Turner]

Created attachment 317707 [details]
Patch

It turned out after investigating that assertion that it does not make sense to check for GST_SECOND, because we are not the only creators of media times, but we can assume (and Apple ports do also) that the time is always fractional seconds. Addressed other review comments.

[Xabier Rodríguez Calvar]

Comment on attachment 317707 [details]
Patch

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

> Source/WebCore/ChangeLog:41
> +        Make consistent use of the MediaTime class in the GStreamer media
> +        player implementations.
> +
> +        Covered by existing tests.
> +
> +        * platform/graphics/gstreamer/GStreamerUtilities.cpp:
> +        (WebCore::toGstClockTime):
> +        * platform/graphics/gstreamer/GStreamerUtilities.h:
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
> +        (WebCore::MediaPlayerPrivateGStreamer::MediaPlayerPrivateGStreamer):
> +        (WebCore::MediaPlayerPrivateGStreamer::load):
> +        (WebCore::MediaPlayerPrivateGStreamer::playbackPosition):
> +        (WebCore::MediaPlayerPrivateGStreamer::durationMediaTime):
> +        (WebCore::MediaPlayerPrivateGStreamer::currentMediaTime):
> +        (WebCore::MediaPlayerPrivateGStreamer::seek):
> +        (WebCore::MediaPlayerPrivateGStreamer::doSeek):
> +        (WebCore::MediaPlayerPrivateGStreamer::updatePlaybackRate):
> +        (WebCore::MediaPlayerPrivateGStreamer::asyncStateChangeDone):
> +        (WebCore::MediaPlayerPrivateGStreamer::updateStates):
> +        (WebCore::MediaPlayerPrivateGStreamer::didEnd):
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:
> +        * platform/graphics/gstreamer/mse/GStreamerMediaSample.cpp:
> +        (WebCore::GStreamerMediaSample::offsetTimestampsBy):
> +        * platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::seek):
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::notifySeekNeedsDataForTime):
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::doSeek):
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::isTimeBuffered):
> +        (WebCore::MediaPlayerPrivateGStreamerMSE::currentMediaTime):
> +        * platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:
> +        * platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:
> +        (WebCore::PlaybackPipeline::enqueueSample):
> +        * platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:
> +        (webKitMediaSrcQueryWithParent):

You need to be more verbose here, either by commenting more in the general section or in the file specific one.

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:184
> +uint64_t toGstUnsigned64Time(const MediaTime &mediaTime)

Move the & next to the type

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:191
> +GstClockTime toGstClockTime(const MediaTime &mediaTime)

Ditto. And leave a space between the two functions. Or even better, make this inline in the header file.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:477
> +    time = std::min(time, durationMediaTime());

You can delay the creation of time until here because first you should be able to use mediaTime.

[Enrique Ocaña]

Created attachment 321341 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 321341 [details]
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.cpp:191
> +    MediaTime prevSeekTime = m_seekTime;

Let's name this properly as previousSeekTime.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp: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.

[Enrique Ocaña]

Comment on attachment 321341 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:310
> +        return durationMediaTime();

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

>> 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.

Yes, the spirit was to return 0 because that's what the spec says (and all the above layers rely on us to return 0). See comment on line 304 and https://www.w3.org/TR/html52/semantics-embedded-content.html#default-playback-start-position

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:250
>> +    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.

=:-O

It's true that the modified seekTime is being ignored and that's bad. Thanks a lot for spotting it!

[Xabier Rodríguez Calvar]

Comment on attachment 321341 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:250
>>> +    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.
> 
> =:-O
> 
> It's true that the modified seekTime is being ignored and that's bad. Thanks a lot for spotting it!

This patch is also probably a good moment to avoid having the position variable (you can probably the right value there where it's used).
Maybe you can also remove the seekTime variable and use the m_seekTime attribute, though I don't know if we want it replaced by its nearest buffered time when avoiding gaps (it is the only place where it's modified inside the method.
You can move rate to the first place it is used.
seekType can probably be removed as it is used only once and we can replace it by its right value.

Please, let me know if you find anything of this not recommended.

[Philippe Normand]

Is accurate seek still working with this patch? IIRC there is a layout test about this but I don't know if it's skipped and/or failing nowadays.

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #16)
> Is accurate seek still working with this patch? IIRC there is a layout test
> about this but I don't know if it's skipped and/or failing nowadays.

which part of the code are you worried about?

[Philippe Normand]

Comment on attachment 321341 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:-187
> -    // Extract the integer part of the time (seconds) and the fractional part (microseconds). Attempt to
> -    // round the microseconds so no floating point precision is lost and we can perform an accurate seek.
> -    float seconds;
> -    float microSeconds = modff(time, &seconds) * 1000000;
> -    GTimeVal timeValue;
> -    timeValue.tv_sec = static_cast<glong>(seconds);
> -    timeValue.tv_usec = static_cast<glong>(floor(microSeconds + 0.5));
> -    return GST_TIMEVAL_TO_TIME(timeValue);

This.

[Charlie Turner]

Comment on attachment 321341 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:-187
>> -    return GST_TIMEVAL_TO_TIME(timeValue);
> 
> This.

My understanding is that toTimeScale() does basically exactly the same thing, just safely. I noticed now regressions in the TS when I first made this change.

[Enrique Ocaña]

Created attachment 321809 [details]
Patch

[Enrique Ocaña]

Created attachment 321811 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 321811 [details]
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.

[Xabier Rodríguez Calvar]

At this point, if you check MediaPlayerPrivate.h you can see:

    virtual float duration() const { return 0; }
    virtual double durationDouble() const { return duration(); }
    virtual MediaTime durationMediaTime() const { return MediaTime::createWithDouble(durationDouble()); }

    virtual float currentTime() const { return 0; }
    virtual double currentTimeDouble() const { return currentTime(); }
    virtual MediaTime currentMediaTime() const { return MediaTime::createWithDouble(currentTimeDouble()); }

    virtual void seek(float) { }
    virtual void seekDouble(double time) { seek(time); }
    virtual void seek(const MediaTime& time) { seekDouble(time.toDouble()); }

    virtual float maxTimeSeekable() const { return 0; }
    virtual MediaTime maxMediaTimeSeekable() const { return MediaTime::createWithDouble(maxTimeSeekable()); }
    virtual double minTimeSeekable() const { return 0; }
    virtual MediaTime minMediaTimeSeekable() const { return MediaTime::createWithDouble(minTimeSeekable()); }

So we have functions taking and returning float, doubles and MediaTime. Preference is for floating point here.

I think we need to reimplement all these in Base.h and reverse them. Let's make them MediaTime based where the floating point based call toFloat/toDouble/createWithFloat/createWithDouble. Of course, inside our regular and and MSE player, we need to guarantee that we redefine only the non floating point ones and we use only those.

[Enrique Ocaña]

Comment on attachment 321811 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:872
>> +                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).

Unfortunately, (rangeStart / GST_FORMAT_PERCENT_MAX) will be a value ranging from 0 to 1, unsuitable for an integer type without loss of precision.

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

Same counterargument as before: If I convert fillStatus (range 0..100) to int32_t, the loss of precision would be too much. In the best case, I could multiply mediaDuration * (int32_t)((double)fillStatus * GST_FORMAT_PERCENT_SCALE) * MediaTime(1, GST_FORMAT_PERCENT_SCALE).

GST_FORMAT_PERCENT_SCALE=10000

[Enrique Ocaña]

Comment on attachment 321811 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1247
>>> +            m_maxTimeLoaded = MediaTime(fillStatus * static_cast<double>(toGstUnsigned64Time(mediaDuration)) / 100, GST_SECOND);
>> 
>> Use operator* here too.
> 
> Same counterargument as before: If I convert fillStatus (range 0..100) to int32_t, the loss of precision would be too much. In the best case, I could multiply mediaDuration * (int32_t)((double)fillStatus * GST_FORMAT_PERCENT_SCALE) * MediaTime(1, GST_FORMAT_PERCENT_SCALE).
> 
> GST_FORMAT_PERCENT_SCALE=10000

Not even that: I can't multiply MediaTime * MediaTime.

[Xabier Rodríguez Calvar]

(In reply to Enrique Ocaña from comment #25)
> >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1247
> >>> +            m_maxTimeLoaded = MediaTime(fillStatus * static_cast<double>(toGstUnsigned64Time(mediaDuration)) / 100, GST_SECOND);
> >> 
> >> Use operator* here too.
> > 
> > Same counterargument as before: If I convert fillStatus (range 0..100) to int32_t, the loss of precision would be too much. In the best case, I could multiply mediaDuration * (int32_t)((double)fillStatus * GST_FORMAT_PERCENT_SCALE) * MediaTime(1, GST_FORMAT_PERCENT_SCALE).
> > 
> > GST_FORMAT_PERCENT_SCALE=10000
> 
> Not even that: I can't multiply MediaTime * MediaTime.

You could if you implement the operator but maybe it is too much hassle. Feel free to leave it as it is then.

[Enrique Ocaña]

Created attachment 321955 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 321955 [details]
Patch

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

Still a nit a thing that was left behind but very good job. Thanks Enrique and Charlie. We'll live happier lives with this code now.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:-178
> +    MediaTime current = currentMediaTime();
>      // Avoid useless seeking.
> -    float current = currentMediaTime().toFloat();

Good type change, bad line move.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:933
> +    return result.toFloat();

This function should not be here. We should be overriding maxMediaTimeSeekable instead.

[Enrique Ocaña]

Created attachment 322079 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322079 [details]
Patch

Clearing flags on attachment: 322079

Committed r222649: <http://trac.webkit.org/changeset/222649>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.