New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 887464 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression

Blocked on:
issue 880827
issue 888476



Sign in to add a comment

WebRTC TestPacketBuffer.SeqNumWrapOneFrame failing due to clang update

Reported by yvesg@webrtc.org, Sep 20

Issue description

Steps to reproduce the problem:
On android build either dbg or rel, run:
% ./modules_unittests --gtest_filter='TestPacketBuffer.SeqNumWrapOneFrame'

What is the expected behavior?
Test passing.

What went wrong?
[FAIL] TestPacketBuffer.SeqNumWrapOneFrame:
[ RUN      ] TestPacketBuffer.SeqNumWrapOneFrame
Main  ../../modules/video_coding/video_packet_buffer_unittest.cc:80: Failure
Value of: frame_it == frames_from_callback_.end()
  Actual: true
Expected: false
Could not find frame with first sequence number 65535.
[  FAILED  ] TestPacketBuffer.SeqNumWrapOneFrame (0 ms)
[----------] 1 test from TestPacketBuffer (0 ms total)

Did this work before? Yes d757e00e8cfe98f8c5ea8c7b710e2b1a569c4a1d

Chrome version: 69.0.3497.92  Channel: n/a
OS Version: 
Flash Version: 

You can see the failure:
https://webrtc-review.googlesource.com/c/src/+/101104
I have manually checked it was introduced by this clang roll:
https://chromium.googlesource.com/chromium/src/tools/+/1ee31b34bd46a000286f808662c2c051d41e2d48
The test currently passes thanks to the revert:
https://chromium.googlesource.com/chromium/src/+/f6a8902e39eecb5d7a0be8dc61f432e451213023
 
Blockedon: 880827
Why do these tests not run on the chromium cq?
Also, chromium tests pass, any chance this is due to a code bug in the test that's just exposed by the new compiler?
Components: Internals>WebRTC
Owner: philipel@chromium.org
Status: Assigned (was: Unconfirmed)
> Why do these tests not run on the chromium cq?
I guess because these are WebRTC own tests. 

> Any chance this is due to a code bug in the test that's just exposed by the new compiler?
Yes that's what I've thought at first. Then I found this less likely after a quick inspection and considering nothing suspicious is detected by the compiler or the sanitizers.
+philipel@ Could you double-check that? Also, in the meantime, if the tests are non critical, we might want to disable them to unblock the auto-roller.
 
Owner: ----
Status: Available (was: Assigned)
I managed to boil down the issue to this simple test case:

    uint16_t a = 0xFFFF;
    uint16_t b = 0;
    
    TEST_F(TestPacketBuffer, UInt16SanityCheck) {
      if ( a != static_cast<uint16_t>(b-1) ) {
        // Huge fail, shouldn't reach this point.
        EXPECT_TRUE(0);
      } else {
        EXPECT_TRUE(1);
      }
    }

In my book, that's a compiler bug :)
See this example live here: https://webrtc-review.googlesource.com/c/src/+/101400
Cc: h...@chromium.org r...@chromium.org
Urgh :-(

What compiler flags does webrtc use? -Oz or -O2?
For Android that's -Oz. Size matters.

Here are the flags used to compile the tests (video_packet_buffer_unittest.cc)

defines = -DV8_DEPRECATION_WARNINGS -DDCHECK_ALWAYS_ON=1 -DNO_TCMALLOC -DSAFE_BROWSING_DB_REMOTE -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_GNU_SOURCE -DANDROID -DHAVE_SYS_UIO_H -DANDROID_NDK_VERSION_ROLL=r16_1 -DCR_CLANG_REVISION=\"340925-2\" -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -D__GNU_SOURCE=1 -DCHROMIUM_CXX_TWEAK_INLINES -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DWEBRTC_ENABLE_PROTOBUF=1 -DWEBRTC_INCLUDE_INTERNAL_AUDIO_DEVICE -DHAVE_SCTP -DUSE_BUILTIN_SW_CODECS -DWEBRTC_ARCH_ARM -DWEBRTC_ARCH_ARM_V7 -DWEBRTC_HAS_NEON -DWEBRTC_LIBRARY_IMPL -DWEBRTC_NON_STATIC_TRACE_EVENT_HANDLERS=1 -DWEBRTC_POSIX -DWEBRTC_LINUX -DWEBRTC_ANDROID -DABSL_ALLOCATOR_NOTHROW=1 -DGTEST_API_= -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=1 -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_HAS_ABSL=1 -DUNIT_TEST -DUNIT_TEST
include_dirs = -I../.. -Igen -I../../third_party/abseil-cpp -I../../third_party/libyuv/include -I../../third_party/googletest/custom -I../../third_party/googletest/src/googlemock/include -I../../third_party/googletest/custom -I../../third_party/googletest/src/googletest/include -I../../third_party/libvpx/source/libvpx
cflags = -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -fcolor-diagnostics -fmerge-all-constants -no-canonical-prefixes -ffunction-sections -fno-short-enums --target=arm-linux-androideabi -isystem../../third_party/android_ndk/sysroot/usr/include/arm-linux-androideabi -D__ANDROID_API__=16 -DHAVE_PTHREAD_COND_TIMEDWAIT_MONOTONIC=1 -march=armv7-a -mfloat-abi=softfp -mtune=generic-armv7-a -mfpu=neon -mthumb -Wall -Werror -Wextra -Wimplicit-fallthrough -Wthread-safety -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-user-defined-warnings -Wno-unused-lambda-capture -Wno-null-pointer-arithmetic -Wno-enum-compare-switch -Wno-ignored-pragma-optimize -Oz -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer -gdwarf-3 -g1 -fdebug-info-for-profiling -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wc++11-narrowing -Wimplicit-fallthrough -Wthread-safety -Winconsistent-missing-override -Wundef -Wunused-lambda-capture -Wno-inconsistent-missing-override
cflags_cc = -std=c++14 -fno-exceptions -fno-rtti --sysroot=../../third_party/android_ndk/sysroot -isystem../../third_party/android_ndk/sources/cxx-stl/llvm-libc++/include -isystem../../third_party/android_ndk/sources/cxx-stl/llvm-libc++abi/include -isystem../../third_party/android_ndk/sources/android/support/include -fvisibility-inlines-hidden -Wnon-virtual-dtor -Woverloaded-virtual
Cc: artit@chromium.org
I'm looking at what changed in Clang now.
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/03c592a1e9f9dbad02bfc9d1f55d8b8c5c499208

commit 03c592a1e9f9dbad02bfc9d1f55d8b8c5c499208
Author: Artem Titarenko <artit@webrtc.org>
Date: Mon Sep 24 11:12:37 2018

Disabled TestPacketBuffer.SeqNumWrapOneFrame test due to clang update

Until further investigation.
Clang update: chromium:880827

Bug:  chromium:887464 
Change-Id: Id1fe85a013920e6ae8c6ac69efb0a0502b9dd6fe
Reviewed-on: https://webrtc-review.googlesource.com/101561
Commit-Queue: Artem Titarenko <artit@webrtc.org>
Reviewed-by: Artem Titarenko <artit@webrtc.org>
Reviewed-by: Patrik Höglund <phoglund@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#24795}
[modify] https://crrev.com/03c592a1e9f9dbad02bfc9d1f55d8b8c5c499208/modules/video_coding/video_packet_buffer_unittest.cc

Blockedon: 888476
The problem was caused by a new feature that hans reverted in r342873. hans, thanks for the upstream analysis. yvesg, thanks for the excellent reduced test case. This should be better after the next clang roll (tracked in  issue 888476 ).
Owner: h...@chromium.org
Status: ExternalDependency (was: Available)
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/76346c9b0afb2febbf2bf3c8d2785ce342e8ad0e

commit 76346c9b0afb2febbf2bf3c8d2785ce342e8ad0e
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Mon Sep 24 23:40:00 2018

Roll src/third_party/webrtc 645512ba5966..a36631cdb571 (26 commits)

https://webrtc.googlesource.com/src.git/+log/645512ba5966..a36631cdb571


git log 645512ba5966..a36631cdb571 --date=short --no-merges --format='%ad %ae %s'
2018-09-24 danilchap@webrtc.org Expect late delayed task is deleted.
2018-09-24 buildbot@webrtc.org Roll chromium_revision c92470b71e..695c43e432 (593506:593611)
2018-09-24 niklas.enbom@webrtc.org Revert "Bug in histogram metric reporting."
2018-09-24 srte@webrtc.org Don't reset initial constraints in congestion controller.
2018-09-24 orphis@webrtc.org Add dummy implementation for SetCodecPReferences.
2018-09-24 terelius@webrtc.org Add helper class to process RtcEventLog events in order.
2018-09-24 yvesg@webrtc.org ResultsContainer: Initialize fields.
2018-09-24 yvesg@webrtc.org Auto roller: [unittest cleanup] Proper patching mechanism.
2018-09-24 kwiberg@webrtc.org TestStereo: Don't rely on the ACM to create encoders
2018-09-24 gustaf@webrtc.org AEC3: Delay estimator adapts even when estimated echo saturates
2018-09-24 kron@webrtc.org Refactor to remove direct memory dependency on kMaxId
2018-09-24 aleloi@webrtc.org Bug in histogram metric reporting.
2018-09-24 buildbot@webrtc.org Roll chromium_revision a7544fa319..c92470b71e (592771:593506)
2018-09-24 sprang@webrtc.org Revert "Reland "Enable simulcast screenshare by default""
2018-09-24 phensman@webrtc.org Include stringutils to allow build on chromium
2018-09-24 devicentepena@webrtc.org AEC3: Bounding the nearend spectrum used as input for the suppressor gain computation
2018-09-24 artit@webrtc.org Disabled TestPacketBuffer.SeqNumWrapOneFrame test due to clang update
2018-09-24 nisse@webrtc.org Delete unused Url class.
2018-09-24 nisse@webrtc.org Delete unused HttpData methods.
2018-09-24 kwiberg@webrtc.org AudioCodingModuleTest.TestStereo: Delete write-only variables
2018-09-24 mflodman@webrtc.org Don't throttle key-frame requests per layer.
2018-09-24 jonasolsson@webrtc.org Make fewer copies when using StringBuilder.
2018-09-24 asapersson@webrtc.org Android: Add maxFramerate to RtpParameters.
2018-09-24 artit@webrtc.org Revert "Added support of getting coverage on mac"
2018-09-24 phensman@webrtc.org Add WriteVideoToFile to video_file_reader.
2018-09-24 henrika@webrtc.org Ensures that ADM unittest uses default audio devices for all platforms.


Created with:
  gclient setdep -r src/third_party/webrtc@a36631cdb571

The AutoRoll server is located here: https://autoroll.skia.org/r/webrtc-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng

BUG=chromium:None,chromium:None,chromium:755660,chromium:855108,chromium:None,chromium:690537,chromium:887464,chromium:844647
TBR=webrtc-chromium-sheriffs-robots@google.com

Change-Id: I906ee8f2b3f7798a61aef95bce71fbcbe0d81221
Reviewed-on: https://chromium-review.googlesource.com/1241517
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#593747}
[modify] https://crrev.com/76346c9b0afb2febbf2bf3c8d2785ce342e8ad0e/DEPS

Status: Verified (was: ExternalDependency)
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 30

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/97e35ce05d4b7921acd2d523780e3e1d2d402867

commit 97e35ce05d4b7921acd2d523780e3e1d2d402867
Author: Artem Titarenko <artit@webrtc.org>
Date: Tue Oct 30 12:26:48 2018

Revert "Disabled TestPacketBuffer.SeqNumWrapOneFrame test due to clang update"

This reverts commit 03c592a1e9f9dbad02bfc9d1f55d8b8c5c499208.

Reason for revert: Problem with clang should be solved now

Original change's description:
> Disabled TestPacketBuffer.SeqNumWrapOneFrame test due to clang update
> 
> Until further investigation.
> Clang update: chromium:880827
> 
> Bug:  chromium:887464 
> Change-Id: Id1fe85a013920e6ae8c6ac69efb0a0502b9dd6fe
> Reviewed-on: https://webrtc-review.googlesource.com/101561
> Commit-Queue: Artem Titarenko <artit@webrtc.org>
> Reviewed-by: Artem Titarenko <artit@webrtc.org>
> Reviewed-by: Patrik Höglund <phoglund@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#24795}

TBR=phoglund@webrtc.org,artit@webrtc.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:887464 
Change-Id: Id4d1722b289d9f56ae2aebf576f28f3b02a4c942
Reviewed-on: https://webrtc-review.googlesource.com/c/108583
Reviewed-by: Artem Titarenko <artit@webrtc.org>
Reviewed-by: Patrik Höglund <phoglund@webrtc.org>
Commit-Queue: Artem Titarenko <artit@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25428}
[modify] https://crrev.com/97e35ce05d4b7921acd2d523780e3e1d2d402867/modules/video_coding/video_packet_buffer_unittest.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8309477a707e831908b954a314b8055de7631499

commit 8309477a707e831908b954a314b8055de7631499
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Tue Oct 30 23:44:27 2018

Roll src/third_party/webrtc 3b149e4be880..1a92cd7312ca (22 commits)

https://webrtc.googlesource.com/src.git/+log/3b149e4be880..1a92cd7312ca


git log 3b149e4be880..1a92cd7312ca --date=short --no-merges --format='%ad %ae %s'
2018-10-30 chromium-webrtc-autoroll@webrtc-ci.iam.gserviceaccount.com Roll chromium_revision 34bb9a9162..0cb3899c4e (603839:603959)
2018-10-30 mellem@webrtc.org Create a MediaTransportState enum and add a state callback to MediaTransport.
2018-10-30 emircan@webrtc.org Remove event wait logic from DesktopConfigurationMonitor
2018-10-30 alessiob@webrtc.org AGC2: renaming GainCurveApplier to Limiter.
2018-10-30 ilnik@webrtc.org Revert "Use only first payload timestamp for RTCP SR generation for audio"
2018-10-30 crodbro@webrtc.org Fix for clock reset repair.
2018-10-30 nisse@webrtc.org Delete StreamInterface::ReadLine.
2018-10-30 Peter) Slatala Update media transport settings struct
2018-10-30 terelius@webrtc.org Add support for field trials in peerconnection_client|server
2018-10-30 ilnik@webrtc.org Use only first payload timestamp for RTCP SR generation for audio
2018-10-30 terelius@webrtc.org Add field trial to enable the new RTC event log format.
2018-10-30 artit@webrtc.org Revert "Disabled TestPacketBuffer.SeqNumWrapOneFrame test due to clang update"
2018-10-30 robert@bares.me Always call ConvertToI420 with positive crop_height
2018-10-30 nisse@webrtc.org Delete OptionsFile class. Refactored only user, TurnFileAuth.
2018-10-30 srte@webrtc.org Makes PacketResult::GetSentPacket const.
2018-10-30 chromium-webrtc-autoroll@webrtc-ci.iam.gserviceaccount.com Roll chromium_revision 89ed1da2c8..34bb9a9162 (603733:603839)
2018-10-30 nisse@webrtc.org Delete unused function rtc::Flow.
2018-10-30 artit@webrtc.org Add iOS SDK unit tests for nalu_rewriter
2018-10-30 kron@webrtc.org Propagate SDP negotiation of extmap-allow-mixed to RtpHeaderExtensionMap
2018-10-30 oprypin@webrtc.org Remove likely obsolete entries from WATCHLISTS
2018-10-30 chromium-webrtc-autoroll@webrtc-ci.iam.gserviceaccount.com Roll chromium_revision 03b56190ff..89ed1da2c8 (603619:603733)
2018-10-29 chromium-webrtc-autoroll@webrtc-ci.iam.gserviceaccount.com Roll chromium_revision 55624cc6cd..03b56190ff (603513:603619)


Created with:
  gclient setdep -r src/third_party/webrtc@1a92cd7312ca

The AutoRoll server is located here: https://autoroll.skia.org/r/webrtc-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng

BUG=chromium:None,chromium:796889,chromium:none,chromium:887464,chromium:None,chromium:None,chromium:None,chromium:None
TBR=webrtc-chromium-sheriffs-robots@google.com

Change-Id: I6de52c483d648ac07efede2b984bc1b7228e6754
Reviewed-on: https://chromium-review.googlesource.com/c/1308696
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#604058}
[modify] https://crrev.com/8309477a707e831908b954a314b8055de7631499/DEPS

Sign in to add a comment