Bug 38842 - [GStreamer] float variables misused
Summary: [GStreamer] float variables misused
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 39202
  Show dependency treegraph
Reported: 2010-05-10 06:35 PDT by Philippe Normand
Modified: 2010-05-31 09:38 PDT (History)
2 users (show)

See Also:

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

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-05-10 06:35:47 PDT
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 Philippe Normand 2010-05-10 06:39:33 PDT
Created attachment 55549 [details]
proposed patch
Comment 2 Dirk Schulze 2010-05-11 01:20:33 PDT
Comment on attachment 55549 [details]
proposed patch

Please don't use static_cast<float>. narrowPrecisionToFloat is better.
Comment 3 Philippe Normand 2010-05-11 02:25:19 PDT
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 Philippe Normand 2010-05-11 07:19:16 PDT
Comment on attachment 55549 [details]
proposed patch

Asking for another review as per previous comment.
Comment 5 Eric Seidel 2010-05-20 01:21:29 PDT
I'm not sure why narrowPrecisionToFloat exists.
Comment 6 Xan Lopez 2010-05-27 01:19:53 PDT
Comment on attachment 55549 [details]
proposed patch

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 Philippe Normand 2010-05-27 11:55:30 PDT
Created attachment 57263 [details]
proposed patch
Comment 8 Philippe Normand 2010-05-27 11:56:39 PDT
Comment on attachment 57263 [details]
proposed patch

New version tackling the issues pointed out by Xan
Comment 9 Xan Lopez 2010-05-31 08:50:21 PDT
Comment on attachment 57263 [details]
proposed patch

-    GstClockTime sec = (GstClockTime)(time * GST_SECOND);
+    GstClockTime sec = (GstClockTime)(time * static_cast<float>(GST_SECOND));

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