Closed
Bug 1122586
Opened 9 years ago
Closed 9 years ago
dom/media/MediaDecoderStateMachine.cpp:1429:7: error: call of overloaded 'abs(int64_t&)' is ambiguous
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: jbeich, Assigned: jbeich)
References
Details
Attachments
(1 file, 2 obsolete files)
2.44 KB,
patch
|
dholbert
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
std::abs(long long) may not be implemented on some platforms. Same as bug 1122547, bug 824317 or bug 818391. In file included from dom/media/Unified_cpp_dom_media1.cpp:38: dom/media/MediaDecoderStateMachine.cpp:1429:7: error: call to 'abs' is ambiguous std::abs(aDuration - duration) > ESTIMATED_DURATION_FUZZ_FACTOR_USECS) { ^~~~~~~~ /usr/include/stdlib.h:83:6: note: candidate function int abs(int) __pure2; ^ /usr/include/c++/4.2/cmath:89:3: note: candidate function abs(double __x) ^ /usr/include/c++/4.2/cmath:93:3: note: candidate function abs(float __x) ^ /usr/include/c++/4.2/cmath:97:3: note: candidate function abs(long double __x) ^ /usr/include/c++/4.2/cstdlib:143:3: note: candidate function abs(long __i) { return labs(__i); } ^ 1 error generated.
Attachment #8550332 -
Flags: review?(dholbert)
Comment 2•9 years ago
|
||
Comment on attachment 8550332 [details] [diff] [review] use mozilla::Abs Looks good -- thanks! I wasn't aware of the problem of std::abs vs. llabs for long long. (addressed w/ mozilla::Abs per bug 835542) r=me with one nit: >+++ b/dom/media/MediaDecoderStateMachine.cpp >@@ -35,16 +35,18 @@ > #include "prenv.h" > #include "mozilla/Preferences.h" > #include "gfx2DGlue.h" > #include "nsPrintfCString.h" > #include "DOMMediaStream.h" > > #include <algorithm> > >+#include "mozilla/MathAlgorithms.h" >+ > namespace mozilla { So, we're not really consistent about #include-ordering, across the tree -- but in this file, it looks like the "mozilla/" #includes are intermixed with the rest (as shown above with e.g. "mozilla/Preferences.h" So, rather than creating a new space-separated block of includes (with only this one new line), it'd be cleaner to just stick this new #include adjacent to an existing "#include mozilla/whatever" in the main #include-list up above.
Attachment #8550332 -
Flags: review?(dholbert) → review+
Try failed with the following. I had to remove |const| in order to reproduce. dom/media/MediaDecoderStateMachine.cpp: In member function 'void mozilla::MediaDecoderStateMachine::UpdateEstimatedDuration(int64_t)': dom/media/MediaDecoderStateMachine.cpp:1434:44: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] cc1plus: all warnings being treated as errors try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=281cc61caa3c re-try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99cca830efc6
Attachment #8550332 -
Attachment is obsolete: true
Attachment #8550430 -
Flags: review?(dholbert)
Comment 4•9 years ago
|
||
Gotcha, that's because ESTIMATED_DURATION_FUZZ_FACTOR_USECS is an int64_t. This comparison is actually the only place it's used: https://mxr.mozilla.org/mozilla-central/search?string=ESTIMATED_DURATION_FUZZ_FACTOR_USECS ...so rather than static_casting the result of Abs() to match ESTIMATED_DURATION_FUZZ_FACTOR_USECS, I'd prefer we instead convert ESTIMATED_DURATION_FUZZ_FACTOR_USECS to match the result of this Abs() call. i.e. let's turn ESTIMATED_DURATION_FUZZ_FACTOR_USECS into a uint64_t. (Presumably it was only signed before because abs() & std::abs returned a signed value.)
Comment 5•9 years ago
|
||
Comment on attachment 8550430 [details] [diff] [review] use mozilla::Abs, v2 Marking r- per comment 4, but I'll gladly grant swift r+ on a patch that changes the type of ESTIMATED_DURATION_FUZZ_FACTOR_USECS, assuming that doesn't cause any unexpected bustage. (also: technically this should have sign-off from someone on the media team, but it's a small enough change [and dependent-enough on my patch in bug 1073716] that I'm comfortable reviewing.)
Attachment #8550430 -
Flags: review?(dholbert) → review-
uint64_t for ESTIMATED_DURATION_FUZZ_FACTOR_USECS https://treeherder.mozilla.org/#/jobs?repo=try&revision=97bc729b1d24
Attachment #8550648 -
Flags: review?(dholbert)
Attachment #8550430 -
Attachment is obsolete: true
Comment 7•9 years ago
|
||
Comment on attachment 8550648 [details] [diff] [review] use mozilla::Abs, v3 Looks great -- thanks, and sorry for the build bustage! (Looks like the patch is credited to Barbara Guida, rather than to you, in the patch-headers -- I assume that's intentional & OK with you? (i.e. maybe she wrote the first version of this patch?))
Attachment #8550648 -
Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #7) > (Looks like the patch is credited to Barbara Guida, rather than to you, in > the patch-headers -- I assume that's intentional & OK with you? (i.e. maybe > she wrote the first version of this patch?)) Yes, the first version author. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=196772#c2
Keywords: checkin-needed
Comment on attachment 8550648 [details] [diff] [review] use mozilla::Abs, v3 Approval Request Comment [Feature/regressing bug #]: regression from bug 1073716 [User impact if declined]: build broken on at least FreeBSD 8.4/9.3 i386 [Describe test coverage new/current, TBPL]: Try in comment 6, m-i soon [Risks and why]: Low, can only break build. [String/UUID change made/needed]: None
Attachment #8550648 -
Flags: approval-mozilla-beta?
Attachment #8550648 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox35:
--- → affected
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9577955e37cc
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9577955e37cc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•9 years ago
|
Attachment #8550648 -
Flags: approval-mozilla-beta?
Attachment #8550648 -
Flags: approval-mozilla-beta+
Attachment #8550648 -
Flags: approval-mozilla-aurora?
Attachment #8550648 -
Flags: approval-mozilla-aurora+
Comment 12•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba151cf00e17 https://hg.mozilla.org/releases/mozilla-beta/rev/506cfb41b8f3 Word on the street is that there may be a 35.0.1 point release. Feel free to nominate this patch for that if you think it's a safe ride-along.
Updated•9 years ago
|
Assignee: nobody → jbeich
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8550648 [details] [diff] [review] use mozilla::Abs, v3 Yes, safe for 35.x unlike bug 1122547 for 36 and 37 which may need some time to bake. Approval Request Comment See comment 9.
Flags: needinfo?(jbeich)
Attachment #8550648 -
Flags: approval-mozilla-release?
Comment 15•9 years ago
|
||
(But is there a benefit to backporting this to 35.x? i.e. is this the only thing stopping the Firefox 35 sources from compiling successfullly on FreeBSD? What does it actually mean for FreeBSD, if we don't take this vs. if we do? This patch feels pretty safe, but it's not clear to me whether there's a strong need for a release-branch backport, or much benefit that would come out of the backport.)
Flags: needinfo?(jbeich)
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8550648 [details] [diff] [review] use mozilla::Abs, v3 Fair point. The patch only helps 32bit archs with old toolchain, mostly Tier3. Recent FreeBSD i386 versions are unaffected but not powerpc as it still uses libstdc++. So, I'm fine keeping it downstream as a local backport. At least FreeBSD 10.0, 10.1, -CURRENT i386 or FreeBSD 8.4, 9.3, 10.0, 10.1, -CURRENT amd64 build fine without the patch. I'm not sure about the state of Firefox on other FreeBSD architectures.
Flags: needinfo?(jbeich)
Attachment #8550648 -
Flags: approval-mozilla-release?
Assignee | ||
Comment 17•9 years ago
|
||
Landry, Martin, what libstdc++ version you use in order to build Firefox? Does it fail to build on 32bit archs with error like in comment 0?
Comment 18•9 years ago
|
||
I dont remember experiencing this build failure on powerpc or i386 w/35 (nor in other branches), which use gcc 4.8 and its matching libstdc++. I can give a try to clang/libstdc++ 4.2.1 on i386, that's the way we build firefox on amd64.
Comment 19•9 years ago
|
||
We have gcc 4.8.4 in base in netbsd-7 and -current, and require at least gcc 4.6 (from pkgsrc) on older NetBSD releases (so should not be affected, IIUC)
You need to log in
before you can comment on or make changes to this bug.
Description
•