Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2484493002:
Prepare iOS H264 HW encoder for High Profile (Closed)
|
Created:
4 years, 1 month ago by magjed_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPrepare iOS H264 HW encoder for High Profile
BUG=webrtc:6337
Committed: https://crrev.com/5d54e185d50a3a177676168486af402f287feca7
Cr-Commit-Position: refs/heads/master@{#15091}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Don't use default in switch #
Messages
Total messages: 30 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Prepare iOS H264 HW encoder for High Profile BUG=webrtc:6337 ========== to ========== Prepare iOS H264 HW encoder for High Profile BUG=webrtc:6337 ==========
magjed@webrtc.org changed reviewers: + kthelgason@webrtc.org
Kári - please take a look.
lgtm
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org Link to the patchset: https://codereview.webrtc.org/2484493002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10043)
magjed@webrtc.org changed reviewers: + tkchin@webrtc.org
Zeke - please take a look.
Zeke - ping.
lgtm https://codereview.webrtc.org/2484493002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm (right): https://codereview.webrtc.org/2484493002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:250: default: usually in iOS code we use compiler error to catch when new enum values are added instead of using default if that's easy to do here (e.g. webrtc has autolevel enum) that would be good, but not high pri https://codereview.webrtc.org/2484493002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc (right): https://codereview.webrtc.org/2484493002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc:48: H264::kProfileConstrainedBaseline, H264::kLevel3_1); I don't know how this will affect the encoder. But if AppRTCMobile looks ok this should be fine.
The CQ bit was checked by magjed@webrtc.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.webrtc.org/...
https://codereview.webrtc.org/2484493002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm (right): https://codereview.webrtc.org/2484493002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/h264_video_toolbox_encoder.mm:250: default: On 2016/11/14 18:27:27, tkchin_webrtc wrote: > usually in iOS code we use compiler error to catch when new enum values are > added instead of using default > if that's easy to do here (e.g. webrtc has autolevel enum) that would be good, > but not high pri Cool, didn't know the compiler would help finding missing enums. Done. https://codereview.webrtc.org/2484493002/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc (right): https://codereview.webrtc.org/2484493002/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/videotoolboxvideocodecfactory.cc:48: H264::kProfileConstrainedBaseline, H264::kLevel3_1); On 2016/11/14 18:27:27, tkchin_webrtc wrote: > I don't know how this will affect the encoder. But if AppRTCMobile looks ok this > should be fine. It looks good in AppRTCMobile as far as I can tell. I could keep using AutoLevel since this has worked in the past, but it feels nice to pass as much information as possible about the negotiated profile down to the encoder.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14882)
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2484493002/#ps140001 (title: "Don't use default in switch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Prepare iOS H264 HW encoder for High Profile BUG=webrtc:6337 ========== to ========== Prepare iOS H264 HW encoder for High Profile BUG=webrtc:6337 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Prepare iOS H264 HW encoder for High Profile BUG=webrtc:6337 ========== to ========== Prepare iOS H264 HW encoder for High Profile BUG=webrtc:6337 Committed: https://crrev.com/5d54e185d50a3a177676168486af402f287feca7 Cr-Commit-Position: refs/heads/master@{#15091} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5d54e185d50a3a177676168486af402f287feca7 Cr-Commit-Position: refs/heads/master@{#15091} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
