dom/media/MediaDecoderStateMachine.cpp:1429:7: error: call of overloaded 'abs(int64_t&)' is ambiguous

RESOLVED FIXED in Firefox 36

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Jan Beich, Assigned: Jan Beich)

Tracking

Trunk
mozilla38
x86
FreeBSD
Points:
---

Firefox Tracking Flags

(firefox35 affected, firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8550332 [details] [diff] [review]
use mozilla::Abs
Attachment #8550332 - Flags: review?(dholbert)
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+
(Assignee)

Comment 3

3 years ago
Created attachment 8550430 [details] [diff] [review]
use mozilla::Abs, v2

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)
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 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-
(Assignee)

Comment 6

3 years ago
Created attachment 8550648 [details] [diff] [review]
use mozilla::Abs, v3

uint64_t for ESTIMATED_DURATION_FUZZ_FACTOR_USECS

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97bc729b1d24
Attachment #8550648 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8550430 - Attachment is obsolete: true
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+
(Assignee)

Comment 8

3 years ago
(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
(Assignee)

Comment 9

3 years ago
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?
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
(Assignee)

Updated

3 years ago
status-firefox35: --- → affected
https://hg.mozilla.org/integration/mozilla-inbound/rev/9577955e37cc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9577955e37cc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8550648 - Flags: approval-mozilla-beta?
Attachment #8550648 - Flags: approval-mozilla-beta+
Attachment #8550648 - Flags: approval-mozilla-aurora?
Attachment #8550648 - Flags: approval-mozilla-aurora+
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.
status-firefox36: affected → fixed
status-firefox37: affected → fixed
status-firefox38: affected → fixed
Assignee: nobody → jbeich
Jan? (comment #12)
Flags: needinfo?(jbeich)
(Assignee)

Comment 14

3 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?
(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

3 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

3 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?
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

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