Bug 114654 – [GStreamer] Eclipse warnings in MediaPlayerPrivateGStreamer

• Component: New Bugs
• Status: RESOLVED INVALID

[Brendan Long]

[GStreamer] Eclipse warnings in MediaPlayerPrivateGStreamer

[Brendan Long]

Created attachment 198216 [details]
Patch

[Brendan Long]

Minor issues obviously, but Eclipse is annoying about them.

  * We're not initializing all of the members in GStreamerMediaPlayerPrivate and GStreamerMediaPlayerPrivateBase
  * isinf() isn't in std:: since we're not importing it with #import <cmath>. I'm trying to figure out how it actually is being imported (I assume gstreamer or glib, but I'm not sure).

[Brendan Long]

Created attachment 198220 [details]
Patch

[Brendan Long]

Actually, I think the problem is that isinf() is only in std:: in C++11 (according to http://en.cppreference.com/w/cpp/header/cmath). That can be fixed in Eclipse by explicitly setting __GXX_EXPERIMENTAL_CXX0X__. I'm not sure there's any reason to force that though.

[Philippe Normand]

CCing Zan, who, I think, recently did some work related with isinf.

[Philippe Normand]

Patch looks excepted for the isinf part, I think. We are adding C++11 support so I guess it would make sense for you to bend Eclipse to that decision :)

[Zan Dobersek]

(In reply to comment #2)
>   * isinf() isn't in std:: since we're not importing it with #import <cmath>. I'm trying to figure out how it actually is being imported (I assume gstreamer or glib, but I'm not sure).

If including the <math.h> header isn't putting the isinf method inside std::, the <wtf/MathExtras.h> header (in Source/WTF/wtf/) should handle the OS-specific setup to do so and should then be included where necessary.

To clarify, are we talking about the Eclipse IDE? What compiler is used? What version of it?

[Brendan Long]

(In reply to comment #7)
> If including the <math.h> header isn't putting the isinf method inside std::, the <wtf/MathExtras.h> header (in Source/WTF/wtf/) should handle the OS-specific setup to do so and should then be included where necessary.

I could make it define these when --std=c++11 isn't set if that would be useful.

> To clarify, are we talking about the Eclipse IDE? What compiler is used? What version of it?

This is in the Eclipse IDE. Builds work fine (presumably --std=c++11 is set?), it's just annoying that Eclipse highlights every usage of std::isinf() as an error.

It sounds like it's probably better to just document how to get Eclipse working with QtWebKit and WebKitGtk instead of changing this?

[Brendan Long]

Do you want the patch with just initializing GStreamerMediaPlayerPrivate members?

[Philippe Normand]

Yes.

[Brendan Long]

Oops didn't mean to do that.

[Brendan Long]

Created attachment 198509 [details]
Patch

[Philippe Normand]

Comment on attachment 198509 [details]
Patch

Cool, thanks Brendan!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 198509 [details]:

media/track/track-mode.html bug 114361 (author: [e-mail])
media/video-layer-crash.html bug 114744 (authors: [e-mail], [e-mail], [e-mail], and [e-mail])
transitions/color-transition-rounding.html bug 114182 (author: [e-mail])
transitions/cubic-bezier-overflow-svg-length.html bug 114183 (author: [e-mail])
transitions/interrupt-zero-duration.html bug 114184 (authors: [e-mail], [e-mail], and [e-mail])
transitions/multiple-background-transitions.html bug 114185 (author: [e-mail])
transitions/cubic-bezier-overflow-color.html bug 114186 (author: [e-mail])
transitions/multiple-shadow-transitions.html bug 114187 (author: [e-mail])
transitions/mismatched-shadow-transitions.html bug 114188 (author: [e-mail])
transitions/color-transition-all.html bug 114189 (authors: [e-mail] and [e-mail])
transitions/negative-delay.html bug 114190 (author: [e-mail])
transitions/cubic-bezier-overflow-shadow.html bug 114191 (author: [e-mail])
transitions/min-max-width-height-transitions.html bug 114192 (author: [e-mail])
transitions/cancel-transition.html bug 114193 (authors: [e-mail], [e-mail], and [e-mail])
transitions/border-radius-transition.html bug 114194 (author: [e-mail])
transitions/flex-transitions.html bug 114195 (author: [e-mail])
transitions/mixed-type.html bug 114196 (author: [e-mail])
transitions/multiple-mask-transitions.html bug 114197 (author: [e-mail])
transitions/color-transition-premultiplied.html bug 114198 (author: [e-mail])
transitions/mismatched-shadow-styles.html bug 114199 (author: [e-mail])
transitions/mask-transitions.html bug 114200 (authors: [e-mail], [e-mail], and [e-mail])
transitions/cubic-bezier-overflow-length.html bug 114201 (author: [e-mail])
transitions/multiple-background-size-transitions.html bug 114202 (authors: [e-mail] and [e-mail])
transitions/clip-transition.html bug 114203 (authors: [e-mail] and [e-mail])
transitions/cubic-bezier-overflow-transform.html bug 114204 (author: [e-mail])
transitions/interrupted-accelerated-transition.html bug 56242 (authors: [e-mail], [e-mail], and [e-mail])
transitions/background-transitions.html bug 114206 (author: [e-mail])
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html bug 114208 (authors: [e-mail] and [e-mail])
fast/loader/javascript-url-in-object.html bug 114210 (authors: [e-mail] and [e-mail])
platform/mac/editing/deleting/deletionUI-single-instance.html bug 114181 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 198509 [details]
Patch

Rejecting attachment 198509 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 198509, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ects to file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp.rej
patching file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
Hunk #1 FAILED at 108.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Philippe Normand']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/31627

[Brendan Long]

I'm confused. The commit-queue message says the patch didn't apply, but the git repos say otherwise (https://github.com/WebKit/webkit/commit/c74d372a14a8fc3184b3a61398e13d1f8cb3fa18). Do I need to do anything with this?

[Philippe Normand]

Landed in http://trac.webkit.org/changeset/148611

[Zan Dobersek]

Reviving the discussion here in hope of resolving this problem.

(In reply to comment #8)
> (In reply to comment #7)
> > If including the <math.h> header isn't putting the isinf method inside std::, the <wtf/MathExtras.h> header (in Source/WTF/wtf/) should handle the OS-specific setup to do so and should then be included where necessary.
> 
> I could make it define these when --std=c++11 isn't set if that would be useful.
> 

Let's keep this as a backup plan. It makes sense though I'm not at the moment 100% sure it's the right approach.

Also, FWIW, the MediaPlayerPrivateGStreamer source code is at the moment not compiled with std=c++11.

> > To clarify, are we talking about the Eclipse IDE? What compiler is used? What version of it?
> 
> This is in the Eclipse IDE. Builds work fine (presumably --std=c++11 is set?), it's just annoying that Eclipse highlights every usage of std::isinf() as an error.
> 

Again, -std=c++11 is not used when compiling WebCore.

If I'm understanding right, the build _does_ work but it's the Eclipse static code analysis that's marking the std::isinf as an error?

Could you try including various headers in the MediaPlayerPrivateGStreamer.cpp file and reporting the outcome regarding the std::isinf being marked as an error? The three headers I'd like you to try are
- <math.h> (what's normally included in WebCore nowadays when stdlib's math functions are required),
- <cmath> (what I hope someday will replace the inclusions from the previous line),
- <wtf/MathExtras.h> (the helper header that also sets up missing math functions for various compiler configurations, also includes <cmath>).

> It sounds like it's probably better to just document how to get Eclipse working with QtWebKit and WebKitGtk instead of changing this?

Hrm, I'd prioritize getting to the source of the problem, though I'm admittedly not affected by it.

[Brendan Long]

(In reply to comment #18)
> If I'm understanding right, the build _does_ work but it's the Eclipse static code analysis that's marking the std::isinf as an error?

Yes, the build works. I even went to the trouble to make it build inside of Eclipse to see if it would pick up the settings automatically, but it didn't help (as suggested here: https://code.google.com/p/chromium/wiki/LinuxEclipseDev).

I actually get errors (reported in Eclipse's editor but not the build) on std::isinf (but not ::isinf) and roundf

One guess could be related to my relatively old Ubuntu version  (12.04), but I assume if it was a GCC problem, it wouldn't compile.

> - <math.h> (what's normally included in WebCore nowadays when stdlib's math functions are required),
> - <cmath> (what I hope someday will replace the inclusions from the previous line),
> - <wtf/MathExtras.h> (the helper header that also sets up missing math functions for various compiler configurations, also includes <cmath>).

I get the same results for all three:

  * roundf not resolved
  * std::isinf(float): Invalid arguments '
Candidates are:
__gnu_cxx::__enable_if<&0[std::__is_arithmetic<#0>::__value],int>::__type isinf(#0)
'

[Brendan Long]

It works on my home computer (Fedora 18). Since this only happens on a really old distro (Ubuntu 12.04), I doubt this is really worth spending any time on.