Closed Bug 1122586 Opened 5 years ago Closed 5 years ago

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


(Core :: Audio/Video, defect)

Not set



Tracking Status
firefox35 --- affected
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed


(Reporter: jbeich, Assigned: jbeich)




(1 file, 2 obsolete files)

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.
Attached patch use mozilla::Abs (obsolete) — Splinter Review
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+
Attached patch use mozilla::Abs, v2 (obsolete) — Splinter 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

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: 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-
Attachment #8550648 - Flags: review?(dholbert)
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+
(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.
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?
Closed: 5 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+

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.
Assignee: nobody → jbeich
Jan? (comment #12)
Flags: needinfo?(jbeich)
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)
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?
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.
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.