Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1601863002:
Remove non-batching assumptions from SdkMediaCodecBridgeTest.DoNormal (Closed)
|
Created:
4 years, 11 months ago by kraush Modified:
4 years, 11 months ago Reviewers:
qinmin CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, avayvod+watch_chromium.org, mcasas+watch_chromium.org, mlamouri+watch-media_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove non-batching assumptions from SdkMediaCodecBridgeTest.DoNormal
This change alters SdkMediaCodecBridgeTest.DoNormal in
order to not have it assume that the AudioOutput will
get dequeued in three batches. While this is the maximum
number, some devices batch output (e.g. MT 8127 and MT 8135
chipsets), thus reducing the total number of dequeues required.
Since we also know the expected size of the output, this
test now also verifies the full expected output stream
has been dequeued.
BUG=578881
Committed: https://crrev.com/837de824489ed5d15ac4cd82580e396631d38eb6
Cr-Commit-Position: refs/heads/master@{#371965}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed incorrect (EXPECT_EQ) and redundant (ASSERT_LE) checks #Patch Set 3 : Fixed clang build by using constants #Messages
Total messages: 20 (7 generated)
Description was changed from ========== Remove non-batching assumptions from SdkMediaCodecBridgeTest.DoNormal This change alters SdkMediaCodecBridgeTest.DoNormal in order to not have it assume that the AudioOutput will get dequeued in three batches. While this is the maximum number, some devices batch output (e.g. MT 8127 and MT 8135 chipsets), thus reducing the total number of dequeues required. Since we also know the expected size of the output, this test now also verifies the full expected output stream has been dequeued. BUG=578881 ========== to ========== Remove non-batching assumptions from SdkMediaCodecBridgeTest.DoNormal This change alters SdkMediaCodecBridgeTest.DoNormal in order to not have it assume that the AudioOutput will get dequeued in three batches. While this is the maximum number, some devices batch output (e.g. MT 8127 and MT 8135 chipsets), thus reducing the total number of dequeues required. Since we also know the expected size of the output, this test now also verifies the full expected output stream has been dequeued. BUG=578881 ==========
kraush@amazon.com changed reviewers: + qinmin@chromium.org
Hi qinmin, Can you take a look at this proposed fix for https://crbug.com/578881 ? Thanks! :) Holger
https://codereview.chromium.org/1601863002/diff/1/media/base/android/sdk_medi... File media/base/android/sdk_media_codec_bridge_unittest.cc (right): https://codereview.chromium.org/1601863002/diff/1/media/base/android/sdk_medi... media/base/android/sdk_media_codec_bridge_unittest.cc:211: EXPECT_EQ(++input_pts, timestamp.InMicroseconds()); should this also be removed? it is possible that the first output is for 2 inputs, and the 2nd output is for the 3rd input
On 2016/01/19 00:57:49, qinmin wrote: > https://codereview.chromium.org/1601863002/diff/1/media/base/android/sdk_medi... > File media/base/android/sdk_media_codec_bridge_unittest.cc (right): > > https://codereview.chromium.org/1601863002/diff/1/media/base/android/sdk_medi... > media/base/android/sdk_media_codec_bridge_unittest.cc:211: > EXPECT_EQ(++input_pts, timestamp.InMicroseconds()); > should this also be removed? it is possible that the first output is for 2 > inputs, and the 2nd output is for the 3rd input Good point, thanks! :) I'll remove it and post an updated diff.
New diff. I've removed the timestamp check as recommended, and also removed the redundant input_pts assert inside the while (we already have an identical check right after the while) Please take a look at this new revision. https://codereview.chromium.org/1601863002/diff/1/media/base/android/sdk_medi... File media/base/android/sdk_media_codec_bridge_unittest.cc (right): https://codereview.chromium.org/1601863002/diff/1/media/base/android/sdk_medi... media/base/android/sdk_media_codec_bridge_unittest.cc:211: EXPECT_EQ(++input_pts, timestamp.InMicroseconds()); On 2016/01/19 00:57:49, qinmin wrote: > should this also be removed? it is possible that the first output is for 2 > inputs, and the 2nd output is for the 3rd input Acknowledged.
lgtm
The CQ bit was checked by kraush@amazon.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1601863002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1601863002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
On 2016/01/27 18:46:16, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) Sorry, seems I messed up variable use between the diffs! :( gcc didn't notice, and neither did I. I'll update the variable use and re-build locally using clang. Will post diff 3 soon.
Sorry for the failure on diff 2! I've fixed variable used and verified that it now builds with clang and still succeeds C 25.564s Main ******************************************************************************** C 25.564s Main [==========] 1 test ran. C 25.564s Main [ PASSED ] 1 test. C 25.564s Main ******************************************************************************** I 25.570s Main Wrote device cache: /home/local/ANT/kraush/workspaces/chrome_android/src/out/Debug/devi Can you please take another look? Thanks! Holger
lgtm
The CQ bit was checked by kraush@amazon.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1601863002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1601863002/40001
Message was sent while issue was closed.
Description was changed from ========== Remove non-batching assumptions from SdkMediaCodecBridgeTest.DoNormal This change alters SdkMediaCodecBridgeTest.DoNormal in order to not have it assume that the AudioOutput will get dequeued in three batches. While this is the maximum number, some devices batch output (e.g. MT 8127 and MT 8135 chipsets), thus reducing the total number of dequeues required. Since we also know the expected size of the output, this test now also verifies the full expected output stream has been dequeued. BUG=578881 ========== to ========== Remove non-batching assumptions from SdkMediaCodecBridgeTest.DoNormal This change alters SdkMediaCodecBridgeTest.DoNormal in order to not have it assume that the AudioOutput will get dequeued in three batches. While this is the maximum number, some devices batch output (e.g. MT 8127 and MT 8135 chipsets), thus reducing the total number of dequeues required. Since we also know the expected size of the output, this test now also verifies the full expected output stream has been dequeued. BUG=578881 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove non-batching assumptions from SdkMediaCodecBridgeTest.DoNormal This change alters SdkMediaCodecBridgeTest.DoNormal in order to not have it assume that the AudioOutput will get dequeued in three batches. While this is the maximum number, some devices batch output (e.g. MT 8127 and MT 8135 chipsets), thus reducing the total number of dequeues required. Since we also know the expected size of the output, this test now also verifies the full expected output stream has been dequeued. BUG=578881 ========== to ========== Remove non-batching assumptions from SdkMediaCodecBridgeTest.DoNormal This change alters SdkMediaCodecBridgeTest.DoNormal in order to not have it assume that the AudioOutput will get dequeued in three batches. While this is the maximum number, some devices batch output (e.g. MT 8127 and MT 8135 chipsets), thus reducing the total number of dequeues required. Since we also know the expected size of the output, this test now also verifies the full expected output stream has been dequeued. BUG=578881 Committed: https://crrev.com/837de824489ed5d15ac4cd82580e396631d38eb6 Cr-Commit-Position: refs/heads/master@{#371965} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/837de824489ed5d15ac4cd82580e396631d38eb6 Cr-Commit-Position: refs/heads/master@{#371965} |
