Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1818903004:
Bitrate controller for VideoToolbox Video Encode Accelerator (Closed)
|
Created:
4 years, 9 months ago by emircan Modified:
4 years, 7 months ago Reviewers:
kjellander_chromium, stefan-webrtc, niklas.enbom, DaleCurtis, tkchin_webrtc, kjellander_webrtc, Stefan, no sievers CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBitrate controller for VideoToolbox Video Encode Accelerator
This CL adds WebRTC's bitrate adjuster to Chrome's H264 HW encoder
implementation.
Design Doc: https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sUGU/edit?usp=sharing
BUG=500605
TEST= Tested AppRTC loopback with Chrome flag
"--enable-webrtc-hw-h264-encoding" on
https://apprtc.appspot.com/?debug=loopback&vsc=h264
Committed: https://crrev.com/ece1ceb03f53e76b2bc972def501661f57e82bc6
Cr-Commit-Position: refs/heads/master@{#393888}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : Rebase. #Patch Set 5 : #
Messages
Total messages: 74 (38 generated)
Description was changed from ========== bitrate BUG= ========== to ========== Bitrate controller for VT VEA BUG= ==========
Description was changed from ========== Bitrate controller for VT VEA BUG= ========== to ========== Bitrate controller for VideoToolbox Video Encode Accelerator Design Doc: https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sU... BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ==========
Description was changed from ========== Bitrate controller for VideoToolbox Video Encode Accelerator Design Doc: https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sU... BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== Bitrate controller for VideoToolbox Video Encode Accelerator This CL adds WebRTC's bitrate adjuster to Chrome's H264 HW encoder implementation. Design Doc: https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sU... BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ==========
emircan@chromium.org changed reviewers: + tkchin@chromium.org
PTAL.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
tkchin@webrtc.org changed reviewers: + holmer@chromium.org
tkchin@webrtc.org changed reviewers: + tkchin@webrtc.org
As alluded to by the other CL, this wasn't code we were anticipating to share. Added Stefan, who has more knowledge of WebRTC+Chrome code sharing than I do.
We already use video_coding/include directly from chromium code, I don't see why this would be a problem. On 2016/03/22 17:04:59, tkchin_webrtc wrote: > As alluded to by the other CL, this wasn't code we were anticipating to share. > Added Stefan, who has more knowledge of WebRTC+Chrome code sharing than I do.
emircan@chromium.org changed reviewers: + jfroy@chromium.org
emircan@chromium.org changed reviewers: - jfroy@chromium.org
Friendly PING. Can you PTAL at how BitrateController is used inside VEA? That wouldn't depend on the location of webrtc file.
On 2016/03/29 01:53:04, emircan wrote: > Friendly PING. Can you PTAL at how BitrateController is used inside VEA? That > wouldn't depend on the location of webrtc file. I leave the review of how BitrateAdjuster is used to tkchin, since he did the work in webrtc and knows it better than me.
On 2016/03/29 14:57:40, Stefan wrote: > On 2016/03/29 01:53:04, emircan wrote: > > Friendly PING. Can you PTAL at how BitrateController is used inside VEA? That > > wouldn't depend on the location of webrtc file. > > I leave the review of how BitrateAdjuster is used to tkchin, since he did the > work in webrtc and knows it better than me. We shouldn't depend on headers sitting in locations of targets that we don't depend on. GN will enforce this sooner or later (try it out with gn gen out/Default --check, it should complain). Moving the header into webrtc/common_video sounds good since the content+content_renderer targets already depend on it via the webrtc_h264 dependency: https://code.google.com/p/chromium/codesearch#search/&q=third_party/webrtc%20... (webrtc_h264: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...).
Thanks for the ping. lgtm for usage afaict. As long as you are updating it with the value of the encoded frame, and setting it before you try to encode it should work as expected. https://codereview.chromium.org/1818903004/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1818903004/diff/40001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:73: : bitrate_adjuster_(webrtc::Clock::GetRealTimeClock(), .5, .95), I'd check to see that these settings work on Mac as expected without bad frame drop behavior.
https://codereview.chromium.org/1818903004/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1818903004/diff/40001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:332: adjusted_bitrate_ = bitrate_adjuster_.GetAdjustedBitrateBps(); shouldn't this be set/checked on each iteration of encode task though?
emircan@chromium.org changed reviewers: - tkchin@chromium.org
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
emircan@chromium.org changed reviewers: + kjellander@webrtc.org
PTAL. I updated after moving the folders in webrtc as kjellander@ suggested. https://codereview.chromium.org/1818903004/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1818903004/diff/40001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:332: adjusted_bitrate_ = bitrate_adjuster_.GetAdjustedBitrateBps(); On 2016/03/29 21:21:20, tkchin_webrtc wrote: > shouldn't this be set/checked on each iteration of encode task though? l.276 does that. I changed it so that it takes the adjusted_bitrate as the input.
emircan@chromium.org changed reviewers: + sievers@google.com
Adding sievers@ for owners review. PTAL.
kjellander@chromium.org changed reviewers: + kjellander@chromium.org
GYP,GN,DEPS: lgtm
lgtm
On 2016/04/28 03:43:35, emircan wrote: > Adding sievers@ for owners review. PTAL. lgtm stamp
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.chromium.org/1818903004/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818903004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818903004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818903004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818903004/120001
emircan@chromium.org changed reviewers: - sievers@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Presubmit checks for third_party/webrtc OWNERS for DEPS. kjellander@ I think you are an owner but it looks for @webrtc account. Can you PTAL?
On 2016/04/28 22:33:27, emircan wrote: > Presubmit checks for third_party/webrtc OWNERS for DEPS. > > kjellander@ I think you are an owner but it looks for @webrtc account. Can you > PTAL? Sorry about that. lgtm with proper account.
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818903004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818903004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kjellander@chromium.org changed reviewers: + stefan@webrtc.org
+stefan@webrtc.org Oh, I thought the problem was that I had accidentally reviewed using my kjellander@webrtc.org account, but I actually hadn't. The problem isn't that though, the problem is that we don't have any people in the third_party/webrtc/OWNERS file (since I'm not there, it wouldn't have helped if I stamped reviewed with kjellander@webrtc.org either). See my comment for a workaround. I created https://codereview.webrtc.org/1934523002/ to fix the problem in WebRTC(adding our root owners also to the webrtc/ subdirectory, which is what's used in Chrome as src/third_party/webrtc). https://codereview.chromium.org/1818903004/diff/120001/content/common/gpu/med... File content/common/gpu/media/DEPS (right): https://codereview.chromium.org/1818903004/diff/120001/content/common/gpu/med... content/common/gpu/media/DEPS:9: "+third_party/webrtc", Oh, I've never seen that presubmit error before. It seems the check is actually looking at owners inside this directory. Unfortunately https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... doesn't have any owners (except for certain files), I suggest you change this to third_party/webrtc/common_video instead, and let's ask Stefan to approve with his stefan@webrtc.org account.
Thanks. stefan@webrtc.org PTAL? https://codereview.chromium.org/1818903004/diff/120001/content/common/gpu/med... File content/common/gpu/media/DEPS (right): https://codereview.chromium.org/1818903004/diff/120001/content/common/gpu/med... content/common/gpu/media/DEPS:9: "+third_party/webrtc", On 2016/04/29 10:27:03, kjellander (chromium) wrote: > Oh, I've never seen that presubmit error before. It seems the check is actually > looking at owners inside this directory. Unfortunately > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > doesn't have any owners (except for certain files), I suggest you change this to > third_party/webrtc/common_video instead, and let's ask Stefan to approve with > his mailto:stefan@webrtc.org account. Thanks. It makes more sense now. I thought your @webrtc.org account would be high level owner due to l.8. Adding specific webrtc folders now.
lgtm
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@chromium.org, sievers@chromium.org, tkchin@webrtc.org, holmer@chromium.org Link to the patchset: https://codereview.chromium.org/1818903004/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818903004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818903004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
emircan@chromium.org changed reviewers: + dalecurtis@chromium.org
These files has moved folders in the meantime. I need a new OWNERS stamp. dalecurtis@ can you PTAL?
dalecurtis@chromium.org changed reviewers: + sievers@chromium.org
Hmm, this is a bit odd to take a dep on WebRTC from media/ code. It looks like you also have a stale DEPS in content too. Do we really want to dep media on webrtc?
Patchset #5 (id:180001) has been deleted
On 2016/05/05 19:58:43, DaleCurtis_OOO_Until_May_16 wrote: > Hmm, this is a bit odd to take a dep on WebRTC from media/ code. It looks like > you also have a stale DEPS in content too. Do we really want to dep media on > webrtc? This code is common between WebRTC standalone(ios/mac) and media VEA(mac). I wanted to avoid code duplication by doing this. These files were in content/common/ when I started and there were already some DEPS on WebRTC there. Now, it is in /media, do you have any other suggestion for folder or DEPS structure? I removed the stale DEPS and looked at the compile problems as wel.
With the rebase, I see this is really just for media/gpu and we take a few thirdparty deps there already, so lgtm
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@chromium.org, stefan@webrtc.org, sievers@chromium.org, tkchin@webrtc.org, holmer@chromium.org Link to the patchset: https://codereview.chromium.org/1818903004/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818903004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818903004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
RS lgtm for DEPS On 2016/05/16 17:33:12, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1818903004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1818903004/200001
Message was sent while issue was closed.
Description was changed from ========== Bitrate controller for VideoToolbox Video Encode Accelerator This CL adds WebRTC's bitrate adjuster to Chrome's H264 HW encoder implementation. Design Doc: https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sU... BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== Bitrate controller for VideoToolbox Video Encode Accelerator This CL adds WebRTC's bitrate adjuster to Chrome's H264 HW encoder implementation. Design Doc: https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sU... BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Bitrate controller for VideoToolbox Video Encode Accelerator This CL adds WebRTC's bitrate adjuster to Chrome's H264 HW encoder implementation. Design Doc: https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sU... BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== Bitrate controller for VideoToolbox Video Encode Accelerator This CL adds WebRTC's bitrate adjuster to Chrome's H264 HW encoder implementation. Design Doc: https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sU... BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Committed: https://crrev.com/ece1ceb03f53e76b2bc972def501661f57e82bc6 Cr-Commit-Position: refs/heads/master@{#393888} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ece1ceb03f53e76b2bc972def501661f57e82bc6 Cr-Commit-Position: refs/heads/master@{#393888} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
