Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 6 months ago by wdzierzanowski Modified:
3 years, 6 months ago Reviewers:
xjz, wolenetz CC:
chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDeclare kAppendWholeFile as constexpr
kAppendWholeFile was dynamically initialized, causing it to be 0 at the
time of the initialization of the kMediaSourceADTSTests array when
built with VC 2015. This resulted in an assertion failure in
MockMediaSource constructor.
std::numeric_limits::max() is constexpr in C++11, so fix that by
declaring kAppendWholeFile as constexpr too.
While we're here, let's change DCHECKs in mock_media_source.cc to
CHECKS, because there is no reason to prefer DCHECK over CHECK in
test-only code.
TEST=media_unittests --gtest_filter=ProprietaryCodecs/BasicMSEPlaybackTest.PlayToEnd/*
Review-Url: https://codereview.chromium.org/2920243002
Cr-Commit-Position: refs/heads/master@{#477755}
Committed: https://chromium.googlesource.com/chromium/src/+/1250a508840b955c6e96eae698cf9783ec1f39d7
Patch Set 1 #
Created: 3 years, 6 months ago
Messages
Total messages: 21 (12 generated)
wdzierzanowski@opera.com changed reviewers: + xjz@chromium.org
Hi! I think this regressed in https://codereview.chromium.org/2808583002. What do you think about this fix?
Description was changed from ========== Declare kAppendWholeFile as constexpr kAppendWholeFile was dynamically initialized, causing it to be 0 at the time of the initialization of the kMediaSourceADTSTests array when built with VC 2015. This resulted in an assertion failure in MockMediaSource constructor. std::numeric_limits::max() is constexpr in C++11, so fix that by declaring kAppendWholeFile as constexpr too. While we're here, let's change DCHECKs in mock_media_source.cc to CHECKS, because there is no reason to prefer DCHECK over CHECK in test-only code. TEST=media_unittests --gtest_filter=ProprietaryCodecs/BasicMSEPlaybackTest.PlayToEnd/* ========== to ========== Declare kAppendWholeFile as constexpr kAppendWholeFile was dynamically initialized, causing it to be 0 at the time of the initialization of the kMediaSourceADTSTests array when built with VC 2015. This resulted in an assertion failure in MockMediaSource constructor. std::numeric_limits::max() is constexpr in C++11, so fix that by declaring kAppendWholeFile as constexpr too. While we're here, let's change DCHECKs in mock_media_source.cc to CHECKS, because there is no reason to prefer DCHECK over CHECK in test-only code. TEST=media_unittests --gtest_filter=ProprietaryCodecs/BasicMSEPlaybackTest.PlayToEnd/* ==========
wdzierzanowski@opera.com changed reviewers: + xhwang@chromium.org
xhwang@chromium.org changed reviewers: + wolenetz@chromium.org
looking good, but I'd like to defer to wolenetz@ to take a look as well
xhwang@chromium.org changed reviewers: - xhwang@chromium.org
LGTM % link in the CL description the relevant crbug, if any (I assume VC 2015 failures were cropping up on bots. If not, how did this issue get found?) Regardless, thanks for fixing this!
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I could find no crbug for this. I can make one if it's needed. I actually found this issue when running a Debug build of media_unittests for Opera. Do the bots run media_unittests built with dcheck_always_on=1?
On 2017/06/07 19:28:11, wdzierzanowski wrote: > I could find no crbug for this. I can make one if it's needed. > > I actually found this issue when running a Debug build of media_unittests for > Opera. Do the bots run media_unittests built with dcheck_always_on=1? Ah - probably they don't -- good catch. no crbug is required -- I was just curious.
OK. Thanks for reviewing!
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wdzierzanowski@opera.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1496867636458850, "parent_rev":
"81f90ea81c1e46f8e20f84e0449f397d19f809c8", "commit_rev":
"1250a508840b955c6e96eae698cf9783ec1f39d7"}
Message was sent while issue was closed.
Description was changed from ========== Declare kAppendWholeFile as constexpr kAppendWholeFile was dynamically initialized, causing it to be 0 at the time of the initialization of the kMediaSourceADTSTests array when built with VC 2015. This resulted in an assertion failure in MockMediaSource constructor. std::numeric_limits::max() is constexpr in C++11, so fix that by declaring kAppendWholeFile as constexpr too. While we're here, let's change DCHECKs in mock_media_source.cc to CHECKS, because there is no reason to prefer DCHECK over CHECK in test-only code. TEST=media_unittests --gtest_filter=ProprietaryCodecs/BasicMSEPlaybackTest.PlayToEnd/* ========== to ========== Declare kAppendWholeFile as constexpr kAppendWholeFile was dynamically initialized, causing it to be 0 at the time of the initialization of the kMediaSourceADTSTests array when built with VC 2015. This resulted in an assertion failure in MockMediaSource constructor. std::numeric_limits::max() is constexpr in C++11, so fix that by declaring kAppendWholeFile as constexpr too. While we're here, let's change DCHECKs in mock_media_source.cc to CHECKS, because there is no reason to prefer DCHECK over CHECK in test-only code. TEST=media_unittests --gtest_filter=ProprietaryCodecs/BasicMSEPlaybackTest.PlayToEnd/* Review-Url: https://codereview.chromium.org/2920243002 Cr-Commit-Position: refs/heads/master@{#477755} Committed: https://chromium.googlesource.com/chromium/src/+/1250a508840b955c6e96eae698cf... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/1250a508840b955c6e96eae698cf... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
