Bug 38842 - [GStreamer] float variables misused
: [GStreamer] float variables misused
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 39202
  Show dependency treegraph
 
Reported: 2010-05-10 06:35 PST by
Modified: 2010-05-31 09:38 PST (History)


Attachments
proposed patch (6.26 KB, patch)
2010-05-10 06:39 PST, Philippe Normand
no flags Review Patch | Details | Formatted Diff | Diff
proposed patch (6.65 KB, patch)
2010-05-27 11:55 PST, Philippe Normand
xan.lopez: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-05-10 06:35:47 PST
In the player code we currently use 0.0 for float variables. But 0.0 is usually for doubles. We should use 0.0f instead. There are also missing casts around GST_SECOND.
------- Comment #1 From 2010-05-10 06:39:33 PST -------
Created an attachment (id=55549) [details]
proposed patch
------- Comment #2 From 2010-05-11 01:20:33 PST -------
(From update of attachment 55549 [details])
Please don't use static_cast<float>. narrowPrecisionToFloat is better.
------- Comment #3 From 2010-05-11 02:25:19 PST -------
As discussed on IRC with krit, if we were to use narrowPrecisionToFloat() we'd need to add templates in FloatConversion.h for the glib types. It doesn't seem to make much sense as that function only performs a static_cast<float> underneath. It just complicates code readability in my opinion ;)
------- Comment #4 From 2010-05-11 07:19:16 PST -------
(From update of attachment 55549 [details])
Asking for another review as per previous comment.
------- Comment #5 From 2010-05-20 01:21:29 PST -------
I'm not sure why narrowPrecisionToFloat exists.
------- Comment #6 From 2010-05-27 01:19:53 PST -------
(From update of attachment 55549 [details])
Just a couple of comments:


>-        width = gst_util_uint64_scale_int(originalHeight, displayWidth, displayHeight);
>+        width = static_cast<int>(gst_util_uint64_scale_int(originalHeight, displayWidth, displayHeight));
>         height = originalHeight;

There's precision loss in this kind of casting.

>-    float currentPosition = playbackPosition(m_playBin) * GST_SECOND;
>+    float currentPosition = playbackPosition(m_playBin) * static_cast<float>(GST_SECOND);

This seems strange to me; for one I'd assume the casting would happen automatically, but if it does not, it seems more reasonable to cast the whole operation instead of just one operand (which also happens to be a constant).
------- Comment #7 From 2010-05-27 11:55:30 PST -------
Created an attachment (id=57263) [details]
proposed patch
------- Comment #8 From 2010-05-27 11:56:39 PST -------
(From update of attachment 57263 [details])
New version tackling the issues pointed out by Xan
------- Comment #9 From 2010-05-31 08:50:21 PST -------
(From update of attachment 57263 [details])
-    GstClockTime sec = (GstClockTime)(time * GST_SECOND);
+    GstClockTime sec = (GstClockTime)(time * static_cast<float>(GST_SECOND));

You forgot this, please fix when landing.
------- Comment #10 From 2010-05-31 09:37:24 PST -------
Landed in http://trac.webkit.org/changeset/60442
Thanks for the review :)