Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1636083003:
H264 HW encode using VideoToolbox (Closed)
|
Created:
4 years, 11 months ago by emircan Modified:
4 years, 9 months ago Reviewers:
Avi (use Gerrit), jfroy, sandersd (OOO until July 31), mcasas, DaleCurtis, Nico, Pawel Osciak, miu CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, hbos_chromium, jam, mcasas+watch_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org, tkchin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionH264 HW encode using VideoToolbox
This CL adds VTVideoEncodeAccelerator which enables H264 encode support using
VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes
under video_toolbox_helpers.*.
Note that, this is the first CL and H264 codec is still behind a flag. More
patches will follow adding additional codec profiles and support for
bitrate adaptations.
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/3956fa1cac34dd5682c271d77463accdd7191102
Cr-Commit-Position: refs/heads/master@{#381286}
Patch Set 1 : #
Total comments: 28
Patch Set 2 : mcasas@ and hbos@ comments. #Patch Set 3 : miu@ comments. #
Total comments: 39
Patch Set 4 : miu@ comments. #
Total comments: 58
Patch Set 5 : posciak@ comments. #Patch Set 6 : Rename vt_video_encode_accelerator to vt_video_encode_accelerator_mac. #
Total comments: 18
Patch Set 7 : miu@ comments. #
Total comments: 36
Patch Set 8 : miu@ and jfroy@ comments #
Total comments: 6
Patch Set 9 : miu@ and jfroy@ comments. #
Total comments: 22
Patch Set 10 : posciak@ comments #Patch Set 11 : Updates #
Total comments: 8
Patch Set 12 : Fix unittests. #Patch Set 13 : jfroy@ comments. #
Total comments: 14
Patch Set 14 : miu@ comments. #
Total comments: 8
Patch Set 15 : miu@ nits. #Patch Set 16 : Fix cast tests. #
Total comments: 6
Patch Set 17 : Update comment. #
Total comments: 40
Patch Set 18 : posciak@ comments. #Patch Set 19 : mcasas@ nit. #
Total comments: 2
Patch Set 20 : posciak@ comments. #
Total comments: 16
Patch Set 21 : posciak@ comments. #Patch Set 22 : Rebase & posciak nits. #Messages
Total messages: 98 (40 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== adding base for mac. BUG= ========== to ========== H264 VideoToolbox hw encode BUG= ==========
Description was changed from ========== H264 VideoToolbox hw encode BUG= ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideo BUG=500605 ==========
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideo BUG=500605 ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator whcih enables H264 encode support using VideoToolbox on mac platforms. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. BUG=500605 ==========
Patchset #1 (id:20001) has been deleted
emircan@chromium.org changed reviewers: + hbos@chromium.org, mcasas@chromium.org, miu@chromium.org
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator whcih enables H264 encode support using VideoToolbox on mac platforms. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. BUG=500605 ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator whcih enables H264 encode support using VideoToolbox on mac platforms. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/r/196623743?debug=loopback&vsc=h264 ==========
Patchset #1 (id:40001) has been deleted
PTAL
On 2016/02/02 06:08:46, emircan wrote: > PTAL It would be great if we could try to use video_encode_accelerator_unittest.cc please. Thanks!
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator whcih enables H264 encode support using VideoToolbox on mac platforms. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/r/196623743?debug=loopback&vsc=h264 ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator whcih enables H264 encode support using VideoToolbox on mac platforms. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ==========
Patchset #1 (id:60001) has been deleted
On 2016/02/02 06:15:38, Pawel Osciak wrote: > On 2016/02/02 06:08:46, emircan wrote: > > PTAL > > It would be great if we could try to use video_encode_accelerator_unittest.cc > please. Thanks! Thanks for the notice Pawel. I uploaded a new version using vea_unittest. Since we don't support frame rate & fps changes and H264 ffmpeg decoder, I decided to define the enabled tests seperately. We can add more there as functionality comes. Kindly ping for PTAL.
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator whcih enables H264 encode support using VideoToolbox on mac platforms. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread and additional profile and functionality support. 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: + posciak@chromium.org
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread and additional profile and functionality support. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ==========
This code is all very much unfamiliar to me, it would be difficult for me to do an adequate review of this. Only had time for a quick look-through. https://codereview.chromium.org/1636083003/diff/80001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/1636083003/diff/80001/content/common/BUILD.gn... content/common/BUILD.gn:238: "gpu/media/vt_video_encode_accelerator.h", Is there a reason the decoder accelerator is named "_mac" and the encoder isn't? https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1098: }; It seems unlikely that VTVEA would be used, this only happens if all the other encoders fail to initialize? What is VEAClient::CreateEncoder() used for? https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:103: input_visible_size_.GetArea() / kOutputBufferSizeRatio)); About thread-safety, how is this class used across threads? Is |child_task_runner_| not always the current thread? Is it used on multiple threads and if not, why PostTask? https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:169: if (compression_session_) { Does it make sense to DCHECK instead?
a few comments, could only read so far. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:35: }; Recommend a private: DISALLOW_IMPLICIT_CONSTRUCTORS(InProgressFrameEncode); https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:47: }; Same here, use DISALLOW_IMPLICIT_CONSTRUCTORS https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:90: const bool session_created = ResetCompressionSession(); No need for temporary? https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:96: client_ptr_factory_.reset(new base::WeakPtrFactory<Client>(client)); This |client_ptr_factory_| is only used here, do you need to hold on to it? https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:128: // CVPixelBufferPool for the allocation of incoming video frames. nit: s/video frames/VideoFrames/? https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:169: if (compression_session_) { nit: if (!compression_session_) return; ... https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:233: // TODO(emircan): Investigate IOS constraints. ? https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... File content/common/gpu/media/vt_video_encode_accelerator.h (right): https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.h:42: typedef CoreMediaGlue::CMSampleBufferRef CMSampleBufferRef; using CMSampleBufferRef = CoreMediaGlue::CMSampleBufferRef; here and in the next 2 lines. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.h:79: const scoped_refptr<base::SingleThreadTaskRunner> child_task_runner_; nit: s/child_task_runner_/callback_task_runner_/ ? https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.h:83: // child_task_runner_. |child_task_runner_|
https://codereview.chromium.org/1636083003/diff/80001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/1636083003/diff/80001/content/common/BUILD.gn... content/common/BUILD.gn:238: "gpu/media/vt_video_encode_accelerator.h", On 2016/02/03 18:21:08, hbos_chromium wrote: > Is there a reason the decoder accelerator is named "_mac" and the encoder isn't? I realize there are some inconsistency in this, and unfortunately I couldn't find a straight answer from style guides. But looking over this folder, vt_video_decode_accelerator_mac and dxva_video_decode_accelerator_win has platform suffixes. Although it is CrOS specific, v4l2_video_decode_accelerator doesn't have a suffix. android_video_decode_accelerator is a prefix. The ideal solution would be to have platform folders in this directory and remove all platform suffix/prefixs from the names. So I decided to go in that direction as I am planning to touch more files in this directory soon, but I am open to suggestions as well. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:1098: }; On 2016/02/03 18:21:08, hbos_chromium wrote: > It seems unlikely that VTVEA would be used, this only happens if all the other > encoders fail to initialize? What is VEAClient::CreateEncoder() used for? Since CreateV4L2VEA() and CreateVaapiVEA() are CrOS specific, they would be skipped. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:35: }; On 2016/02/03 20:01:31, mcasas wrote: > Recommend a > private: > DISALLOW_IMPLICIT_CONSTRUCTORS(InProgressFrameEncode); Done. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:47: }; On 2016/02/03 20:01:31, mcasas wrote: > Same here, use DISALLOW_IMPLICIT_CONSTRUCTORS Done. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:90: const bool session_created = ResetCompressionSession(); On 2016/02/03 20:01:31, mcasas wrote: > No need for temporary? Done. I was trying to be verbose. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:96: client_ptr_factory_.reset(new base::WeakPtrFactory<Client>(client)); On 2016/02/03 20:01:31, mcasas wrote: > This |client_ptr_factory_| is only used here, > do you need to hold on to it? I can invalidate the weakptrs in Destroy() using it as a class member. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:103: input_visible_size_.GetArea() / kOutputBufferSizeRatio)); On 2016/02/03 18:21:08, hbos_chromium wrote: > About thread-safety, how is this class used across threads? Is > |child_task_runner_| not always the current thread? Is it used on multiple > threads and if not, why PostTask? Thanks for pointing it out. Actually, I don't need to PostTask here. All the threads in this class run on |child_task_runner_| except VTVideoEncodeAccelerator::CompressionCallback() where I still need to post a task. I realize I was inconsistent with thread_checker so consistently checking it in every method as well. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:128: // CVPixelBufferPool for the allocation of incoming video frames. On 2016/02/03 20:01:31, mcasas wrote: > nit: s/video frames/VideoFrames/? Done. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:169: if (compression_session_) { On 2016/02/03 20:01:31, mcasas wrote: > nit: > if (!compression_session_) > return; > ... Done. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:169: if (compression_session_) { On 2016/02/03 18:21:08, hbos_chromium wrote: > Does it make sense to DCHECK instead? Acknowledged. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.cc:233: // TODO(emircan): Investigate IOS constraints. On 2016/02/03 20:01:31, mcasas wrote: > ? Removed. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... File content/common/gpu/media/vt_video_encode_accelerator.h (right): https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.h:42: typedef CoreMediaGlue::CMSampleBufferRef CMSampleBufferRef; On 2016/02/03 20:01:31, mcasas wrote: > using CMSampleBufferRef = CoreMediaGlue::CMSampleBufferRef; > here and in the next 2 lines. Done. https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.h:79: const scoped_refptr<base::SingleThreadTaskRunner> child_task_runner_; On 2016/02/03 20:01:31, mcasas wrote: > nit: s/child_task_runner_/callback_task_runner_/ ? callback might be confusing since there are callbacks to VTSession. I will go with client_task_runner_? https://codereview.chromium.org/1636083003/diff/80001/content/common/gpu/medi... content/common/gpu/media/vt_video_encode_accelerator.h:83: // child_task_runner_. On 2016/02/03 20:01:31, mcasas wrote: > |child_task_runner_| Done.
not lgtm -- Please re-upload using `git cl upload --similarity=10` so that we can see the actual differences, and also to preserve change history in the code repository. (There's hundreds of LOC here, most of which haven't changed, and I don't want to have to re-review all that.) Also, to help the reviewer, please restore the original ordering of the methods and helper functions (e.g., in media/cast/sender/h264_vt_encoder.cc the DictionaryWithKeysAndValues() function came before the others; but when it was moved to media/base/mac/videotoolbox_helpers.cc, it appears near the bottom). Thanks! :)
On 2016/02/04 20:35:27, miu wrote: > not lgtm -- Please re-upload using `git cl upload --similarity=10` so that we > can see the actual differences, and also to preserve change history in the code > repository. (There's hundreds of LOC here, most of which haven't changed, and I > don't want to have to re-review all that.) Also, to help the reviewer, please > restore the original ordering of the methods and helper functions (e.g., in > media/cast/sender/h264_vt_encoder.cc the DictionaryWithKeysAndValues() function > came before the others; but when it was moved to > media/base/mac/videotoolbox_helpers.cc, it appears near the bottom). Thanks! > :) Thanks for pointing out. I reuploaded a patch with these done.
https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:18: const size_t kNumInputBuffers = 4; IIRC, the VT encoder can hold onto several buffers at a time to help optimize motion prediction. I have no idea how many buffers it actually *will* hold onto (1, 2, 8, or something else). Suggest you consult documentation for this and/or jfroy@. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:19: const size_t kMaxFrameRateNumerator = 30; Is 30 really the max for all hardware solutions? I'd imagine the max framerate to be machine-dependent, and also dependent on the resolution. Furthermore, there are likely some hardware encoders that cannot do 30 FPS at 4K resolution. If anything, you should put a TODO+crbug here to query the platform for supported profiles rather than hard-code these. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:23: const size_t kOutputBufferSizeRatio = 10; Please document this. What does 10 mean? https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:122: (ref_time - base::TimeTicks()).InMicroseconds(), USEC_PER_SEC); IIUC, this should be frame->timestamp(), not (ref_time - base::TimeTicks()). Though, I see it's that way in the cast code as well. Can you fix it there too? :) https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:201: return; You can't return early anywhere in this function. Client::BitstreamBufferReady() must be called to release the buffer back to the client. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:210: if (!encoder) { This should never be null. DCHECK(!encoder) would suffice here. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:215: DLOG(ERROR) << "No more bitstream buffer to encode into."; You should call Client::NotifyError() and either: 1) shutdown the encoder; or 2) recover from this error by requiring a key frame be encoded next. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:228: memcpy(buffer_ref->shm->memory(), annexb_buffer.data(), Please remove this redundant memcpy(). Just provide buffer_ref->shm->memory() as the 2nd argument in the call to media::CopySampleBufferToAnnexBBuffer() above. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:246: ->kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder(), 80 chars https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:296: media::SetSessionProperty( Suggestion: Instead of passing compression_session_ and videotoolbox_glue_ to every function call, perhaps the SetSessionProperty() methods could be in a simple class? Example: media::video_toolbox::SessionPropertySetter setter(compression_session_, videotoolbox_glue_); setter.Set(videotoolbox_glue_->kVTCompressionPropertyKey_ProfileLevel(), videotoolbox_glue_->kVTProfileLevel_H264_Baseline_AutoLevel()); setter.Set(...); setter.Set(...); setter.Set(...); https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.h (left): https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:76: void UpdateFrameSize(const gfx::Size& size_needed); It's unfortunate that VEA only works with fixed input frame sizes. :( https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.h (right): https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:70: // Initial parameters given by media::VideoEncodeAccelerator::Initialize() This comment is not accurate: |bitrate_| may change repeatedly during a session. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:71: uint32_t bitrate_; Type should be int. Unsigneds are for index counters (size_t) and bit twiddling. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:76: std::deque<scoped_ptr<BitstreamBufferRef>> encoder_output_queue_; nit: If this is a LIFO, seems like you want a std::stack. https://codereview.chromium.org/1636083003/diff/120001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/120001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:15: DictionaryWithKeysAndValues(CFTypeRef* keys, CFTypeRef* values, size_t size); All of these functions are VT-specific, but have rather generalized-sounding names. Could you put everything in this file in, say, a video_toolbox namespace? namespace media { namespace video_toolbox { ...functions... } // namespace video_toolbox } // namespace media https://codereview.chromium.org/1636083003/diff/120001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:15: DictionaryWithKeysAndValues(CFTypeRef* keys, CFTypeRef* values, size_t size); Also, throughout this file, can you please add a comment to explain what each function does? https://codereview.chromium.org/1636083003/diff/120001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:34: using VTCompressionSessionRef = VideoToolboxGlue::VTCompressionSessionRef; style guide violation: Please remove this. Better to be more specific and use the fully-qualified name in header files, anyways. ;) https://codereview.chromium.org/1636083003/diff/120001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/1636083003/diff/120001/media/media.gyp#newcod... media/media.gyp:340: 'base/mac/videotoolbox_helpers.cc', These should only be built and linked on MacOS or iOS, right? The BUILD.gn file seems to do this, so maybe these should be under the corresponding GYP target?
emircan@chromium.org changed reviewers: + jfroy@chromium.org - posciak@chromium.org
https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:18: const size_t kNumInputBuffers = 4; On 2016/02/04 22:23:20, miu wrote: > IIRC, the VT encoder can hold onto several buffers at a time to help optimize > motion prediction. I have no idea how many buffers it actually *will* hold onto > (1, 2, 8, or something else). Suggest you consult documentation for this and/or > jfroy@. I discussed with jfroy@ and added him as a reviewer. Unfortunately, in theory VT can hold onto as much buffers as it decides to. I couldn't find any information about it, and it doesn't look like something we can easily query. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:19: const size_t kMaxFrameRateNumerator = 30; On 2016/02/04 22:23:20, miu wrote: > Is 30 really the max for all hardware solutions? I'd imagine the max framerate > to be machine-dependent, and also dependent on the resolution. Furthermore, > there are likely some hardware encoders that cannot do 30 FPS at 4K resolution. > > If anything, you should put a TODO+crbug here to query the platform for > supported profiles rather than hard-code these. I added a TODO for querying platform capabilities. Unfortunately, there isn't an easy way to query these. One way we discussed with jfroy@ is to create sessions but that would definitely effect the initialize time. I will look out for better solution and leave it as a TODO for now. Coming to framerate and resolution; I agree that in theory there should be a tradeoff. VTCompressionSession provides a parameter kVTCompressionPropertyKey_ExpectedFrameRate, but there is nothing that I found regarding resolution. Even if kVTCompressionPropertyKey_ExpectedFrameRate is supported for 60, I am doubtful if it would do 4K 60 fps. So, I am just hardcording max capabilities for now that we return in VTVideoEncodeAccelerator::GetSupportedProfiles(). https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:23: const size_t kOutputBufferSizeRatio = 10; On 2016/02/04 22:23:20, miu wrote: > Please document this. What does 10 mean? Done. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:122: (ref_time - base::TimeTicks()).InMicroseconds(), USEC_PER_SEC); On 2016/02/04 22:23:20, miu wrote: > IIUC, this should be frame->timestamp(), not (ref_time - base::TimeTicks()). > Though, I see it's that way in the cast code as well. Can you fix it there too? > :) Done. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:201: return; On 2016/02/04 22:23:20, miu wrote: > You can't return early anywhere in this function. > Client::BitstreamBufferReady() must be called to release the buffer back to the > client. I am holding onto a BitstreamBuffer from the queue on l.218. If we early return here, the next callback can use it, and it should be fine? https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:210: if (!encoder) { On 2016/02/04 22:23:20, miu wrote: > This should never be null. DCHECK(!encoder) would suffice here. Done. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:215: DLOG(ERROR) << "No more bitstream buffer to encode into."; On 2016/02/04 22:23:20, miu wrote: > You should call Client::NotifyError() and either: 1) shutdown the encoder; or 2) > recover from this error by requiring a key frame be encoded next. I added NotifyError call. However, I don't understand why we would need to shutdown encoder or require a keyframe. AFAIU, other encoders aren't doing that: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:228: memcpy(buffer_ref->shm->memory(), annexb_buffer.data(), On 2016/02/04 22:23:20, miu wrote: > Please remove this redundant memcpy(). Just provide buffer_ref->shm->memory() > as the 2nd argument in the call to media::CopySampleBufferToAnnexBBuffer() > above. media::CopySampleBufferToAnnexBBuffer() expects a string input and reserves buffer in the flights, so it isn't as simple. I was trying to leave it for a follow-up patch, but since it came up I am doing it. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:246: ->kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder(), On 2016/02/04 22:23:20, miu wrote: > 80 chars Done. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:296: media::SetSessionProperty( On 2016/02/04 22:23:20, miu wrote: > Suggestion: Instead of passing compression_session_ and videotoolbox_glue_ to > every function call, perhaps the SetSessionProperty() methods could be in a > simple class? Example: > > media::video_toolbox::SessionPropertySetter setter(compression_session_, > videotoolbox_glue_); > setter.Set(videotoolbox_glue_->kVTCompressionPropertyKey_ProfileLevel(), > videotoolbox_glue_->kVTProfileLevel_H264_Baseline_AutoLevel()); > setter.Set(...); > setter.Set(...); > setter.Set(...); > Done. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.h (left): https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:76: void UpdateFrameSize(const gfx::Size& size_needed); On 2016/02/04 22:23:20, miu wrote: > It's unfortunate that VEA only works with fixed input frame sizes. :( It might be a good fit for V4L2 and VAAPI but VideoToolbox would benefit from this. Maybe a future TODO about this. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.h (right): https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:70: // Initial parameters given by media::VideoEncodeAccelerator::Initialize() On 2016/02/04 22:23:20, miu wrote: > This comment is not accurate: |bitrate_| may change repeatedly during a session. Removed it. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:71: uint32_t bitrate_; On 2016/02/04 22:23:20, miu wrote: > Type should be int. Unsigneds are for index counters (size_t) and bit > twiddling. Done. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:76: std::deque<scoped_ptr<BitstreamBufferRef>> encoder_output_queue_; On 2016/02/04 22:23:20, miu wrote: > nit: If this is a LIFO, seems like you want a std::stack. Actually it is a FIFO at this point, changing the comment. https://codereview.chromium.org/1636083003/diff/120001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/120001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:15: DictionaryWithKeysAndValues(CFTypeRef* keys, CFTypeRef* values, size_t size); On 2016/02/04 22:23:20, miu wrote: > All of these functions are VT-specific, but have rather generalized-sounding > names. Could you put everything in this file in, say, a video_toolbox > namespace? > > namespace media { > namespace video_toolbox { > ...functions... > } // namespace video_toolbox > } // namespace media Done. https://codereview.chromium.org/1636083003/diff/120001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:15: DictionaryWithKeysAndValues(CFTypeRef* keys, CFTypeRef* values, size_t size); On 2016/02/04 22:23:20, miu wrote: > Also, throughout this file, can you please add a comment to explain what each > function does? Done. https://codereview.chromium.org/1636083003/diff/120001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:34: using VTCompressionSessionRef = VideoToolboxGlue::VTCompressionSessionRef; On 2016/02/04 22:23:20, miu wrote: > style guide violation: Please remove this. Better to be more specific and use > the fully-qualified name in header files, anyways. ;) Done.
posciak@chromium.org changed reviewers: + posciak@chromium.org
https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:657: decoder_(new media::FFmpegVideoDecoder()), Did you mean ffmpeg was just not compiled/working on Mac at all, or that it would fail for some other reason? I thought ffmpeg worked on Mac... https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1713: #else Even if framerate changes are not supported, perhaps we could still test bitrate changes? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:33: struct InProgressFrameEncode { Could this be a private substruct of VTVEA? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:48: scoped_ptr<base::SharedMemory> shm, const& perhaps? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:60: : videotoolbox_glue_(VideoToolboxGlue::Get()), We used to check this for != nullptr (https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/...). Perhaps it would be safer to still do this? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:96: DCHECK_EQ(media::PIXEL_FORMAT_I420, format); I'd suggest if()s and careful check for argument values. VEA is a Pepper interface too and there may be other clients that could try to create us with various arguments and depend on us to fail Initialize(). https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:97: DCHECK_EQ(media::H264PROFILE_BASELINE, output_profile); I'd prefer we if()'d profile here nevertheless. There is no requirement to check supported profiles and some clients may not do this, relying on Initialize() to fail. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:107: client_ptr_factory_.reset(new base::WeakPtrFactory<Client>(client)); I'd suggest doing this before calling any methods, just in case (for later changes in this class). https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:148: DLOG_IF(ERROR, status != noErr) No need to NOTIFY_ERROR? Can we continue from next Encode() without reinitialization after VTCompressionSessionEncodeFrame() fails? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:156: DCHECK_GE(buffer.size(), static_cast<size_t>(input_visible_size_.GetArea() / I think it'd be better to if() here please. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:162: DLOG(ERROR) << "Failed mapping shared memory."; NOTIFY_ERROR? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:168: encoder_output_queue_.push_back(std::move(buffer_ref)); Do we need to wake something up here? If we got Encode() first, but no output buffers are ready at that time, we would stall? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:173: uint32_t framerate) { If this class cannot handle changing framerate, should we return an error if such a change is requested? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:177: bitrate_ = bitrate; We should preferably check input values here. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:178: if (!compression_session_) NOTIFY_ERROR? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:181: session_property_setter_->SetSessionProperty( SetSessionProperty() methods are bool, but we are not checking return values, here and in other places... https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:195: void VTVideoEncodeAccelerator::CompressionCallback(void* encoder_opaque, Do we know what thread this is called on? Should we DCHECK(encoder->thread_checker_.CalledOnValidThread()) ? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:203: DLOG(ERROR) << " encode failed: " << status; NOTIFY_ERROR? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:218: DLOG(ERROR) << "No more bitstream buffer to encode into."; Please see my previous comment, but I think this could happen in normal cases if Client called Encode() first and then UseOutputBitstreamBuffer(). This should be an acceptable case I feel though and we should instead just not encode from Encode(), but once an input:output buffer pair is available, either from Encode() or UseOutputBitstreamBuffer() ? Or, perhaps even better, if possible, always encode in Encode(), but defer this copy here if encoder_output_queue_.empty(), putting sbuf on some kind of a queue and copying after next UseOutputBitstreamBuffer()... https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:238: // This method is NOT called on |client_task_runner_|, so we still need to Uhh, if so, I don't think we can access encoder at all, including members like encoder_output_queue_. Aren't we pushing to encoder_output_queue_ on client_task_runner_ for example? If that's the case, could we trampoline from this method at the beginning to client_task_runner_ to actually handle things there? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:255: // Keep these in-sync with those in ConfigureSession(). Where is ConfigureSession() ? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:265: const int format[] = { Is this a fourcc? If so uint32_t please. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:308: videotoolbox_glue_->kVTProfileLevel_H264_Baseline_AutoLevel()); Is baseline preferred by us? Normally constrained baseline is more compatible with various devices (unless this actually is constrained). Main may also be better in general. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.h (right): https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:17: // interface for MacOSX. Should we keep threading comment, or is VT no longer non-thread-safe? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:40: struct BitstreamBufferRef; Could this be private? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:71: int32_t bitrate_; uint32_t?
https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:657: decoder_(new media::FFmpegVideoDecoder()), On 2016/02/08 04:33:42, Pawel Osciak wrote: > Did you mean ffmpeg was just not compiled/working on Mac at all, or that it > would fail for some other reason? I thought ffmpeg worked on Mac... It points to this line when built: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu.... Do you know if it is still the case? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1713: #else On 2016/02/08 04:33:42, Pawel Osciak wrote: > Even if framerate changes are not supported, perhaps we could still test bitrate > changes? Actually, bitrate code isn't doing much as VT does not allow reconfiguration while in session. See https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... and https://code.google.com/p/chromium/issues/detail?id=425352 . I will add a comment regarding this in that section as well. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:33: struct InProgressFrameEncode { On 2016/02/08 04:33:42, Pawel Osciak wrote: > Could this be a private substruct of VTVEA? Done. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:48: scoped_ptr<base::SharedMemory> shm, On 2016/02/08 04:33:42, Pawel Osciak wrote: > const& perhaps? I cannot use std::move on const scoped_ptr&. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:60: : videotoolbox_glue_(VideoToolboxGlue::Get()), On 2016/02/08 04:33:43, Pawel Osciak wrote: > We used to check this for != nullptr > (https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/...). > Perhaps it would be safer to still do this? Hmm we dont have a similar IsSupported() call though. I will move it to Initialize() and fail if it is nullptr. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:96: DCHECK_EQ(media::PIXEL_FORMAT_I420, format); On 2016/02/08 04:33:42, Pawel Osciak wrote: > I'd suggest if()s and careful check for argument values. VEA is a Pepper > interface too and there may be other clients that could try to create us with > various arguments and depend on us to fail Initialize(). Done. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:97: DCHECK_EQ(media::H264PROFILE_BASELINE, output_profile); On 2016/02/08 04:33:42, Pawel Osciak wrote: > I'd prefer we if()'d profile here nevertheless. There is no requirement to check > supported profiles and some clients may not do this, relying on Initialize() to > fail. Done. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:107: client_ptr_factory_.reset(new base::WeakPtrFactory<Client>(client)); On 2016/02/08 04:33:42, Pawel Osciak wrote: > I'd suggest doing this before calling any methods, just in case (for later > changes in this class). Ok, moving it right after the initial ifs. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:148: DLOG_IF(ERROR, status != noErr) On 2016/02/08 04:33:42, Pawel Osciak wrote: > No need to NOTIFY_ERROR? Can we continue from next Encode() without > reinitialization after VTCompressionSessionEncodeFrame() fails? Thanks for pointing out. I realize I haven't used enough NotifyErrors. Adding them now to wherever appropriate. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:156: DCHECK_GE(buffer.size(), static_cast<size_t>(input_visible_size_.GetArea() / On 2016/02/08 04:33:42, Pawel Osciak wrote: > I think it'd be better to if() here please. Done. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:162: DLOG(ERROR) << "Failed mapping shared memory."; On 2016/02/08 04:33:42, Pawel Osciak wrote: > NOTIFY_ERROR? Done. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:168: encoder_output_queue_.push_back(std::move(buffer_ref)); On 2016/02/08 04:33:42, Pawel Osciak wrote: > Do we need to wake something up here? If we got Encode() first, but no output > buffers are ready at that time, we would stall? Replied to the later comment. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:173: uint32_t framerate) { On 2016/02/08 04:33:42, Pawel Osciak wrote: > If this class cannot handle changing framerate, should we return an error if > such a change is requested? Actually both changes aren't supported, but at least bitrate has an input param for session creation. Restarting the session with the new bitrate param should work, but that wouldn't be a cheap step. I will visit this issue in a later CL. If we were to return error, how would the clients act accordingly? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:177: bitrate_ = bitrate; On 2016/02/08 04:33:42, Pawel Osciak wrote: > We should preferably check input values here. What kind of checks? I found (bitrate < 1) checks from CrOS VEAs. Would that be enough? https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:178: if (!compression_session_) On 2016/02/08 04:33:42, Pawel Osciak wrote: > NOTIFY_ERROR? Done. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:181: session_property_setter_->SetSessionProperty( On 2016/02/08 04:33:42, Pawel Osciak wrote: > SetSessionProperty() methods are bool, but we are not checking return values, > here and in other places... I will go through them. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:195: void VTVideoEncodeAccelerator::CompressionCallback(void* encoder_opaque, On 2016/02/08 04:33:42, Pawel Osciak wrote: > Do we know what thread this is called on? > > Should we DCHECK(encoder->thread_checker_.CalledOnValidThread()) ? No. To quote the documentation: "This function may be called asynchronously, on a different thread from the one that calls VTCompressionSessionEncodeFrame" https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:203: DLOG(ERROR) << " encode failed: " << status; On 2016/02/08 04:33:43, Pawel Osciak wrote: > NOTIFY_ERROR? Done. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:218: DLOG(ERROR) << "No more bitstream buffer to encode into."; On 2016/02/08 04:33:42, Pawel Osciak wrote: > Please see my previous comment, but I think this could happen in normal cases if > Client called Encode() first and then UseOutputBitstreamBuffer(). This should be > an acceptable case I feel though and we should instead just not encode from > Encode(), but once an input:output buffer pair is available, either from > Encode() or UseOutputBitstreamBuffer() ? > > Or, perhaps even better, if possible, always encode in Encode(), but defer this > copy here if encoder_output_queue_.empty(), putting sbuf on some kind of a queue > and copying after next UseOutputBitstreamBuffer()... Thanks for pointing out. I didn't know Encode() and then UseOutputBitstreamBuffer() is a valid case. I like the second option better, and adding a new queue to retain those buffers. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:238: // This method is NOT called on |client_task_runner_|, so we still need to On 2016/02/08 04:33:42, Pawel Osciak wrote: > Uhh, if so, I don't think we can access encoder at all, including members like > encoder_output_queue_. Aren't we pushing to encoder_output_queue_ on > client_task_runner_ for example? > > If that's the case, could we trampoline from this method at the beginning to > client_task_runner_ to actually handle things there? I am posting a task as you suggest. But why wouldn't we access encoder at all? Accessing client_task_runner through encoder is also done on cast code: https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... . If your concern is regarding thread safety of std::deque, I checked it prior and found mixed answers regarding it. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:255: // Keep these in-sync with those in ConfigureSession(). On 2016/02/08 04:33:42, Pawel Osciak wrote: > Where is ConfigureSession() ? Changed it to ConfigureCompressionSession(). https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:265: const int format[] = { On 2016/02/08 04:33:42, Pawel Osciak wrote: > Is this a fourcc? If so uint32_t please. We need to create CFArrayRef<int> from it. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:308: videotoolbox_glue_->kVTProfileLevel_H264_Baseline_AutoLevel()); On 2016/02/08 04:33:42, Pawel Osciak wrote: > Is baseline preferred by us? Normally constrained baseline is more compatible > with various devices (unless this actually is constrained). Main may also be > better in general. I discussed this with WebRTC people. I learned that Baseline is the only one that openh264 supports -SW case- and compatible with Firefox as well. I am planning to support only Baseline in this CL. I can add other profiles after further discussion, and it wouldn't be hard. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.h (right): https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:17: // interface for MacOSX. On 2016/02/08 04:33:43, Pawel Osciak wrote: > Should we keep threading comment, or is VT no longer non-thread-safe? I will keep the threading comment. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:40: struct BitstreamBufferRef; On 2016/02/08 04:33:43, Pawel Osciak wrote: > Could this be private? Done. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:71: int32_t bitrate_; On 2016/02/08 04:33:43, Pawel Osciak wrote: > uint32_t? It was uint32_t but miu@ asked me to change it on PS#3.
Patchset #5 (id:160001) has been deleted
https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:201: return; On 2016/02/07 04:24:04, emircan wrote: > On 2016/02/04 22:23:20, miu wrote: > > You can't return early anywhere in this function. > > Client::BitstreamBufferReady() must be called to release the buffer back to > the > > client. > > I am holding onto a BitstreamBuffer from the queue on l.218. If we early return > here, the next callback can use it, and it should be fine? I could be misunderstanding the intended semantics of the VEA API, but if the client sends a frame to VEA for encoding and the encoder decides to drop the frame, shouldn't the client be made aware of that fact? The client might need to do something to account for that. Also, the client may be assuming that for a sequence of calls to VEA::Encode(), say for frame #1, #2, #3, etc.; the BitstreamBufferReady() method would be invoked for those frames in the same ordering. Meaning, the client would assume the first BitstreamBufferReady() call is the result for frame #1, the next one for frame #2, and so on. FWIW, the media::cast::ExternalVideoEncoder (which uses VEA) makes this assumption. https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:215: DLOG(ERROR) << "No more bitstream buffer to encode into."; On 2016/02/07 04:24:05, emircan wrote: > On 2016/02/04 22:23:20, miu wrote: > > You should call Client::NotifyError() and either: 1) shutdown the encoder; or > 2) > > recover from this error by requiring a key frame be encoded next. > > I added NotifyError call. However, I don't understand why we would need to > shutdown encoder or require a keyframe. AFAIU, other encoders aren't doing that: > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... When decoding frames, there are implicit dependencies on prior frames. So, if the decoder is missing a P-frame or B-frame, it will not be able to continue decoding until it receives an I-frame (a key frame). One suggestion for solving this problem in your code here: Instead of calling encoder_output_queue_.pop_front() here, perhaps you should make that call *before* starting the encode (i.e., where you create the InProgressFrameEncode struct). That way, you know before you even start encoding the frame whether you will have an output buffer available. In other words, you'd be "reserving" the output buffer for a frame upfront. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.h (right): https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.h:71: int32_t bitrate_; On 2016/02/08 23:41:24, emircan wrote: > On 2016/02/08 04:33:43, Pawel Osciak wrote: > > uint32_t? > > It was uint32_t but miu@ asked me to change it on PS#3. Chromium C++ style guide discusses when and when not to use unsigned types: https://www.chromium.org/developers/coding-style#TOC-Types In this case, it seems to me it was being used for "documentative purposes" which the style guide says not to do. (That, and there was a cast to a signed value where |bitrate_| is being passed to a VT API call.) https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:30: const size_t kOutputBufferSizeRatio = 10; Instead of a constant, shouldn't this depend on the configured bit rate? e.g., 2 Mbps versus 20 Mbps https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:160: reinterpret_cast<void*>(request.release()), nullptr); I could be mistaken, but it seems memory is being leaked here. What uses/destroys the InProgressFrameEncode object? https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:378: session_property_setter_.reset( This shouldn't be allocated for the lifetime of the class, since it's only used briefly in this method (which is not being called very often). Instead, just make it a local stack variable: video_toolbox::SessionPropertySetter setter(...); setter.SetSessionProperty(...); ... https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right): https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.h:80: scoped_ptr<media::video_toolbox::SessionPropertySetter> Please remove (see comment in .cc file). https://codereview.chromium.org/1636083003/diff/200001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/200001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:31: MEDIA_EXPORT bool CopySampleBufferToAnnexBBuffer( For simplicity, you should delete the version of this function that takes a std::string output. Client code can just pre-allocate the string and truncate it afterwards, like so: std::string annexb_buffer(kMaxAnnexBSize, '\0'); size_t used_size = 0; CopySampleBufferToAnnex(sbuf, reinterpret_cast<uint8_t*>(annexb_buffer.data()), kMaxAnnexBSize, keyframe, &used_size); annexb_buffer.resize(used_size); Then, you don't need that extra implementation in the .cc file to handle both string and uint8_t* output buffer cases. https://codereview.chromium.org/1636083003/diff/200001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:37: uint8_t* annexb_buffer, style: output arguments go last https://codereview.chromium.org/1636083003/diff/200001/media/cast/sender/h264... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/1636083003/diff/200001/media/cast/sender/h264... media/cast/sender/h264_vt_encoder.cc:302: session_property_setter_.reset(new video_toolbox::SessionPropertySetter( ditto here: Please use local variable instead. https://codereview.chromium.org/1636083003/diff/200001/media/cast/sender/h264... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/1636083003/diff/200001/media/cast/sender/h264... media/cast/sender/h264_vt_encoder.h:123: scoped_ptr<video_toolbox::SessionPropertySetter> session_property_setter_; Please remove (see comment in .cc file).
emircan@chromium.org changed reviewers: - jfroy@chromium.org
https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:201: return; On 2016/02/09 23:29:22, miu wrote: > On 2016/02/07 04:24:04, emircan wrote: > > On 2016/02/04 22:23:20, miu wrote: > > > You can't return early anywhere in this function. > > > Client::BitstreamBufferReady() must be called to release the buffer back to > > the > > > client. > > > > I am holding onto a BitstreamBuffer from the queue on l.218. If we early > return > > here, the next callback can use it, and it should be fine? > > I could be misunderstanding the intended semantics of the VEA API, but if the > client sends a frame to VEA for encoding and the encoder decides to drop the > frame, shouldn't the client be made aware of that fact? The client might need > to do something to account for that. > > Also, the client may be assuming that for a sequence of calls to VEA::Encode(), > say for frame #1, #2, #3, etc.; the BitstreamBufferReady() method would be > invoked for those frames in the same ordering. Meaning, the client would assume > the first BitstreamBufferReady() call is the result for frame #1, the next one > for frame #2, and so on. FWIW, the media::cast::ExternalVideoEncoder (which > uses VEA) makes this assumption. I see the sequence assumption. Cast VT code seems to continue and send back an empty output buffer -payload_size=0- for that case, and I will follow that logic. https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... https://codereview.chromium.org/1636083003/diff/120001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:215: DLOG(ERROR) << "No more bitstream buffer to encode into."; On 2016/02/09 23:29:22, miu wrote: > On 2016/02/07 04:24:05, emircan wrote: > > On 2016/02/04 22:23:20, miu wrote: > > > You should call Client::NotifyError() and either: 1) shutdown the encoder; > or > > 2) > > > recover from this error by requiring a key frame be encoded next. > > > > I added NotifyError call. However, I don't understand why we would need to > > shutdown encoder or require a keyframe. AFAIU, other encoders aren't doing > that: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > When decoding frames, there are implicit dependencies on prior frames. So, if > the decoder is missing a P-frame or B-frame, it will not be able to continue > decoding until it receives an I-frame (a key frame). > > One suggestion for solving this problem in your code here: Instead of calling > encoder_output_queue_.pop_front() here, perhaps you should make that call > *before* starting the encode (i.e., where you create the InProgressFrameEncode > struct). That way, you know before you even start encoding the frame whether > you will have an output buffer available. In other words, you'd be "reserving" > the output buffer for a frame upfront. I changed this behavior after posciak@ comments. I am now pushing those to a queue and waiting for the next BitstreamBuffer. https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:30: const size_t kOutputBufferSizeRatio = 10; On 2016/02/09 23:29:22, miu wrote: > Instead of a constant, shouldn't this depend on the configured bit rate? e.g., > 2 Mbps versus 20 Mbps I tried doing that but it would be very dependent on initial_bitrate, since we request for Bitstream buffers once only in Initialize(). I will take max of these two values to be safe. WDYT? https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:160: reinterpret_cast<void*>(request.release()), nullptr); On 2016/02/09 23:29:22, miu wrote: > I could be mistaken, but it seems memory is being leaked here. What > uses/destroys the InProgressFrameEncode object? Thanks for pointing out. I will make a scoped_ptr to release it properly in the callback. https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:378: session_property_setter_.reset( On 2016/02/09 23:29:22, miu wrote: > This shouldn't be allocated for the lifetime of the class, since it's only used > briefly in this method (which is not being called very often). Instead, just > make it a local stack variable: > > video_toolbox::SessionPropertySetter setter(...); > setter.SetSessionProperty(...); > ... It is used on l.233 as well. Following example: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... . https://codereview.chromium.org/1636083003/diff/200001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/200001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:31: MEDIA_EXPORT bool CopySampleBufferToAnnexBBuffer( On 2016/02/09 23:29:22, miu wrote: > For simplicity, you should delete the version of this function that takes a > std::string output. Client code can just pre-allocate the string and truncate > it afterwards, like so: > > std::string annexb_buffer(kMaxAnnexBSize, '\0'); > size_t used_size = 0; > CopySampleBufferToAnnex(sbuf, > reinterpret_cast<uint8_t*>(annexb_buffer.data()), > kMaxAnnexBSize, > keyframe, > &used_size); > annexb_buffer.resize(used_size); > > Then, you don't need that extra implementation in the .cc file to handle both > string and uint8_t* output buffer cases. But wouldn't it be costly to do "std::string annexb_buffer(kMaxAnnexBSize, '\0')" before each time? Also, I am not sure what to set a kMaxAnnexBSize in cast code that is always big enough. We are expecting to lose some frames in VTVEA when BitstreamBuffer size isn't big enough. https://codereview.chromium.org/1636083003/diff/200001/media/cast/sender/h264... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/1636083003/diff/200001/media/cast/sender/h264... media/cast/sender/h264_vt_encoder.cc:302: session_property_setter_.reset(new video_toolbox::SessionPropertySetter( On 2016/02/09 23:29:22, miu wrote: > ditto here: Please use local variable instead. Done. https://codereview.chromium.org/1636083003/diff/200001/media/cast/sender/h264... File media/cast/sender/h264_vt_encoder.h (right): https://codereview.chromium.org/1636083003/diff/200001/media/cast/sender/h264... media/cast/sender/h264_vt_encoder.h:123: scoped_ptr<video_toolbox::SessionPropertySetter> session_property_setter_; On 2016/02/09 23:29:22, miu wrote: > Please remove (see comment in .cc file). Done.
jfroy@chromium.org changed reviewers: + jfroy@chromium.org
https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:20: const size_t kNumInputBuffers = 1; This is somewhat configurable using kVTCompressionPropertyKey_MaxFrameDelayCount. It's the maximum number of input buffers the compression can buffer until it emits a frame. It defaults to "unlimited", but you can set it to a known value and then report that as your limit. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:96: if (media::PIXEL_FORMAT_I420 != format) { You can also support NV12 pretty easily. I don't know if that's a common format in Chrome. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:232: // TODO(emircan): VideoToolbox does not seem to support bitrate See my update to that bug. You can actually control the bitrate at runtime using 2 properties. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:306: auto sample_attachments = static_cast<CFDictionaryRef>(CFArrayGetValueAtIndex( This code seems duplicated from UseOutputBitstreamBuffer(). It would be good to refactor it, since it's a bit subtle. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:339: ->kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder(), Unless you want to fallback to Apple's *TERRIBLE* software encoder, I suggest you also specify RequireHardwareAcceleratedVideoEncoder. If you can't use the hardware encoder, you'd be much better served falling back on Chromium's software encoder.
https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:30: const size_t kOutputBufferSizeRatio = 10; On 2016/02/10 05:21:53, emircan wrote: > On 2016/02/09 23:29:22, miu wrote: > > Instead of a constant, shouldn't this depend on the configured bit rate? > e.g., > > 2 Mbps versus 20 Mbps > > I tried doing that but it would be very dependent on initial_bitrate, since we > request for Bitstream buffers once only in Initialize(). I will take max of > these two values to be safe. WDYT? Sounds fine. I'll leave it to the VEA owners to work out the larger issue (affecting all VEA's) in some future change. https://codereview.chromium.org/1636083003/diff/200001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:378: session_property_setter_.reset( On 2016/02/10 05:21:53, emircan wrote: > On 2016/02/09 23:29:22, miu wrote: > > This shouldn't be allocated for the lifetime of the class, since it's only > used > > briefly in this method (which is not being called very often). Instead, just > > make it a local stack variable: > > > > video_toolbox::SessionPropertySetter setter(...); > > setter.SetSessionProperty(...); > > ... > > It is used on l.233 as well. Following example: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > . The point is that the optimizing compiler can do a lot to eliminate extra storage, skip steps w.r.t. construction/destruction, etc. when the class is a local variable. When it's heap-allocated, it can't. Also, by making this a data member, memory is being wasted to hold an object that is only rarely and briefly used. https://codereview.chromium.org/1636083003/diff/200001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/200001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:31: MEDIA_EXPORT bool CopySampleBufferToAnnexBBuffer( On 2016/02/10 05:21:53, emircan wrote: > On 2016/02/09 23:29:22, miu wrote: > > For simplicity, you should delete the version of this function that takes a > > std::string output. Client code can just pre-allocate the string and truncate > > But wouldn't it be costly to do "std::string annexb_buffer(kMaxAnnexBSize, > '\0')" before each time? Also, I am not sure what to set a kMaxAnnexBSize in > cast code that is always big enough. We are expecting to lose some frames in > VTVEA when BitstreamBuffer size isn't big enough. In terms of run-time cost: No, because it's still just one allocation of a chunk of memory. In terms of space efficiency: It should be no less efficient than what client code has to do to pre-allocate a chunk of memory for the raw pointer version of this function. Whoops, maybe not kMaxAnnexBSize for the size. jfroy@, do you know? https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1714: INSTANTIATE_TEST_CASE_P( BTW--Will these tests run in the waterfall? IIRC, this module's tests are currently only configured to run on ChromeOS bots. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:126: bitrate_ / kBitsPerByte)); Sanity-check: Is the bitrate_ setting a "max bitrate" or a "target bitrate". If it's a target, there will be some variance where some frames take more or fewer bytes than the average; and so you'd want the value here to be some multiple. If it's a max, do we know the encoder absolutely cannot exceed the max bitrate in certain edge-case circumstances? https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:234: const bool rv = session_property_setter_->SetSessionProperty( Suggestion (to eliminate extra heap-allocated data member): media::video_toolbox::SessionPropertySetter(compression_session_, videotoolbox_glue_) .SetSessionProperty(...); See explanation in comment below... https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:273: scoped_ptr<InProgressFrameEncode> request( This is happening after a possible return statement (above, on line 268). In order to avoid leaking memory, it should be moved to before the if-statement above. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:297: return; Same problem w.r.t. early return. The client should get a BitstreamBufferReady() call with zero-byte payload so it knows which Encode()'s failed. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:303: frame_dropped = true; Looks good here. Just need to do this above too. https://codereview.chromium.org/1636083003/diff/220001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.cc (right): https://codereview.chromium.org/1636083003/diff/220001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.cc:45: class AnnexBBuffer { If you're going to go this route, you should split this class up rather than mash everything together with switch statements. Meaning, something like: class AnnexBBuffer { virtual bool Reserve(size_t size) = 0; virtual void Append(const char* s, size_t n) = 0; virtual size_t GetReservedSize() const = 0; }; class StringAnnexBBuffer : public AnnexBBuffer { ...method overrides for std::string storage... }; class RawAnnexBBuffer : public AnnexBBuffer { ...method overrides for output directly to memory region... }; ...but I'm not sure you need to handle anything other than the raw-pointer + size case, per my comment above. https://codereview.chromium.org/1636083003/diff/220001/media/cast/sender/h264... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/1636083003/diff/220001/media/cast/sender/h264... media/cast/sender/h264_vt_encoder.cc:302: scoped_ptr<video_toolbox::SessionPropertySetter> session_property_setter( Please don't allocate on the heap here. It should be just: video_toolbox::SessionPropertySetter session_property_setter(...); to remove unnecessary indirection and also to let the compiler optimize the code better.
https://codereview.chromium.org/1636083003/diff/200001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/200001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:31: MEDIA_EXPORT bool CopySampleBufferToAnnexBBuffer( On 2016/02/10 21:04:56, miu wrote: > On 2016/02/10 05:21:53, emircan wrote: > > On 2016/02/09 23:29:22, miu wrote: > > > For simplicity, you should delete the version of this function that takes a > > > std::string output. Client code can just pre-allocate the string and > truncate > > > > But wouldn't it be costly to do "std::string annexb_buffer(kMaxAnnexBSize, > > '\0')" before each time? Also, I am not sure what to set a kMaxAnnexBSize in > > cast code that is always big enough. We are expecting to lose some frames in > > VTVEA when BitstreamBuffer size isn't big enough. > > In terms of run-time cost: No, because it's still just one allocation of a chunk > of memory. In terms of space efficiency: It should be no less efficient than > what client code has to do to pre-allocate a chunk of memory for the raw pointer > version of this function. > > Whoops, maybe not kMaxAnnexBSize for the size. jfroy@, do you know? My personal opinion on this is that I would only deal with raw buffers (or a suitable "buffer byte type") and use a stateful converter initialized with a CMSampleBuffer to calculate the capacity on construction. That way, a client would be able to allocate, reserve or check capacity as needed, obtain a contiguous memory pointer as appropriate and convert. The original implementation of this code used std::string because that's what media/cast used. I consider std::string to be a terrible byte buffer type. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:126: bitrate_ / kBitsPerByte)); On 2016/02/10 21:04:56, miu wrote: > Sanity-check: Is the bitrate_ setting a "max bitrate" or a "target bitrate". If > it's a target, there will be some variance where some frames take more or fewer > bytes than the average; and so you'd want the value here to be some multiple. > If it's a max, do we know the encoder absolutely cannot exceed the max bitrate > in certain edge-case circumstances? Myself and lite@ did extensive tests of the iOS encoder's bitrate control. I can't really speak to how VT will behave on OS X using QuickSync, but... The AverageBitRate property (currently used by this implementation) really means an average. The observed bitrate over a 1-5 seconds window was observed as much as 200% higher than this average. This is especially true when the encoder is configured for real-time use. The DataRateLimits property (see crbug.com/425352) is better at setting the maximum bitrate, within about 10% over a 1-5 seconds window.
https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1714: INSTANTIATE_TEST_CASE_P( On 2016/02/10 21:04:56, miu wrote: > BTW--Will these tests run in the waterfall? IIRC, this module's tests are > currently only configured to run on ChromeOS bots. I changed content/content_tests.gypi to build this test for mac as well. Would it be picked up as a target? If not, how should I add it to waterfall? https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:20: const size_t kNumInputBuffers = 1; On 2016/02/10 18:56:27, jfroy wrote: > This is somewhat configurable using > kVTCompressionPropertyKey_MaxFrameDelayCount. It's the maximum number of input > buffers the compression can buffer until it emits a frame. It defaults to > "unlimited", but you can set it to a known value and then report that as your > limit. For me, setting kVTCompressionPropertyKey_MaxFrameDelayCount to any number fails. I tried to 0-4 with no luck. I will look into this issue in a follow-up. (OSX 10.11.3 Macbook Air 13) https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:96: if (media::PIXEL_FORMAT_I420 != format) { On 2016/02/10 18:56:27, jfroy wrote: > You can also support NV12 pretty easily. I don't know if that's a common format > in Chrome. I420 is the common format in Chrome HW encode. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:126: bitrate_ / kBitsPerByte)); On 2016/02/10 23:15:34, jfroy wrote: > On 2016/02/10 21:04:56, miu wrote: > > Sanity-check: Is the bitrate_ setting a "max bitrate" or a "target bitrate". > If > > it's a target, there will be some variance where some frames take more or > fewer > > bytes than the average; and so you'd want the value here to be some multiple. > > If it's a max, do we know the encoder absolutely cannot exceed the max bitrate > > in certain edge-case circumstances? > > Myself and lite@ did extensive tests of the iOS encoder's bitrate control. I > can't really speak to how VT will behave on OS X using QuickSync, but... > > The AverageBitRate property (currently used by this implementation) really means > an average. The observed bitrate over a 1-5 seconds window was observed as much > as 200% higher than this average. This is especially true when the encoder is > configured for real-time use. > > The DataRateLimits property (see crbug.com/425352) is better at setting the > maximum bitrate, within about 10% over a 1-5 seconds window. I will go with jfroy@ suggestions. I will remove the use of kOutputBufferSizeRatio. Instead, I will define an average_bitrate_ and max_bitrate_ params. I will take client input as average_bitrate_, and calculate max_bitrate_ via a constant. WDYT? https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:232: // TODO(emircan): VideoToolbox does not seem to support bitrate On 2016/02/10 18:56:27, jfroy wrote: > See my update to that bug. You can actually control the bitrate at runtime using > 2 properties. Done. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:234: const bool rv = session_property_setter_->SetSessionProperty( On 2016/02/10 21:04:56, miu wrote: > Suggestion (to eliminate extra heap-allocated data member): > > media::video_toolbox::SessionPropertySetter(compression_session_, > videotoolbox_glue_) > .SetSessionProperty(...); > > See explanation in comment below... Done. Keeping it as a class member on stack. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:273: scoped_ptr<InProgressFrameEncode> request( On 2016/02/10 21:04:56, miu wrote: > This is happening after a possible return statement (above, on line 268). In > order to avoid leaking memory, it should be moved to before the if-statement > above. Done. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:297: return; On 2016/02/10 21:04:56, miu wrote: > Same problem w.r.t. early return. The client should get a > BitstreamBufferReady() call with zero-byte payload so it knows which Encode()'s > failed. I do not have a BitstreamBuffer to return at this case. This is Encoder() followed by UseBitstreamBuffer() case that posciak@ mentioned. If I return buffer->id=-1, this is same as kPlatformFailureError. So instead, I wait for a buffer to arrive first. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:303: frame_dropped = true; On 2016/02/10 21:04:56, miu wrote: > Looks good here. Just need to do this above too. Done. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:306: auto sample_attachments = static_cast<CFDictionaryRef>(CFArrayGetValueAtIndex( On 2016/02/10 18:56:27, jfroy wrote: > This code seems duplicated from UseOutputBitstreamBuffer(). It would be good to > refactor it, since it's a bit subtle. Done. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:339: ->kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder(), On 2016/02/10 18:56:27, jfroy wrote: > Unless you want to fallback to Apple's *TERRIBLE* software encoder, I suggest > you also specify RequireHardwareAcceleratedVideoEncoder. If you can't use the > hardware encoder, you'd be much better served falling back on Chromium's > software encoder. Thanks for the notice, I added RequireHardwareAcceleratedVideoEncoder. I will compare this non-HW case against the SW implementation once it lands, and revisit the fallback situation. https://codereview.chromium.org/1636083003/diff/220001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.cc (right): https://codereview.chromium.org/1636083003/diff/220001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.cc:45: class AnnexBBuffer { On 2016/02/10 21:04:56, miu wrote: > If you're going to go this route, you should split this class up rather than > mash everything together with switch statements. Meaning, something like: > > class AnnexBBuffer { > virtual bool Reserve(size_t size) = 0; > virtual void Append(const char* s, size_t n) = 0; > virtual size_t GetReservedSize() const = 0; > }; > > class StringAnnexBBuffer : public AnnexBBuffer { > ...method overrides for std::string storage... > }; > > class RawAnnexBBuffer : public AnnexBBuffer { > ...method overrides for output directly to memory region... > }; > > ...but I'm not sure you need to handle anything other than the raw-pointer + > size case, per my comment above. I am taking your first suggestion into account, but what would be the ideal kMaxAnnexBSizeInBytes? 100MB? https://codereview.chromium.org/1636083003/diff/220001/media/cast/sender/h264... File media/cast/sender/h264_vt_encoder.cc (right): https://codereview.chromium.org/1636083003/diff/220001/media/cast/sender/h264... media/cast/sender/h264_vt_encoder.cc:302: scoped_ptr<video_toolbox::SessionPropertySetter> session_property_setter( On 2016/02/10 21:04:56, miu wrote: > Please don't allocate on the heap here. It should be just: > > video_toolbox::SessionPropertySetter session_property_setter(...); > > to remove unnecessary indirection and also to let the compiler optimize the code > better. My bad, it makes total sense to put it on the stack.
https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:126: bitrate_ / kBitsPerByte)); On 2016/02/11 09:22:09, emircan wrote: > On 2016/02/10 23:15:34, jfroy wrote: > > On 2016/02/10 21:04:56, miu wrote: > > > Sanity-check: Is the bitrate_ setting a "max bitrate" or a "target bitrate". > > > If > > > it's a target, there will be some variance where some frames take more or > > fewer > > > bytes than the average; and so you'd want the value here to be some > multiple. > > > If it's a max, do we know the encoder absolutely cannot exceed the max > bitrate > > > in certain edge-case circumstances? > > > > Myself and lite@ did extensive tests of the iOS encoder's bitrate control. I > > can't really speak to how VT will behave on OS X using QuickSync, but... > > > > The AverageBitRate property (currently used by this implementation) really > means > > an average. The observed bitrate over a 1-5 seconds window was observed as > much > > as 200% higher than this average. This is especially true when the encoder is > > configured for real-time use. > > > > The DataRateLimits property (see crbug.com/425352) is better at setting the > > maximum bitrate, within about 10% over a 1-5 seconds window. > > I will go with jfroy@ suggestions. I will remove the use of > kOutputBufferSizeRatio. Instead, I will define an average_bitrate_ and > max_bitrate_ params. I will take client input as average_bitrate_, and calculate > max_bitrate_ via a constant. WDYT? I would really run a few experiments to see how your rate control is working. VT tries to be a uniform interface to widely different encoders, so those parameters might work is a surprising and different way than on iOS. For what it's worth, in cast we assume the bitrate specified by the client (i.e. the target rate calculated by cast's rate control algorithm) is where it wants the encoder to go and we set both the average bit rate and target rate parameters to the same value (after converting to the right units -- the target data rate takes bytes, not bits).
https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:1714: INSTANTIATE_TEST_CASE_P( On 2016/02/11 09:22:09, emircan wrote: > On 2016/02/10 21:04:56, miu wrote: > > BTW--Will these tests run in the waterfall? IIRC, this module's tests are > > currently only configured to run on ChromeOS bots. > > I changed content/content_tests.gypi to build this test for mac as well. Would > it be picked up as a target? If not, how should I add it to waterfall? I'm not sure about this one. I think it has dependencies on downloading videos from external sources. Pawel would know. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:126: bitrate_ / kBitsPerByte)); On 2016/02/11 09:22:09, emircan wrote: > On 2016/02/10 23:15:34, jfroy wrote: > > On 2016/02/10 21:04:56, miu wrote: > > > Sanity-check: Is the bitrate_ setting a "max bitrate" or a "target bitrate". > > > If > > > it's a target, there will be some variance where some frames take more or > > fewer > > > bytes than the average; and so you'd want the value here to be some > multiple. > > > If it's a max, do we know the encoder absolutely cannot exceed the max > bitrate > > > in certain edge-case circumstances? > > > > Myself and lite@ did extensive tests of the iOS encoder's bitrate control. I > > can't really speak to how VT will behave on OS X using QuickSync, but... > > > > The AverageBitRate property (currently used by this implementation) really > means > > an average. The observed bitrate over a 1-5 seconds window was observed as > much > > as 200% higher than this average. This is especially true when the encoder is > > configured for real-time use. > > > > The DataRateLimits property (see crbug.com/425352) is better at setting the > > maximum bitrate, within about 10% over a 1-5 seconds window. > > I will go with jfroy@ suggestions. I will remove the use of > kOutputBufferSizeRatio. Instead, I will define an average_bitrate_ and > max_bitrate_ params. I will take client input as average_bitrate_, and calculate > max_bitrate_ via a constant. WDYT? I can't remember whether the VEA interface treats bitrate as a target or a max. Pawel would know for sure. https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:297: return; On 2016/02/11 09:22:10, emircan wrote: > On 2016/02/10 21:04:56, miu wrote: > > Same problem w.r.t. early return. The client should get a > > BitstreamBufferReady() call with zero-byte payload so it knows which > Encode()'s > > failed. > > I do not have a BitstreamBuffer to return at this case. This is Encoder() > followed by UseBitstreamBuffer() case that posciak@ mentioned. If I return > buffer->id=-1, this is same as kPlatformFailureError. So instead, I wait for a > buffer to arrive first. Acknowledged. My misunderstanding. https://codereview.chromium.org/1636083003/diff/220001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.cc (right): https://codereview.chromium.org/1636083003/diff/220001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.cc:45: class AnnexBBuffer { On 2016/02/11 09:22:10, emircan wrote: > On 2016/02/10 21:04:56, miu wrote: > > If you're going to go this route, you should split this class up rather than > > mash everything together with switch statements. Meaning, something like: > > > > class AnnexBBuffer { > > virtual bool Reserve(size_t size) = 0; > > virtual void Append(const char* s, size_t n) = 0; > > virtual size_t GetReservedSize() const = 0; > > }; > > > > class StringAnnexBBuffer : public AnnexBBuffer { > > ...method overrides for std::string storage... > > }; > > > > class RawAnnexBBuffer : public AnnexBBuffer { > > ...method overrides for output directly to memory region... > > }; > > > > ...but I'm not sure you need to handle anything other than the raw-pointer + > > size case, per my comment above. > > I am taking your first suggestion into account, but what would be the ideal > kMaxAnnexBSizeInBytes? 100MB? It looks like the code at the top of CopySampleBufferToAnnexBBuffer() computes this (as |total_bytes|). Seems to me you could just break this code out into a separate ComputeAnnexBBufferSize(CMSampleBufferRef) function and now you only need a single memory allocation. :) Also, CopySampleBufferToAnnexBBuffer() doesn't use |total_bytes| for anything other than to confirm its output buffer is big enough. Therefore, it should be reasonable for it to just assume the client provided a big enough buffer as a precondition. Client code might look like: const size_t annex_b_size = ComputeAnnexBBufferSize(sbuf, key_frame); scoped_ptr<uint8_t[]> annex_b_buffer(new uint8_t[annex_b_size]); // OR: annex_b_buffer_as_string.resize(annex_b_size, '\0'); CopySampleBufferToAnnexBBuffer(sbuf, annex_b_buffer.get(), key_frame); https://codereview.chromium.org/1636083003/diff/240001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/240001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:310: if (info & VideoToolboxGlue::kVTEncodeInfo_FrameDropped) { nit: Add DCHECK(thread_checker_...); https://codereview.chromium.org/1636083003/diff/240001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:419: session_property_setter_ = media::video_toolbox::SessionPropertySetter( Could you explain why you're trying to keep the |session_property_setter_| class member variable? The point I've been trying to make is that you do not need to share this across two different methods. Instead, just create it *separately* wherever you need it (as a short-lived temporary object); just like you did in the cast h264_vt_encoder.cc code. Yes, it will be created each time either of the methods is called. However, that is actually desirable: You save memory by not storing it in the VTVEA object long-term, and the optimizing compiler can eliminate many of the overhead costs with constructing/destroying it when it is a local object. https://codereview.chromium.org/1636083003/diff/240001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/240001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:45: SessionPropertySetter(); Please remove the zero-arg and copy ctors (see comments in other file).
https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/220001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:126: bitrate_ / kBitsPerByte)); On 2016/02/11 21:09:40, miu wrote: > On 2016/02/11 09:22:09, emircan wrote: > > On 2016/02/10 23:15:34, jfroy wrote: > > > On 2016/02/10 21:04:56, miu wrote: > > > > Sanity-check: Is the bitrate_ setting a "max bitrate" or a "target > bitrate". > > > > > If > > > > it's a target, there will be some variance where some frames take more or > > > fewer > > > > bytes than the average; and so you'd want the value here to be some > > multiple. > > > > If it's a max, do we know the encoder absolutely cannot exceed the max > > bitrate > > > > in certain edge-case circumstances? > > > > > > Myself and lite@ did extensive tests of the iOS encoder's bitrate control. I > > > can't really speak to how VT will behave on OS X using QuickSync, but... > > > > > > The AverageBitRate property (currently used by this implementation) really > > means > > > an average. The observed bitrate over a 1-5 seconds window was observed as > > much > > > as 200% higher than this average. This is especially true when the encoder > is > > > configured for real-time use. > > > > > > The DataRateLimits property (see crbug.com/425352) is better at setting the > > > maximum bitrate, within about 10% over a 1-5 seconds window. > > > > I will go with jfroy@ suggestions. I will remove the use of > > kOutputBufferSizeRatio. Instead, I will define an average_bitrate_ and > > max_bitrate_ params. I will take client input as average_bitrate_, and > calculate > > max_bitrate_ via a constant. WDYT? > > I can't remember whether the VEA interface treats bitrate as a target or a max. > Pawel would know for sure. I did some further investigation as jfroy@ suggested. I found that when HW encoder is actually used (see CreateCompressionSession()), setting kVTCompressionPropertyKey_DataRateLimits parameter ends up giving an error: GVA encoder warning: CFDataRateLimitType = 3 (DataRateLimitsSeconds) . Changing the parameter doesn't make any difference at that point. Googling CFDataRateLimitType returns only a single result which isn't helpful, so I cannot rely on the param as a max_bitrate_. kVTCompressionPropertyKey_AverageBitRate is the only reliable one. Therefore, I will use the given bitrate_ input there as the average, and on kVTCompressionPropertyKey_DataRateLimits when it is available. Also, I will decide BitstreamBuffer size based on (bitrate/8) rather than using other subjective constants. It is big enough to cover most of the output cases. There seems to be a frame once in ~5 seconds that is more than double the buffer size. https://codereview.chromium.org/1636083003/diff/220001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.cc (right): https://codereview.chromium.org/1636083003/diff/220001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.cc:45: class AnnexBBuffer { On 2016/02/11 21:09:40, miu wrote: > On 2016/02/11 09:22:10, emircan wrote: > > On 2016/02/10 21:04:56, miu wrote: > > > If you're going to go this route, you should split this class up rather than > > > mash everything together with switch statements. Meaning, something like: > > > > > > class AnnexBBuffer { > > > virtual bool Reserve(size_t size) = 0; > > > virtual void Append(const char* s, size_t n) = 0; > > > virtual size_t GetReservedSize() const = 0; > > > }; > > > > > > class StringAnnexBBuffer : public AnnexBBuffer { > > > ...method overrides for std::string storage... > > > }; > > > > > > class RawAnnexBBuffer : public AnnexBBuffer { > > > ...method overrides for output directly to memory region... > > > }; > > > > > > ...but I'm not sure you need to handle anything other than the raw-pointer + > > > size case, per my comment above. > > > > I am taking your first suggestion into account, but what would be the ideal > > kMaxAnnexBSizeInBytes? 100MB? > > It looks like the code at the top of CopySampleBufferToAnnexBBuffer() computes > this (as |total_bytes|). Seems to me you could just break this code out into a > separate ComputeAnnexBBufferSize(CMSampleBufferRef) function and now you only > need a single memory allocation. :) > > Also, CopySampleBufferToAnnexBBuffer() doesn't use |total_bytes| for anything > other than to confirm its output buffer is big enough. Therefore, it should be > reasonable for it to just assume the client provided a big enough buffer as a > precondition. Client code might look like: > > const size_t annex_b_size = ComputeAnnexBBufferSize(sbuf, key_frame); > scoped_ptr<uint8_t[]> annex_b_buffer(new uint8_t[annex_b_size]); > // OR: annex_b_buffer_as_string.resize(annex_b_size, '\0'); > CopySampleBufferToAnnexBBuffer(sbuf, annex_b_buffer.get(), key_frame); > There are some parameters like |bb|, |pset_count|, |nal_size_field_bytes| that would need to be calculated each time in these functions, or passed from one to another. I feel like interface and StringAnnexBBuffer/RawAnnexBBuffer is the tidiest solution to all this. https://codereview.chromium.org/1636083003/diff/240001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/240001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:310: if (info & VideoToolboxGlue::kVTEncodeInfo_FrameDropped) { On 2016/02/11 21:09:40, miu wrote: > nit: Add DCHECK(thread_checker_...); Done. https://codereview.chromium.org/1636083003/diff/240001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:419: session_property_setter_ = media::video_toolbox::SessionPropertySetter( On 2016/02/11 21:09:40, miu wrote: > Could you explain why you're trying to keep the |session_property_setter_| class > member variable? The point I've been trying to make is that you do not need to > share this across two different methods. Instead, just create it *separately* > wherever you need it (as a short-lived temporary object); just like you did in > the cast h264_vt_encoder.cc code. > > Yes, it will be created each time either of the methods is called. However, > that is actually desirable: You save memory by not storing it in the VTVEA > object long-term, and the optimizing compiler can eliminate many of the overhead > costs with constructing/destroying it when it is a local object. I am changing it as you suggest. I understand your points and I had those in mind when keeping it as a class member :) It is a tradeoff. RequestEncodingParametersChange() is called many times on an average call, almost every other second. We access 2 class members to construct session_property_setter. I think accessing just the |session_property_setter_| class member and using it would be cheaper as it is small such that it has just two member pointers. https://codereview.chromium.org/1636083003/diff/240001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/240001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:45: SessionPropertySetter(); On 2016/02/11 21:09:40, miu wrote: > Please remove the zero-arg and copy ctors (see comments in other file). Done.
https://chromiumcodereview.appspot.com/1636083003/diff/140001/content/common/... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1636083003/diff/140001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:657: decoder_(new media::FFmpegVideoDecoder()), On 2016/02/08 23:41:23, emircan wrote: > On 2016/02/08 04:33:42, Pawel Osciak wrote: > > Did you mean ffmpeg was just not compiled/working on Mac at all, or that it > > would fail for some other reason? I thought ffmpeg worked on Mac... > > It points to this line when built: > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu.... > Do you know if it is still the case? Are you building and testing on Chromium, instead of Chrome? https://chromiumcodereview.appspot.com/1636083003/diff/140001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1636083003/diff/140001/content/common/... content/common/gpu/media/vt_video_encode_accelerator.cc:238: // This method is NOT called on |client_task_runner_|, so we still need to On 2016/02/08 23:41:23, emircan wrote: > On 2016/02/08 04:33:42, Pawel Osciak wrote: > > Uhh, if so, I don't think we can access encoder at all, including members like > > encoder_output_queue_. Aren't we pushing to encoder_output_queue_ on > > client_task_runner_ for example? > > > > If that's the case, could we trampoline from this method at the beginning to > > client_task_runner_ to actually handle things there? > > I am posting a task as you suggest. > > But why wouldn't we access encoder at all? Accessing client_task_runner through > encoder is also done on cast code: > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... > . If your concern is regarding thread safety of std::deque, I checked it prior > and found mixed answers regarding it. > Concurrent read-only calls to containers are ok, but we are not doing read-only calls only. Also, even if each method of the container was atomic (like empty(), push_back() etc.), the container wouldn't be locked across calls. So for example checking !empty() here and then depending on it staying non-empty to be able to safely call pop() here would not work without explicit locking if another thread could be using it in parallel. https://chromiumcodereview.appspot.com/1636083003/diff/140001/content/common/... content/common/gpu/media/vt_video_encode_accelerator.cc:308: videotoolbox_glue_->kVTProfileLevel_H264_Baseline_AutoLevel()); On 2016/02/08 23:41:24, emircan wrote: > On 2016/02/08 04:33:42, Pawel Osciak wrote: > > Is baseline preferred by us? Normally constrained baseline is more compatible > > with various devices (unless this actually is constrained). Main may also be > > better in general. > > I discussed this with WebRTC people. I learned that Baseline is the only one > that openh264 supports -SW case- and compatible with Firefox as well. I am > planning to support only Baseline in this CL. I can add other profiles after > further discussion, and it wouldn't be hard. Are we sure that was Baseline, and not Constrained Baseline though? A good number of HW decoders doesn't support Baseline, as it requires additional features. https://chromiumcodereview.appspot.com/1636083003/diff/220001/content/common/... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/1636083003/diff/220001/content/common/... content/common/gpu/media/video_encode_accelerator_unittest.cc:1714: INSTANTIATE_TEST_CASE_P( On 2016/02/11 21:09:40, miu wrote: > On 2016/02/11 09:22:09, emircan wrote: > > On 2016/02/10 21:04:56, miu wrote: > > > BTW--Will these tests run in the waterfall? IIRC, this module's tests are > > > currently only configured to run on ChromeOS bots. > > > > I changed content/content_tests.gypi to build this test for mac as well. Would > > it be picked up as a target? If not, how should I add it to waterfall? > > I'm not sure about this one. I think it has dependencies on downloading videos > from external sources. Pawel would know. We have a short raw stream in Chromium tree that can be used for basic sanity testing: media/test/data/bear_320x192_40frames.yuv, but it's not good for bitrate tests. It's already here in the code at the top of this file (g_default_in_filename). So if you run this test as is on the bots, the "media::GetTestDataFilePath(content::g_default_in_filename)" line below will pick it up automagically. Locally you have to make sure https://code.google.com/p/chromium/codesearch#chromium/src/media/base/test_da... resolves to where the file is (using DIR_SOURCE_ROOT for example). https://chromiumcodereview.appspot.com/1636083003/diff/220001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://chromiumcodereview.appspot.com/1636083003/diff/220001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:126: bitrate_ / kBitsPerByte)); On 2016/02/11 21:09:40, miu wrote: > On 2016/02/11 09:22:09, emircan wrote: > > On 2016/02/10 23:15:34, jfroy wrote: > > > On 2016/02/10 21:04:56, miu wrote: > > > > Sanity-check: Is the bitrate_ setting a "max bitrate" or a "target > bitrate". > > > > > If > > > > it's a target, there will be some variance where some frames take more or > > > fewer > > > > bytes than the average; and so you'd want the value here to be some > > multiple. > > > > If it's a max, do we know the encoder absolutely cannot exceed the max > > bitrate > > > > in certain edge-case circumstances? > > > > > > Myself and lite@ did extensive tests of the iOS encoder's bitrate control. I > > > can't really speak to how VT will behave on OS X using QuickSync, but... > > > > > > The AverageBitRate property (currently used by this implementation) really > > means > > > an average. The observed bitrate over a 1-5 seconds window was observed as > > much > > > as 200% higher than this average. This is especially true when the encoder > is > > > configured for real-time use. > > > > > > The DataRateLimits property (see crbug.com/425352) is better at setting the > > > maximum bitrate, within about 10% over a 1-5 seconds window. > > > > I will go with jfroy@ suggestions. I will remove the use of > > kOutputBufferSizeRatio. Instead, I will define an average_bitrate_ and > > max_bitrate_ params. I will take client input as average_bitrate_, and > calculate > > max_bitrate_ via a constant. WDYT? > > I can't remember whether the VEA interface treats bitrate as a target or a max. > Pawel would know for sure. It's not clearly specified in the docs, but it's expected to be the target bitrate. https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:151: scoped_ptr<InProgressFrameEncode> request(new InProgressFrameEncode( Nit: please move above l.163. https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:238: set_data_rate_limit_ &= session_property_setter.SetSessionProperty( I believe this would have the same effect? if (s_d_r_l_) { s_d_r_l = SSP(); } https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:274: encoder->client_task_runner_->PostTask( Do we need to do anything in this method at all? Could we just post to the task and do error checks there? https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:288: base::Unretained(encoder), info, sbuf)); Please add a comment why it's safe to do Unretained. https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:296: // If there isn't any BitstreamBuffer to copy into, add it to a queue for Could we perhaps have a TryReturnBitstreamBuffer() which would do this also, instead of duplicating this code here and in UseOutputBitstreamBuffer() please? https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:348: // Keep these in-sync with those in ConfigureCompressionSession(). These keys below seem different from the ones in CCS()...? What does it mean to keep them in sync? https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:370: // Try creating session again without forcing HW encode. Are we interested in doing SW encode here at all? It would be preferable to do it outside of GPU process then... https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:415: reinterpret_cast<void*>(this), Can we in any way guarantee CompressionCallback caller doesn't outlive this? https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right): https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.h:18: // safe, so this object is pinned to the thread on which it is constructed. We should always avoid doing things on GPU Child thread unless we have no other choice (e.g. need GL context), because this is a performance-critical thread. https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.h:82: // Destroy the current compression session if any. Blocks until all pending This sounds especially concerning if blocking GPU Child. https://chromiumcodereview.appspot.com/1636083003/diff/260001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.h:95: bool set_data_rate_limit_; Please document this field.
https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:151: scoped_ptr<InProgressFrameEncode> request(new InProgressFrameEncode( On 2016/02/18 11:16:14, Pawel Osciak wrote: > Nit: please move above l.163. Done. https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:238: set_data_rate_limit_ &= session_property_setter.SetSessionProperty( On 2016/02/18 11:16:14, Pawel Osciak wrote: > I believe this would have the same effect? > > if (s_d_r_l_) { > s_d_r_l = SSP(); > } Done. https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:274: encoder->client_task_runner_->PostTask( On 2016/02/18 11:16:14, Pawel Osciak wrote: > Do we need to do anything in this method at all? Could we just post to the task > and do error checks there? Done. https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:288: base::Unretained(encoder), info, sbuf)); On 2016/02/18 11:16:14, Pawel Osciak wrote: > Please add a comment why it's safe to do Unretained. Actually, Unretained looks wrong to me now. If this function is called during the destruction sequence, |enocder| might be freed by the time task is posted. I will change it to WeakPtr and invalidate WeakPtr in the beginning of destruction sequence. https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:296: // If there isn't any BitstreamBuffer to copy into, add it to a queue for On 2016/02/18 11:16:14, Pawel Osciak wrote: > Could we perhaps have a TryReturnBitstreamBuffer() which would do this also, > instead of duplicating this code here and in UseOutputBitstreamBuffer() please? Here we are checking if bitstream_buffer_queue_ is empty. Inside UseOutputBitstreamBuffer() , we check if encoder_output_queue_ is empty. The common code is already in ReturnBitstreamBuffer() and I don't know if any further refactor can be done. https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:348: // Keep these in-sync with those in ConfigureCompressionSession(). On 2016/02/18 11:16:14, Pawel Osciak wrote: > These keys below seem different from the ones in CCS()...? What does it mean to > keep them in sync? Thanks for the notice, I am removing the comment since it no longer applies. https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:370: // Try creating session again without forcing HW encode. On 2016/02/18 11:16:14, Pawel Osciak wrote: > Are we interested in doing SW encode here at all? It would be preferable to do > it outside of GPU process then... Eventually, we want to compare the performance of Videotoolbox SW to our OpenH264 SW when they both land completely. Then, we can modify this logic. WDYT? https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:415: reinterpret_cast<void*>(this), On 2016/02/18 11:16:14, Pawel Osciak wrote: > Can we in any way guarantee CompressionCallback caller doesn't outlive this? I followed the logic used by cast that you can find below. I will copy their comments as well to make it clear. https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right): https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.h:18: // safe, so this object is pinned to the thread on which it is constructed. On 2016/02/18 11:16:14, Pawel Osciak wrote: > We should always avoid doing things on GPU Child thread unless we have no other > choice (e.g. need GL context), because this is a performance-critical thread. Ok, I am adding an encoder thread. I was trying to make this patch a starting point behind flag, but it is already big enough at this point :) Adding the third thread that this class can be on would hurt. https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.h:82: // Destroy the current compression session if any. Blocks until all pending On 2016/02/18 11:16:14, Pawel Osciak wrote: > This sounds especially concerning if blocking GPU Child. I will post this on the |encoder_thread| during dtor. https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.h:95: bool set_data_rate_limit_; On 2016/02/18 11:16:14, Pawel Osciak wrote: > Please document this field. Done.
Patchset #11 (id:300001) has been deleted
Patchset #11 (id:320001) has been deleted
PTAL, I made some updates. Here is a Design Doc that explains the limitations and has the performance comparison with the SW encoder implementation. https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sU... https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/video_encode_accelerator_unittest.cc:657: decoder_(new media::FFmpegVideoDecoder()), On 2016/02/18 11:16:13, Pawel Osciak wrote: > On 2016/02/08 23:41:23, emircan wrote: > > On 2016/02/08 04:33:42, Pawel Osciak wrote: > > > Did you mean ffmpeg was just not compiled/working on Mac at all, or that it > > > would fail for some other reason? I thought ffmpeg worked on Mac... > > > > It points to this line when built: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu.... > > Do you know if it is still the case? > > Are you building and testing on Chromium, instead of Chrome? I built with ffmpeg branding and it works fine now. I will turn on the unit test to verify frames now. https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator.cc (right): https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator.cc:308: videotoolbox_glue_->kVTProfileLevel_H264_Baseline_AutoLevel()); On 2016/02/18 11:16:14, Pawel Osciak wrote: > On 2016/02/08 23:41:24, emircan wrote: > > On 2016/02/08 04:33:42, Pawel Osciak wrote: > > > Is baseline preferred by us? Normally constrained baseline is more > compatible > > > with various devices (unless this actually is constrained). Main may also be > > > better in general. > > > > I discussed this with WebRTC people. I learned that Baseline is the only one > > that openh264 supports -SW case- and compatible with Firefox as well. I am > > planning to support only Baseline in this CL. I can add other profiles after > > further discussion, and it wouldn't be hard. > > Are we sure that was Baseline, and not Constrained Baseline though? A good > number of HW decoders doesn't support Baseline, as it requires additional > features. As far as I got answers from WebRTC folks, it is Baseline[0]. I can revisit this to add new formats if needed. [0] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... https://codereview.chromium.org/1636083003/diff/340001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/340001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:126: bitstream_buffer_size_ = input_visible_size.GetArea(); Calculating this based on the initial bitrate isn't a good idea. Eventually, when the bitrate increases, there is some keyframe which are much bigger than expected. Setting it as frame size like VAAPI[0] is safer. [0] https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... https://codereview.chromium.org/1636083003/diff/340001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:216: target_bitrate_ / kBitsPerByte, 1.0f)); I figured out that the error I got here was due to setting 2 integers. Encoder expects second param to be float, and it seems to work fine.
Patchset #11 (id:340001) has been deleted
emircan@chromium.org changed reviewers: - hbos@chromium.org
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. BUG=500605 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. Desing 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. Desing 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. 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 ==========
Other than my small comments, looks good. https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:43: const CMSampleBufferRef sample_buffer; Would it be safer to use a smart pointer to manage the ref count? https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:339: encoder->weak_this_factory_.GetWeakPtr(), status, info, sbuf)); If this turns out to be weak, sbuf gets leaked. I'm not sure how to cleanly work around this. Perhaps a smart pointer that can be managed by base::Bind or base::Callback? https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:344: CMSampleBufferRef sbuf) { Document that sbuf is retained and needs to be released by this function, or use smart pointer. https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:369: CMSampleBufferRef sbuf, Document that sbuf is retained and needs to be released by this function, or use smart pointer.
https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:43: const CMSampleBufferRef sample_buffer; On 2016/03/07 21:57:07, jfroy wrote: > Would it be safer to use a smart pointer to manage the ref count? Done. https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:339: encoder->weak_this_factory_.GetWeakPtr(), status, info, sbuf)); On 2016/03/07 21:57:07, jfroy wrote: > If this turns out to be weak, sbuf gets leaked. I'm not sure how to cleanly work > around this. Perhaps a smart pointer that can be managed by base::Bind or > base::Callback? Good catch. I will defer the lifetime of this object to struct EncodeOutput. https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:344: CMSampleBufferRef sbuf) { On 2016/03/07 21:57:07, jfroy wrote: > Document that sbuf is retained and needs to be released by this function, or use > smart pointer. I defer the lifetime of this object to struct EncodeOutput. https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:369: CMSampleBufferRef sbuf, On 2016/03/07 21:57:07, jfroy wrote: > Document that sbuf is retained and needs to be released by this function, or use > smart pointer. sbuf will go out of scope when struct EncodeOutput goes out of scope.
lgtm
lgtm
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. Additionally, kDisableVTAcceleratedVideoEncode flag is added to disable this test for certain cast tests. 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 ==========
https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:225: client_ptr_factory_.reset(); This is an illegal operation. The WeakPtrs being dereferenced on the "client task" thread must always be invalidated on that same thread as well. Otherwise, there could be code executing concurrently that has already null-checked the WeakPtr but still assumes the object is alive. Hmm...This is a really complex threading model here. IMO, the simplest solution is not to access |client_| via a WeakPtr. Instead, store it as a raw pointer (in Initialize()) and null it out (in Destroy(). Then, for all read-write access and calls into the client's methods, always be holding a base::Lock. And, always null-check |client_|, of course, since tasks can run after Destroy() is called.
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. Additionally, kDisableVTAcceleratedVideoEncode flag is added to disable this test for certain cast tests. 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. Additionally, kDisableVTAcceleratedVideoEncode flag is added to disable this encoder for certain cast tests. 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 ==========
Mostly small nits/tweaks comments: https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:73: } For sanity, after the Destroy() call here, consider adding: DCHECK(!encoder_thread_.IsRunning()); DCHECK(!weak_this_factory_.HasWeakPtrs()); (to be sure the encoder thread and all outstanding tasks have been stopped/canceled) https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:225: client_ptr_factory_.reset(); On 2016/03/08 21:27:41, miu wrote: > This is an illegal operation. The WeakPtrs being dereferenced on the "client > task" thread must always be invalidated on that same thread as well. Otherwise, > there could be code executing concurrently that has already null-checked the > WeakPtr but still assumes the object is alive. > > Hmm...This is a really complex threading model here. IMO, the simplest solution > is not to access |client_| via a WeakPtr. Instead, store it as a raw pointer > (in Initialize()) and null it out (in Destroy(). Then, for all read-write > access and calls into the client's methods, always be holding a base::Lock. > And, always null-check |client_|, of course, since tasks can run after Destroy() > is called. Never mind. My misunderstanding. We discussed this off-line (it's the same thread). https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:296: // Cancel all callbacks. nit: Cancel all encoder thread callbacks. https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:297: weak_this_factory_.InvalidateWeakPtrs(); naming nit: Instead of weak_this_factory_, how about encoder_task_weak_factory_? That would make it clear to the reader that these WeakPtrs are being used only for encoder tasks on the encode thread. https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right): https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.h:118: scoped_ptr<base::WeakPtrFactory<Client> > client_ptr_factory_; nit: It's not necessary to use scoped_ptr<> here. Meaning, in Destroy(), you could just call: client_ptr_factory_.InvalidateWeakPtrs(); https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.cc (right): https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.cc:46: std::vector<CFNumberRef> numbers; This is a good place to use std::array: std::array<CFNumberRef, 2> numbers = { CFNumberCreate(nullptr, kCFNumberSInt32Type, &int_val), CFNumberCreate(nullptr, kCFNumberFloat32Type, &float_val) }; base::ScopedCFTypeRef<CFArrayRef> array(CFArrayCreate( kCFAllocatorDefault, reinterpret_cast<const void**>(numbers.data()), numbers.size(), &kCFTypeArrayCallBacks)); for (auto& number : numbers) CFRelease(number); https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:55: bool SetSessionProperty(CFStringRef key, int32_t value); nit suggestion: When I was reading the code calling these, it seemed a bit redundant to read: session_property_setter.SetSessionProperty(...); Instead, consider just renaming these methods to Set() so that calling code is simplified to: session_property_setter.Set(...);
https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:73: } On 2016/03/08 22:36:05, miu wrote: > For sanity, after the Destroy() call here, consider adding: > > DCHECK(!encoder_thread_.IsRunning()); > DCHECK(!weak_this_factory_.HasWeakPtrs()); > > (to be sure the encoder thread and all outstanding tasks have been > stopped/canceled) Done. https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:296: // Cancel all callbacks. On 2016/03/08 22:36:05, miu wrote: > nit: Cancel all encoder thread callbacks. Done. https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:297: weak_this_factory_.InvalidateWeakPtrs(); On 2016/03/08 22:36:05, miu wrote: > naming nit: Instead of weak_this_factory_, how about encoder_task_weak_factory_? > That would make it clear to the reader that these WeakPtrs are being used only > for encoder tasks on the encode thread. Done. https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right): https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.h:118: scoped_ptr<base::WeakPtrFactory<Client> > client_ptr_factory_; On 2016/03/08 22:36:05, miu wrote: > nit: It's not necessary to use scoped_ptr<> here. Meaning, in Destroy(), you > could just call: > > client_ptr_factory_.InvalidateWeakPtrs(); I cannot make this a class member as I cannot construct it in ctor and copy is private. https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.cc (right): https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.cc:46: std::vector<CFNumberRef> numbers; On 2016/03/08 22:36:05, miu wrote: > This is a good place to use std::array: > > std::array<CFNumberRef, 2> numbers = { > CFNumberCreate(nullptr, kCFNumberSInt32Type, &int_val), > CFNumberCreate(nullptr, kCFNumberFloat32Type, &float_val) > }; > base::ScopedCFTypeRef<CFArrayRef> array(CFArrayCreate( > kCFAllocatorDefault, reinterpret_cast<const void**>(numbers.data()), > numbers.size(), &kCFTypeArrayCallBacks)); > for (auto& number : numbers) > CFRelease(number); Done. https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videoto... File media/base/mac/videotoolbox_helpers.h (right): https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videoto... media/base/mac/videotoolbox_helpers.h:55: bool SetSessionProperty(CFStringRef key, int32_t value); On 2016/03/08 22:36:05, miu wrote: > nit suggestion: When I was reading the code calling these, it seemed a bit > redundant to read: > > session_property_setter.SetSessionProperty(...); > > Instead, consider just renaming these methods to Set() so that calling code is > simplified to: > > session_property_setter.Set(...); Done.
PS14 lgtm % a few things: https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensi... File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc (right): https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensi... chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:330: command_line->AppendSwitch(::switches::kDisableVTAcceleratedVideoEncode); Is this still needed? If the tests now work, let's take this out and also take out the command-line switch. https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: DLOG(ERROR) << "Failed creating compression session with hardware support."; This would be a good thing to have as VLOG(1) instead. FWIW, I'd be curious to try this code out in our testing lab to see which Macs are using HW H264 encoding after this change. :) https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:446: // Try creating session again without forcing HW encode. nit: Maybe add to this comment that this is seems to be needed in order to support resolutions below 640x480, for clients that might downgrade to those resolutions but don't expect the encoder to stop working.
https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensi... File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc (right): https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensi... chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:330: command_line->AppendSwitch(::switches::kDisableVTAcceleratedVideoEncode); On 2016/03/09 01:48:50, miu wrote: > Is this still needed? If the tests now work, let's take this out and also take > out the command-line switch. Unfortunately it is still needed. In my local tests, I am able to create a VT HW session, so H264 gets reported and this code triggers Cast's H264 implementation which fails. https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: DLOG(ERROR) << "Failed creating compression session with hardware support."; On 2016/03/09 01:48:50, miu wrote: > This would be a good thing to have as VLOG(1) instead. FWIW, I'd be curious to > try this code out in our testing lab to see which Macs are using HW H264 > encoding after this change. :) Done. https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:446: // Try creating session again without forcing HW encode. On 2016/03/09 01:48:50, miu wrote: > nit: Maybe add to this comment that this is seems to be needed in order to > support resolutions below 640x480, for clients that might downgrade to those > resolutions but don't expect the encoder to stop working. Done.
https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensi... File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc (right): https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensi... chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:330: command_line->AppendSwitch(::switches::kDisableVTAcceleratedVideoEncode); On 2016/03/09 02:26:03, emircan wrote: > On 2016/03/09 01:48:50, miu wrote: > > Is this still needed? If the tests now work, let's take this out and also > take > > out the command-line switch. > > Unfortunately it is still needed. In my local tests, I am able to create a VT HW > session, so H264 gets reported and this code triggers Cast's H264 implementation > which fails. Ok, I think this may be the problem: https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/receive... In other words, this test is creating a Cast Receiver, but our Chromium implementation lacks H264 decode support. In that case, what you should do is force VP8 to be used: 1. Modify the sender JavaScript part of this test code to use VP8 instead of H264 (which became the default, because of your change). Relevant code is here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e... Just set videoParams.payload.codecName = 'VP8' before start() is called a few lines later. This will force the VP8 encoder to be used instead. 2. Remove this command-line override. 3. Confirm the test runs now. :-)
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. Additionally, kDisableVTAcceleratedVideoEncode flag is added to disable this encoder for certain cast tests. 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. 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 ==========
https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensi... File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc (right): https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensi... chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:330: command_line->AppendSwitch(::switches::kDisableVTAcceleratedVideoEncode); On 2016/03/09 03:03:19, miu wrote: > On 2016/03/09 02:26:03, emircan wrote: > > On 2016/03/09 01:48:50, miu wrote: > > > Is this still needed? If the tests now work, let's take this out and also > > take > > > out the command-line switch. > > > > Unfortunately it is still needed. In my local tests, I am able to create a VT > HW > > session, so H264 gets reported and this code triggers Cast's H264 > implementation > > which fails. > > Ok, I think this may be the problem: > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/receive... > > In other words, this test is creating a Cast Receiver, but our Chromium > implementation lacks H264 decode support. In that case, what you should do is > force VP8 to be used: > > 1. Modify the sender JavaScript part of this test code to use VP8 instead of > H264 (which became the default, because of your change). Relevant code is here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e... > > Just set videoParams.payload.codecName = 'VP8' before start() is called a few > lines later. This will force the VP8 encoder to be used instead. > > 2. Remove this command-line override. > > 3. Confirm the test runs now. :-) Thank you very much. Now, cast test works as expected and I am removing the flag :)
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding an encoder thread, additional codec profiles and support for encoder modifications. 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding additional codec profiles and support for bitrate adaptations. 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: + dalecurtis@chromium.org
dalecurtis@, can you PTAL media/* and content/common/gpu/media/* as OWNERS review?
dalecurtis@chromium.org changed reviewers: + sandersd@chromium.org
=> sandersd@
RS LGTM for media % nit. There seem to be a lot of helper methods that are not actually media-related, but I am not so worried that I would ask for changes as long as it stays in mac/. https://codereview.chromium.org/1636083003/diff/460001/media/base/mac/videoto... File media/base/mac/videotoolbox_glue.h (right): https://codereview.chromium.org/1636083003/diff/460001/media/base/mac/videoto... media/base/mac/videotoolbox_glue.h:16: // requires OS X 10.6 or iOS 6. Linking with VideoToolbox therefore has to This comment is out of date now. In fact we are just waiting for the build SDK bump so that we can delete this glue file. Unless iOS is still at 6? (https://bugs.chromium.org/p/chromium/issues/detail?id=579648)
https://codereview.chromium.org/1636083003/diff/460001/media/base/mac/videoto... File media/base/mac/videotoolbox_glue.h (right): https://codereview.chromium.org/1636083003/diff/460001/media/base/mac/videoto... media/base/mac/videotoolbox_glue.h:16: // requires OS X 10.6 or iOS 6. Linking with VideoToolbox therefore has to On 2016/03/09 22:42:08, sandersd wrote: > This comment is out of date now. In fact we are just waiting for the build SDK > bump so that we can delete this glue file. Unless iOS is still at 6? > > (https://bugs.chromium.org/p/chromium/issues/detail?id=579648) Thanks for the heads up. I updated the comment and referenced the bug.
emircan@chromium.org changed reviewers: + avi@chromium.org, thakis@chromium.org
avi@chromium.org: Please review changes in content/common/BUILD.gn. thakis@chromium.org: Please review changes in build/gn_migration.gypi
gn_migration.gypi lgtm
content/common/BUILD.gn lgtm
https://codereview.chromium.org/1636083003/diff/460001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right): https://codereview.chromium.org/1636083003/diff/460001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.h:54: // Encoding tasks to be run on |encode_thread|. s/encode_thread/encoder_thread_/ https://codereview.chromium.org/1636083003/diff/460001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.h:56: bool force_keyframe); Incorrect indent? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31: InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks ref_time) const& perhaps? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:37: DISALLOW_IMPLICIT_CONSTRUCTORS(InProgressFrameEncode); Is it unsafe to copy this struct? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:75: DCHECK(!encoder_task_weak_factory_.HasWeakPtrs()); This is probably not needed? The factory will be destroyed since this is the destructor and the pointers will be invalidated. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: VLOG(1) << "Failed creating compression session with hardware support."; Perhaps DLOG(ERROR)? Do we expect all Mac platforms to support hardware encode at 4k? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:103: profile.max_resolution = gfx::Size(kMaxResolutionWidth, kMaxResolutionHeight); I'm guessing there is no way to query this? Can we really support 4k, have you tried? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:217: if (!compression_session_) { We use compression_session_ on encoder_thread_ normally, apart from initialization and parameter change. Could we move the two latter tasks to encoder thread as well please? It should still ok to do it on child thread for GetSupportedProfiles. I think we'd only have to post an initialization task from Initialize and have a parameter change task as well? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:289: DLOG(ERROR) << " VTCompressionSessionEncodeFrame failed: " << status; Does VTCompressionSessionEncodeFrame free the request before failing, since we already release()d it? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:311: // This thread runs on |encoder_thread_| if it is alive, otherwise on GPU DCHECK(encoder_thread_.IsRunning() && encoder_thread_.task_runner()->BelongsToCurrentThread()); perhaps? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:355: encoder->encoder_task_weak_factory_.GetWeakPtr(), I don't think we can create a weak pointer on this thread, but dereference it on encoder thread (please see https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...). I think a solution here could be generating the weak pointer on encoder thread and then keeping it in the encoder class and using it from here, instead of generating it here. This is assuming that the strong encoder_opaque pointer will remain valid for all calls to this, since we block destruction of encoder in DestroyTask() until no more callbacks can come according to the comment there. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:424: CFTypeRef attributes_keys[] = { const? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:431: CFTypeRef attributes_values[] = { const? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:436: .release()}; static_assert(arraysize(attributes_keys) == arraysize(attributes_values), ...)? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:441: CFRelease(v); Do we need to explicitly do this, won't this happen when attributes falls out of scope when we return? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:448: // resolutions, we need to create a session. Would just passing false in l.444 have the same effect? https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:459: RequestEncodingParametersChange(target_bitrate_, frame_rate_); Do we want to Request if Configure fails? We will fail in the caller anyway I think...?
https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31: InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks ref_time) On 2016/03/10 05:25:35, Pawel Osciak wrote: > const& perhaps? Done. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:37: DISALLOW_IMPLICIT_CONSTRUCTORS(InProgressFrameEncode); On 2016/03/10 05:25:35, Pawel Osciak wrote: > Is it unsafe to copy this struct? I added after mcasas's comment on #21. It ensures that we do not make an unnecessary copy. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:75: DCHECK(!encoder_task_weak_factory_.HasWeakPtrs()); On 2016/03/10 05:25:35, Pawel Osciak wrote: > This is probably not needed? The factory will be destroyed since this is the > destructor and the pointers will be invalidated. I added after miu's suggestion on #59. It ensures that everything is cleaned as expected. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: VLOG(1) << "Failed creating compression session with hardware support."; On 2016/03/10 05:25:35, Pawel Osciak wrote: > Perhaps DLOG(ERROR)? Do we expect all Mac platforms to support hardware encode > at 4k? I added after miu's suggestion on #61. It helps to check support on different platforms. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:103: profile.max_resolution = gfx::Size(kMaxResolutionWidth, kMaxResolutionHeight); On 2016/03/10 05:25:35, Pawel Osciak wrote: > I'm guessing there is no way to query this? Can we really support 4k, have you > tried? Yes, there isn't a way to query these capabilities. I was able to create a session with 4K setting, but I didn't go through all Macs yet. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:217: if (!compression_session_) { On 2016/03/10 05:25:35, Pawel Osciak wrote: > We use compression_session_ on encoder_thread_ normally, apart from > initialization and parameter change. Could we move the two latter tasks to > encoder thread as well please? It should still ok to do it on child thread for > GetSupportedProfiles. > > I think we'd only have to post an initialization task from Initialize and have a > parameter change task as well? Initialize() is expected to return a bool on child thread. We would like to return false if we fail to create and set properties of compression_session_. So, I think, it would make more sense to do these on child thread. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:289: DLOG(ERROR) << " VTCompressionSessionEncodeFrame failed: " << status; On 2016/03/10 05:25:35, Pawel Osciak wrote: > Does VTCompressionSessionEncodeFrame free the request before failing, since we > already release()d it? I followed the same logic with these two implementations. There isn't a clear documentation I can point to, but I will be safe and release if only successful. https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:311: // This thread runs on |encoder_thread_| if it is alive, otherwise on GPU On 2016/03/10 05:25:35, Pawel Osciak wrote: > DCHECK(encoder_thread_.IsRunning() && > encoder_thread_.task_runner()->BelongsToCurrentThread()); perhaps? Done. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:355: encoder->encoder_task_weak_factory_.GetWeakPtr(), On 2016/03/10 05:25:35, Pawel Osciak wrote: > I don't think we can create a weak pointer on this thread, but dereference it on > encoder thread (please see > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...). > > I think a solution here could be generating the weak pointer on encoder thread > and then keeping it in the encoder class and using it from here, instead of > generating it here. > This is assuming that the strong encoder_opaque pointer will remain valid for > all calls to this, since we block destruction of encoder in DestroyTask() until > no more callbacks can come according to the comment there. Thanks. Done. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:424: CFTypeRef attributes_keys[] = { On 2016/03/10 05:25:35, Pawel Osciak wrote: > const? Done. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:431: CFTypeRef attributes_values[] = { On 2016/03/10 05:25:35, Pawel Osciak wrote: > const? CFDictionaryCreate() expects non-const input. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:436: .release()}; On 2016/03/10 05:25:35, Pawel Osciak wrote: > static_assert(arraysize(attributes_keys) == arraysize(attributes_values), ...)? CFDictionaryCreate() expects non-const input. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:441: CFRelease(v); On 2016/03/10 05:25:35, Pawel Osciak wrote: > Do we need to explicitly do this, won't this happen when attributes falls out of > scope when we return? Yes. We release() the attributes to create the array so we need destruct manually. Same logic applies here: https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:448: // resolutions, we need to create a session. On 2016/03/10 05:25:35, Pawel Osciak wrote: > Would just passing false in l.444 have the same effect? We want to set kVTVideoEncoderSpecification_RequireHardwareAcceleratedVideoEncoder as true in the first try to ensure we get HW support. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:459: RequestEncodingParametersChange(target_bitrate_, frame_rate_); On 2016/03/10 05:25:35, Pawel Osciak wrote: > Do we want to Request if Configure fails? We will fail in the caller anyway I > think...? Done.
LGTM FWIW. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31: InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks ref_time) On 2016/03/10 05:25:35, Pawel Osciak wrote: > const& perhaps? Negative, see [1]. There's even a number of presubmit checks catching this e.g. [2]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&s... [2] https://code.google.com/p/chromium/codesearch#chromium/src/cc/PRESUBMIT.py&q=...
Thanks. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31: InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks ref_time) On 2016/03/10 17:42:07, mcasas wrote: > On 2016/03/10 05:25:35, Pawel Osciak wrote: > > const& perhaps? > > Negative, see [1]. There's even a number > of presubmit checks catching this e.g. [2]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&s... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/cc/PRESUBMIT.py&q=... Reverted. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:424: CFTypeRef attributes_keys[] = { On 2016/03/10 06:51:21, emircan wrote: > On 2016/03/10 05:25:35, Pawel Osciak wrote: > > const? > > Done. Ignore my earlier reply. CFDictionaryCreate() expects non-const input.
https://chromiumcodereview.appspot.com/1636083003/diff/460001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right): https://chromiumcodereview.appspot.com/1636083003/diff/460001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.h:54: // Encoding tasks to be run on |encode_thread|. On 2016/03/10 05:25:35, Pawel Osciak wrote: > s/encode_thread/encoder_thread_/ Done. https://chromiumcodereview.appspot.com/1636083003/diff/460001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.h:56: bool force_keyframe); On 2016/03/10 05:25:35, Pawel Osciak wrote: > Incorrect indent? Done.
https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31: InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks ref_time) On 2016/03/10 18:53:20, emircan wrote: > On 2016/03/10 17:42:07, mcasas wrote: > > On 2016/03/10 05:25:35, Pawel Osciak wrote: > > > const& perhaps? > > > > Negative, see [1]. There's even a number > > of presubmit checks catching this e.g. [2]. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&s... > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/PRESUBMIT.py&q=... > > Reverted. Acknowledged. Good point Miguel. https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: VLOG(1) << "Failed creating compression session with hardware support."; On 2016/03/10 06:51:21, emircan wrote: > On 2016/03/10 05:25:35, Pawel Osciak wrote: > > Perhaps DLOG(ERROR)? Do we expect all Mac platforms to support hardware encode > > at 4k? > > I added after miu's suggestion on #61. It helps to check support on different > platforms. Perhaps the message should say something like "HW acceleration not available", to make the message not look like an error on platforms where it is not available? https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:217: if (!compression_session_) { On 2016/03/10 06:51:21, emircan wrote: > On 2016/03/10 05:25:35, Pawel Osciak wrote: > > We use compression_session_ on encoder_thread_ normally, apart from > > initialization and parameter change. Could we move the two latter tasks to > > encoder thread as well please? It should still ok to do it on child thread for > > GetSupportedProfiles. > > > > I think we'd only have to post an initialization task from Initialize and have > a > > parameter change task as well? > > Initialize() is expected to return a bool on child thread. We would like to > return false if we fail to create and set properties of compression_session_. > So, I think, it would make more sense to do these on child thread. You could block Initialize until that returned on a WaitableEvent. But I'm also ok with doing Initialize things on Child, before encoder thread is started. But parameter change should hopefully happen on encoder thread please. Better not to access compression_session_ concurrently from multiple threads. https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:448: // resolutions, we need to create a session. On 2016/03/10 06:51:21, emircan wrote: > On 2016/03/10 05:25:35, Pawel Osciak wrote: > > Would just passing false in l.444 have the same effect? > > We want to set > kVTVideoEncoderSpecification_RequireHardwareAcceleratedVideoEncoder as true in > the first try to ensure we get HW support. I think you could use kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder for that then? Doc says: "(...)If set to kCFBooleanTrue, use a hardware accelerated video encoder if available.(...)" https://chromiumcodereview.appspot.com/1636083003/diff/520001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://chromiumcodereview.appspot.com/1636083003/diff/520001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:295: CHECK(request.release()); Unless I'm missing something, we passed a raw pointer to VTCompressionSessionEncodeFrame(), so this frees it I believe, and since VTCompressionSessionEncodeFrame() didn't take a ref, now it has a dangling pointer?
https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: VLOG(1) << "Failed creating compression session with hardware support."; On 2016/03/11 00:43:59, Pawel Osciak wrote: > On 2016/03/10 06:51:21, emircan wrote: > > On 2016/03/10 05:25:35, Pawel Osciak wrote: > > > Perhaps DLOG(ERROR)? Do we expect all Mac platforms to support hardware > encode > > > at 4k? > > > > I added after miu's suggestion on #61. It helps to check support on different > > platforms. > > Perhaps the message should say something like "HW acceleration not available", > to make the message not look like an error on platforms where it is not > available? Done. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:217: if (!compression_session_) { On 2016/03/11 00:43:59, Pawel Osciak wrote: > On 2016/03/10 06:51:21, emircan wrote: > > On 2016/03/10 05:25:35, Pawel Osciak wrote: > > > We use compression_session_ on encoder_thread_ normally, apart from > > > initialization and parameter change. Could we move the two latter tasks to > > > encoder thread as well please? It should still ok to do it on child thread > for > > > GetSupportedProfiles. > > > > > > I think we'd only have to post an initialization task from Initialize and > have > > a > > > parameter change task as well? > > > > Initialize() is expected to return a bool on child thread. We would like to > > return false if we fail to create and set properties of compression_session_. > > So, I think, it would make more sense to do these on child thread. > > You could block Initialize until that returned on a WaitableEvent. But I'm also > ok with doing Initialize things on Child, before encoder thread is started. But > parameter change should hopefully happen on encoder thread please. Better not to > access compression_session_ concurrently from multiple threads. I see. I will move RequestEncodingParametersChange to the encoder thread. https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:448: // resolutions, we need to create a session. On 2016/03/11 00:43:59, Pawel Osciak wrote: > On 2016/03/10 06:51:21, emircan wrote: > > On 2016/03/10 05:25:35, Pawel Osciak wrote: > > > Would just passing false in l.444 have the same effect? > > > > We want to set > > kVTVideoEncoderSpecification_RequireHardwareAcceleratedVideoEncoder as true in > > the first try to ensure we get HW support. > > I think you could use > kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder for that > then? > Doc says: "(...)If set to kCFBooleanTrue, use a hardware accelerated video > encoder if available.(...)" As we discussed offline, I am setting it to false on l.444. https://codereview.chromium.org/1636083003/diff/520001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/520001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:295: CHECK(request.release()); On 2016/03/11 00:43:59, Pawel Osciak wrote: > Unless I'm missing something, we passed a raw pointer to > VTCompressionSessionEncodeFrame(), so this frees it I believe, and since > VTCompressionSessionEncodeFrame() didn't take a ref, now it has a dangling > pointer? I am updating the comment. If encode is successful, we release and let callback take care of it. Otherwise we can let if fall out of scope.
Patchset #20 (id:540001) has been deleted
https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:97: VLOG(1) << "Hardware support is not available on this platform."; Perhaps something like "Hardware encode acceleration is not..." https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:171: encoder_thread_.message_loop()->PostTask( Nit: you could in Initialize: encoder_thread_task_runner_ = encoder_thread_.task_runner(); and then: encoder_thread_task_runner_->PostTask() everywhere https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:174: encoder_weak_ptr_, Actually, unretained is ok, because we have a guarantee that the encoder thread will be destroyed before |this|, as it's a member of |this|. https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:278: CHECK(request.release()); Sorry, perhaps I'm missing something, however we are releasing this here, but then releasing it again in CompressionCallback from what I can see? https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:482: ->kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder()); I think Enable is not needed if we Require per docs... so perhaps: if (require_hw_encoding) { push back require } else { push back enable } ? https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:544: // This method may be called on |encoder thread| or GPU child thread. You could DCHECK((encoder_thread_.IsRunning() && <on encoder>) || <on child>) https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right): https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.h:54: // Encoding tasks to be run on |encoder_thread|. Nit: encoder_thread_
Patchset #20 (id:560001) has been deleted
https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:97: VLOG(1) << "Hardware support is not available on this platform."; On 2016/03/11 09:54:46, Pawel Osciak wrote: > Perhaps something like "Hardware encode acceleration is not..." Done. https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:171: encoder_thread_.message_loop()->PostTask( On 2016/03/11 09:54:46, Pawel Osciak wrote: > Nit: you could in Initialize: > > encoder_thread_task_runner_ = encoder_thread_.task_runner(); > > and then: > > encoder_thread_task_runner_->PostTask() > > everywhere Done. https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:174: encoder_weak_ptr_, On 2016/03/11 09:54:46, Pawel Osciak wrote: > Actually, unretained is ok, because we have a guarantee that the encoder thread > will be destroyed before |this|, as it's a member of |this|. I agree. I changed them to WeakPtr after my recent discussion about WeakPtrs vs Unretained where we concluded that it is much safer to avoid Unretained wherever we can. In this instance, Unretained is safe as you explained and WeakPtr is safe as |encoder_weak_ptr| is dereferenced and invalidated in dtor() on |encoder_thread_|. Still, if you think it is not a good practice or cause a different behavior, I can change. https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:278: CHECK(request.release()); On 2016/03/11 09:54:46, Pawel Osciak wrote: > Sorry, perhaps I'm missing something, however we are releasing this here, but > then releasing it again in CompressionCallback from what I can see? I pass request.get() to VTCompressionSessionEncodeFrame which is a naked ptr. - If encode is successful, I call release() here so that |request| doesn't get freed when it goes out of scope. In CompressionCallback, I wrap it into a scoped_ptr and let it go out of scope (by calling reset()). - If encode isn't successful, I let |request| go out of scope and get freed. The reason why I have CHECK() is to suppress unused result warning. https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:482: ->kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder()); On 2016/03/11 09:54:46, Pawel Osciak wrote: > I think Enable is not needed if we Require per docs... > > so perhaps: > > if (require_hw_encoding) { > push back require > } else { > push back enable > } > > ? Done. https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:544: // This method may be called on |encoder thread| or GPU child thread. On 2016/03/11 09:54:46, Pawel Osciak wrote: > You could DCHECK((encoder_thread_.IsRunning() && <on encoder>) || <on child>) Done. https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right): https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.h:54: // Encoding tasks to be run on |encoder_thread|. On 2016/03/11 09:54:46, Pawel Osciak wrote: > Nit: encoder_thread_ Done.
c/c/g/m/ lgtm % comment https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:174: encoder_weak_ptr_, On 2016/03/12 00:16:11, emircan wrote: > On 2016/03/11 09:54:46, Pawel Osciak wrote: > > Actually, unretained is ok, because we have a guarantee that the encoder > thread > > will be destroyed before |this|, as it's a member of |this|. > > I agree. I changed them to WeakPtr after my recent discussion about WeakPtrs vs > Unretained where we concluded that it is much safer to avoid Unretained wherever > we can. In this instance, Unretained is safe as you explained and WeakPtr is > safe as |encoder_weak_ptr| is dereferenced and invalidated in dtor() on > |encoder_thread_|. Still, if you think it is not a good practice or cause a > different behavior, I can change. Personally I'd prefer unretained please.
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jfroy@chromium.org, miu@chromium.org, sandersd@chromium.org, avi@chromium.org, thakis@chromium.org, mcasas@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1636083003/#ps620001 (title: "Rebase & posciak nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1636083003/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1636083003/620001
Thanks. https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... File content/common/gpu/media/vt_video_encode_accelerator_mac.cc (right): https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/med... content/common/gpu/media/vt_video_encode_accelerator_mac.cc:174: encoder_weak_ptr_, On 2016/03/15 07:27:47, Pawel Osciak wrote: > On 2016/03/12 00:16:11, emircan wrote: > > On 2016/03/11 09:54:46, Pawel Osciak wrote: > > > Actually, unretained is ok, because we have a guarantee that the encoder > > thread > > > will be destroyed before |this|, as it's a member of |this|. > > > > I agree. I changed them to WeakPtr after my recent discussion about WeakPtrs > vs > > Unretained where we concluded that it is much safer to avoid Unretained > wherever > > we can. In this instance, Unretained is safe as you explained and WeakPtr is > > safe as |encoder_weak_ptr| is dereferenced and invalidated in dtor() on > > |encoder_thread_|. Still, if you think it is not a good practice or cause a > > different behavior, I can change. > > Personally I'd prefer unretained please. Done.
Message was sent while issue was closed.
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding additional codec profiles and support for bitrate adaptations. 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding additional codec profiles and support for bitrate adaptations. 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 #22 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding additional codec profiles and support for bitrate adaptations. 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 ========== H264 HW encode using VideoToolbox This CL adds VTVideoEncodeAccelerator which enables H264 encode support using VideoToolbox on mac. Also, it includes a refactor of common VideoToolbox classes under video_toolbox_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. More patches will follow adding additional codec profiles and support for bitrate adaptations. 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/3956fa1cac34dd5682c271d77463accdd7191102 Cr-Commit-Position: refs/heads/master@{#381286} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/3956fa1cac34dd5682c271d77463accdd7191102 Cr-Commit-Position: refs/heads/master@{#381286}
Message was sent while issue was closed.
A revert of this CL (patchset #22 id:620001) has been created in https://codereview.chromium.org/1801343002/ by dewittj@chromium.org. The reason for reverting is: This probably breaks Mac compile: https://build.chromium.org/p/chromium/builders/Mac/builds/13208/steps/compile... FAILED: /b/build/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/content/common/gpu/media/jpeg_decode_accelerator_unittest.jpeg_decode_accelerator_unittest.o.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=263324-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_TOPCHROME_MD=1 -DFIELDTRIAL_TESTING_ENABLED -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=0 -DMOJO_USE_SYSTEM_IMPL -DUNIT_TEST -DGTEST_HAS_RTTI=0 -DSK_SUPPORT_GPU=1 -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DMESA_EGL_NO_X11_HEADERS -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen -I../../third_party/libva -I../../third_party/libyuv -I../../third_party/khronos -I../../gpu -I../.. -I../../skia/config -Igen/angle -I../../third_party/WebKit/Source -I../../third_party/opus/src/include -I../../testing/gtest/include -I../../third_party/libyuv/include -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/include/utils/mac -I../../skia/ext -I../../third_party/icu/source/i18n -I../../third_party/icu/source/common -I../../third_party/mesa/src/include -I../../third_party/WebKit -isysroot /Applications/Xcode5.1.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk -O2 -gdwarf-2 -fvisibility=hidden -Werror -mmacosx-version-min=10.6 -arch x86_64 -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wno-selector-type-mismatch -Wpartial-availability -Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration -Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing -Wno-deprecated-register -Wno-inconsistent-missing-override -Wno-shift-negative-value -std=c++11 -stdlib=libc++ -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang -load -Xclang /b/build/slave/Mac/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-templates -Xclang -plugin-arg-find-bad-constructs -Xclang follow-macro-expansion -fcolor-diagnostics -fno-strict-aliasing -c ../../content/common/gpu/media/jpeg_decode_accelerator_unittest.cc -o obj/content/common/gpu/media/jpeg_decode_accelerator_unittest.jpeg_decode_accelerator_unittest.o ../../content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:152:2: error: The JpegDecodeAccelerator is not supported on this platform. #error The JpegDecodeAccelerator is not supported on this platform. ^ 1 error generated. ninja: build stopped: subcommand failed..
Message was sent while issue was closed.
gyp issue is fixed and CL relanded on https://codereview.webrtc.org/1805723002/. |
