Closed Bug 1122547 Opened 5 years ago Closed 5 years ago

media/webrtc/trunk/webrtc/modules/video_coding/main/source/receiver.cc:162:14: error: call of overloaded 'abs(int64_t&)' is ambiguous

Categories

(Core :: WebRTC, defect)

x86
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

Attachments

(1 file, 2 obsolete files)

next_render_time_ms and now_ms are of |int64_t| type. On FreeBSD i386 (and other 32bit archs) it would be |long long|. While FreeBSD has llabs() it doesn't define _GLIBCXX_USE_C99 which means no std::llabs(). For old libstdc++ (from GCC 4.2.1) this also means no |long long std::abs(long long)|.

https://svnweb.freebsd.org/changeset/base/255294
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54686

Not sure why GCC-4.8 with _GLIBCXX_USE_LONG_LONG disabled doesn't fail like Clang-3.4 + libstdc++-4.2.

In file included from media/webrtc/trunk/webrtc/modules/modules_webrtc_video_coding/Unified_cpp__webrtc_video_coding0.cpp:137:
media/webrtc/trunk/webrtc/modules/video_coding/main/source/receiver.cc:162:14: error:
      call to 'abs' is ambiguous
  } else if (std::abs(next_render_time_ms - now_ms) > max_video_delay_ms_) {
             ^~~~~~~~
/usr/include/stdlib.h:83:6: note: candidate function
int      abs(int) __pure2;
         ^
/usr/include/c++/4.2/cstdlib:143:3: note: candidate function
  abs(long __i) { return labs(__i); }
  ^
In file included from media/webrtc/trunk/webrtc/modules/modules_webrtc_video_coding/Unified_cpp__webrtc_video_coding0.cpp:137:
media/webrtc/trunk/webrtc/modules/video_coding/main/source/receiver.cc:167:35: error:
      call to 'abs' is ambiguous
                 static_cast<int>(std::abs(next_render_time_ms - now_ms)),
                                  ^~~~~~~~
/usr/include/stdlib.h:83:6: note: candidate function
int      abs(int) __pure2;
         ^
/usr/include/c++/4.2/cstdlib:143:3: note: candidate function
  abs(long __i) { return labs(__i); }
  ^
2 errors generated.
Attached patch use mozilla::Abs (obsolete) — Splinter Review
mozilla::Abs() adds |unsigned| so casting is required to avoid

  In file included from media/webrtc/trunk/webrtc/modules/modules_webrtc_video_coding/Unified_cpp__webrtc_video_coding0.cpp:137:
  media/webrtc/trunk/webrtc/modules/video_coding/main/source/receiver.cc:164:57: warning:
        comparison of integers of different signs: 'typename detail::AbsReturnType<long long>::Type'
        (aka 'unsigned long long') and 'int' [-Wsign-compare]
    } else if (mozilla::Abs(next_render_time_ms - now_ms) > max_video_delay_ms_) {
               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~
  1 warning generated.
Attachment #8550309 - Flags: review?(rjesup)
Attached patch use mozilla::Abs, v2 (obsolete) — Splinter Review
Oops, use static_cast like the rest of the file.
Attachment #8550309 - Attachment is obsolete: true
Attachment #8550309 - Flags: review?(rjesup)
Attachment #8550322 - Flags: review?(rjesup)
Comment on attachment 8550322 [details] [diff] [review]
use mozilla::Abs, v2

Review of attachment 8550322 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/trunk/webrtc/modules/video_coding/main/source/receiver.cc
@@ +161,5 @@
>    // Assume that render timing errors are due to changes in the video stream.
>    if (next_render_time_ms < 0) {
>      timing_error = true;
> +  } else if (static_cast<int>(mozilla::Abs(next_render_time_ms - now_ms)) >
> +             max_video_delay_ms_) {

We really, really try to avoid mozilla-isms in webrtc.org import files, unless #ifdefed (and mostly we try to avoid those other than platform ifdefs like GONK and BSD OS ifdefs).

a) can we redo this to be non-mozilla-specific but still have the std::abs() functionality needed here?  Perhaps by casting other bits - such that it's upstreamable.

b) if not, let's make it #ifdef'd to the BSDs in question, and try to avoid mozillaisms in that as well.
Attachment #8550322 - Flags: review?(rjesup) → review-
Attached patch overload to intSplinter Review
Let's cast before dropping sign. Return value of overloaded std::abs() is still signed, anyway. And WEBRTC_TRACE already assumes time delta cannot be larger than |int|.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97bc729b1d24
Attachment #8550322 - Attachment is obsolete: true
Attachment #8550645 - Flags: review?(rjesup)
Attachment #8550645 - Flags: review?(rjesup) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/741ba2a22877
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.