Bug 38842

Summary:

[GStreamer] float variables misused

Product:

WebKit

Reporter:

Philippe Normand <pnormand>

Component:

WebKitGTK

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

Bug Blocks:

39202

Attachments:

Description	Flags
proposed patch	none
proposed patch	xan.lopez: review+

Philippe Normand

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

Attachments
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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Philippe Normand

Comment 1 2010-05-10 06:39:33 PDT

Created attachment 55549 [details] proposed patch

Dirk Schulze

Comment 2 2010-05-11 01:20:33 PDT

Comment on attachment 55549 [details] proposed patch Please don't use static_cast<float>. narrowPrecisionToFloat is better.

Philippe Normand

Comment 3 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 ;)

Philippe Normand

Comment 4 2010-05-11 07:19:16 PDT

Comment on attachment 55549 [details] proposed patch Asking for another review as per previous comment.

Eric Seidel (no email)

Comment 5 2010-05-20 01:21:29 PDT

I'm not sure why narrowPrecisionToFloat exists.

Xan Lopez

Comment 6 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).

Philippe Normand

Comment 7 2010-05-27 11:55:30 PDT

Created attachment 57263 [details] proposed patch

Philippe Normand

Comment 8 2010-05-27 11:56:39 PDT

Comment on attachment 57263 [details] proposed patch New version tackling the issues pointed out by Xan

Xan Lopez

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

Philippe Normand

Comment 10 2010-05-31 09:37:24 PDT

Landed in http://trac.webkit.org/changeset/60442 Thanks for the review :)

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