Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2058413003:
H264 HW encode using MediaFoundation (Closed)
|
Created:
4 years, 6 months ago by emircan Modified:
4 years, 5 months ago Reviewers:
Avi (use Gerrit), sandersd (OOO until July 31), wuchengli, ananta, Pawel Osciak, grt (UTC plus 2) CC:
chromium-reviews, posciak+watch_chromium.org, piman+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionH264 HW encode using MediaFoundation
This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support
using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common
MediaFoundation classes under mf_helpers.*.
Note that, this is the first CL and H264 codec is still behind a flag.
Design Doc(with perf measurements): http://goo.gl/UCnwyA
BUG=590060
TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and
"--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264
Also, added WIN specific sections at vea_unittests.
Committed: https://crrev.com/e75bb4429ddcf2e79ae678a544064069718ee7a0
Cr-Commit-Position: refs/heads/master@{#406876}
Patch Set 1 #
Total comments: 36
Patch Set 2 : ananta@ comments. #Patch Set 3 : Rebase. #Patch Set 4 : Refactor unittests. #Patch Set 5 : Rebase #
Total comments: 42
Patch Set 6 : grt@ and sanders@ comments. #
Total comments: 21
Patch Set 7 : grt@ and wuchengli@ comments. #
Total comments: 12
Patch Set 8 : grt@ comments. #
Total comments: 16
Patch Set 9 : ananta@ and sanders@ comments. #
Total comments: 6
Patch Set 10 : wuchengli@ comments. #
Total comments: 2
Patch Set 11 : wuchengli@ and ananta@ comments. #
Total comments: 1
Messages
Total messages: 91 (58 generated)
Description was changed from ========== cleanup moved folderz. adds. adaptive bitrate BUG= TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 patch from issue 1824243002 at patchset 40001 (http://crrev.com/1824243002#ps40001) ========== to ========== H264 Encoder for Win moved folderz. BUG= TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 patch from issue 1824243002 at patchset 40001 (http://crrev.com/1824243002#ps40001) ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== H264 Encoder for Win moved folderz. BUG= TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 patch from issue 1824243002 at patchset 40001 (http://crrev.com/1824243002#ps40001) ========== to ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 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: + ananta@chromium.org
Patchset #1 (id:40001) has been deleted
ananta@ PTAL for Media Foundation related code. I will send it for owners review afterwards.
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 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 MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-win-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ==========
Description was changed from ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-win-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-win-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ==========
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
emircan@chromium.org changed reviewers: + posciak@chromium.org
Patchset #1 (id:180001) has been deleted
Looks reasonable. General comment is please don't assume that a Windows API is going to succeed. For e.g. MFCreateMediaType and others. We don't want to crash if the API fails for some reason. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:8: // #error This file should only be built on Windows. Don't think we need this. The _win suffix should be good enough https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:90: profile.profile = media::H264PROFILE_BASELINE; Does this always have to be the baseline profile?. Does this mean that the other profiles won't be supported going forward?. Please add some notes to that effect. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:102: uint32_t initial_bitrate, The Initialize function is very big. Please split into smaller functions. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:115: } newline here. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:128: media::InitializeMediaFoundation(); newline here. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:152: encoder_thread_task_runner_ = encoder_thread_.task_runner(); newline https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:181: hr &= imf_output_media_type->SetGUID(MF_MT_MAJOR_TYPE, MFMediaType_Video); what happens if MFCreateMediaType fails?. Additionally is there a point in continuing if one of the Set operations fails below? https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:186: hr &= MFSetAttributeSize(imf_output_media_type.get(), MF_MT_FRAME_SIZE, Move this to the next line for consistency https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:199: hr &= imf_input_media_type->SetGUID(MF_MT_MAJOR_TYPE, MFMediaType_Video); Ditto for MFCreateMediaType and the other Set operations here https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:324: { newline here. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:341: input_sample_->SetSampleTime(frame->timestamp().InMicroseconds() * 10); newline here. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:347: ProcessOutput(); Perhaps we need to check if ProcessOutput succeeds. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:350: void MediaFoundationVideoEncodeAccelerator::ProcessOutput() { Should this function be returning void?. What happens if the ProcessOutput call fails below? https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:379: DWORD size = 0; What happens if GetBufferByIndex fails https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.h (right): https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:20: class MEDIA_GPU_EXPORT MediaFoundationVideoEncodeAccelerator Please add some documentation on what the threading model of this class looks like, i.e. which methods are queued to worker threads and how the whole thing is synchronized, etc. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:48: void ProcessOutput(); Please add newlines between the functions. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:49: void UseOutputBitstreamBufferTask( Some commentary about what these functions do would be very helpful for future reference. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:58: base::win::Version win_version_; Why do you need to save the version?
https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:8: // #error This file should only be built on Windows. On 2016/06/28 22:50:01, ananta wrote: > Don't think we need this. The _win suffix should be good enough Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:90: profile.profile = media::H264PROFILE_BASELINE; On 2016/06/28 22:50:01, ananta wrote: > Does this always have to be the baseline profile?. Does this mean that the other > profiles won't be supported going forward?. Please add some notes to that > effect. OpenH264 SW only supports baseline right now. In case of a SW fallback we want to be consistent with that. I can add additional profile if necessary as well later. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:102: uint32_t initial_bitrate, On 2016/06/28 22:50:01, ananta wrote: > The Initialize function is very big. Please split into smaller functions. Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:115: } On 2016/06/28 22:50:01, ananta wrote: > newline here. Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:128: media::InitializeMediaFoundation(); On 2016/06/28 22:50:00, ananta wrote: > newline here. Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:152: encoder_thread_task_runner_ = encoder_thread_.task_runner(); On 2016/06/28 22:50:01, ananta wrote: > newline Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:181: hr &= imf_output_media_type->SetGUID(MF_MT_MAJOR_TYPE, MFMediaType_Video); On 2016/06/28 22:50:01, ananta wrote: > what happens if MFCreateMediaType fails?. Additionally is there a point in > continuing if one of the Set operations fails below? I will add a return there if fails. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:186: hr &= MFSetAttributeSize(imf_output_media_type.get(), MF_MT_FRAME_SIZE, On 2016/06/28 22:50:01, ananta wrote: > Move this to the next line for consistency Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:199: hr &= imf_input_media_type->SetGUID(MF_MT_MAJOR_TYPE, MFMediaType_Video); On 2016/06/28 22:50:01, ananta wrote: > Ditto for MFCreateMediaType and the other Set operations here Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:324: { On 2016/06/28 22:50:01, ananta wrote: > newline here. Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:341: input_sample_->SetSampleTime(frame->timestamp().InMicroseconds() * 10); On 2016/06/28 22:50:00, ananta wrote: > newline here. Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:347: ProcessOutput(); On 2016/06/28 22:50:01, ananta wrote: > Perhaps we need to check if ProcessOutput succeeds. I answered below. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:350: void MediaFoundationVideoEncodeAccelerator::ProcessOutput() { On 2016/06/28 22:50:00, ananta wrote: > Should this function be returning void?. What happens if the ProcessOutput call > fails below? We are fine if it fails. - If it says MF_E_TRANSFORM_NEED_MORE_INPUT, we dont call this until after the next ProcessInput call. - We make recursive calls to ProcessOutput to flush out all the buffers. We stop when it says MF_E_TRANSFORM_NEED_MORE_INPUT. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:379: DWORD size = 0; On 2016/06/28 22:50:01, ananta wrote: > What happens if GetBufferByIndex fails Adding hr checks. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.h (right): https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:20: class MEDIA_GPU_EXPORT MediaFoundationVideoEncodeAccelerator On 2016/06/28 22:50:01, ananta wrote: > Please add some documentation on what the threading model of this class looks > like, i.e. which methods are queued to worker threads and how the whole thing is > synchronized, etc. Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:48: void ProcessOutput(); On 2016/06/28 22:50:01, ananta wrote: > Please add newlines between the functions. Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:49: void UseOutputBitstreamBufferTask( On 2016/06/28 22:50:01, ananta wrote: > Some commentary about what these functions do would be very helpful for future > reference. Done. https://codereview.chromium.org/2058413003/diff/200001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:58: base::win::Version win_version_; On 2016/06/28 22:50:01, ananta wrote: > Why do you need to save the version? Removed.
Description was changed from ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-win-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 ========== to ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-win-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ==========
Friendly PING on this. Rebasing takes quite a while.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Patchset #5 (id:280001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
emircan@chromium.org changed reviewers: + sandersd@chromium.org, wuchengli@chromium.org
PING again. Adding wuchengli@ and sandersd@ for /media/gpu OWNERS review.
grt@chromium.org changed reviewers: + grt@chromium.org
Some comments on the new _win file to get you started. Not sure I'm the best choice to take charge of the review, but happy to help get the ball rolling for you. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:47: EncodeOutput(int buffer_size, bool key_frame, base::TimeDelta timestamp) a DWORD is passed in as the size, so i suspect uint32_t is a better choice than int https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:48: : size(buffer_size), keyframe(key_frame), capture_timestamp(timestamp) { , data_(size) and remove data_.resize(size); below https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:53: const int size; this member is redundant. remove it and replace it with something like int size() const { return static_cast<int>(data_.size()); } https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:58: std::vector<uint8_t> data_; #include <vector> https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:59: DISALLOW_IMPLICIT_CONSTRUCTORS(EncodeOutput); shouldn't this be COPY_AND_ASSIGN since the implicit ctor vanished when you introduced your ctor? https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:80: encoder_weak_ptr_ = encoder_task_weak_factory_.GetWeakPtr(); remove this member and call encoder_task_weak_factory_.GetWeakPtr() in each Bind() https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:243: new BitstreamBufferRef(buffer.id(), std::move(shm), buffer.size())); #include <utility> https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:363: void MediaFoundationVideoEncodeAccelerator::EncodeTask( to avoid the trap of accidentally touching member variables that lived on the client's thread when running on the encoder thread, can you split this class into two: one that lives and dies on the client thread (or sequence) and one that lives and dies on the encoder thread? this makes it easier to reason about what's going on. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:520: frame_rate_ = framerate > 1 ? framerate : 1; either: std::max(1, framerate); since it says what you mean, or: framerate ? framerate : 1; since framerate is unsigned https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:531: hr &= imf_output_media_type->SetUINT32(MF_MT_AVG_BITRATE, target_bitrate_); do you really want to invoke a method on |imf_output_media_type| if the call above fails? https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.h (right): https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:5: #ifndef CONTENT_COMMON_GPU_MEDIA_MEDIA_FOUNDATION_VIDEO_ENCODE_ACCELERATOR_H_ CONTENT_COMMON -> MEDIA H_ -> WIN_H_ https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:25: : public media::VideoEncodeAccelerator { nit: omit "media::" here and elsewhere since you're in the namespace. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:70: std::unique_ptr<BitstreamBufferRef> buffer_ref); #include <memory> i see many other missing #includes. please review everything in this file for iwyu https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:79: void RequestEncodingParametersChangeTask(uint32_t bitrate, #include <stdint.h> for uint32_t https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:86: std::deque<std::unique_ptr<BitstreamBufferRef>> bitstream_buffer_queue_; #include <deque> https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:99: const scoped_refptr<base::SingleThreadTaskRunner> client_task_runner_; do you really require that the caller be bound to a specific thread rather than to a sequence? consider making this SequencedTaskRunner and use SequencedTaskRunnerHandle. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:105: std::unique_ptr<base::WeakPtrFactory<Client>> client_ptr_factory_; this seems backward. does base::CancelableCallback suit your needs? https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:134: }; nit: blank line after this https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:135: } // namespace content media
https://codereview.chromium.org/2058413003/diff/300001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2058413003/diff/300001/media/base/media_switc... media/base/media_switches.cc:59: const char kEnableWinHWH264Encoding[] = "enable-win-hw-h264-encoding"; Probably better to remove 'win' from the name. It's probably also a good idea to swap 'hw' for 'mf'/'mediafoundation', but of course that makes it a platform-specific flag again.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-win-hw-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ========== to ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ==========
https://codereview.chromium.org/2058413003/diff/300001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2058413003/diff/300001/media/base/media_switc... media/base/media_switches.cc:59: const char kEnableWinHWH264Encoding[] = "enable-win-hw-h264-encoding"; On 2016/07/14 22:38:04, sandersd wrote: > Probably better to remove 'win' from the name. > > It's probably also a good idea to swap 'hw' for 'mf'/'mediafoundation', but of > course that makes it a platform-specific flag again. Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:47: EncodeOutput(int buffer_size, bool key_frame, base::TimeDelta timestamp) On 2016/07/14 20:07:24, grt (UTC plus 2) wrote: > a DWORD is passed in as the size, so i suspect uint32_t is a better choice than > int Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:48: : size(buffer_size), keyframe(key_frame), capture_timestamp(timestamp) { On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > , data_(size) > and remove data_.resize(size); below Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:53: const int size; On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > this member is redundant. remove it and replace it with something like > int size() const { return static_cast<int>(data_.size()); } Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:58: std::vector<uint8_t> data_; On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > #include <vector> Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:59: DISALLOW_IMPLICIT_CONSTRUCTORS(EncodeOutput); On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > shouldn't this be COPY_AND_ASSIGN since the implicit ctor vanished when you > introduced your ctor? Done. I initially had this as a struct, and forgot to change it from then. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:80: encoder_weak_ptr_ = encoder_task_weak_factory_.GetWeakPtr(); On 2016/07/14 20:07:24, grt (UTC plus 2) wrote: > remove this member and call encoder_task_weak_factory_.GetWeakPtr() in each > Bind() Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:243: new BitstreamBufferRef(buffer.id(), std::move(shm), buffer.size())); On 2016/07/14 20:07:24, grt (UTC plus 2) wrote: > #include <utility> Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:363: void MediaFoundationVideoEncodeAccelerator::EncodeTask( On 2016/07/14 20:07:24, grt (UTC plus 2) wrote: > to avoid the trap of accidentally touching member variables that lived on the > client's thread when running on the encoder thread, can you split this class > into two: one that lives and dies on the client thread (or sequence) and one > that lives and dies on the encoder thread? this makes it easier to reason about > what's going on. Sorry, I am a little confused by the split you are suggesting here. The main reason we are using an encoder_thread_ here is so that we do not block the GPU main thread(client_thread) with any of these operations. As a result, all the other main thread functions are just trivial posting tasks. It is with the exception of Initialize() which requires to return a success bool, so even if I posted a task on encoder_thread_, I would have to wait till it returns. In this function, we do all the work on encoder_thread and post the result back to client_thread at the end of ProcessOutput(). l.404 is for the case when we run out of buffers, like the logic in https://cs.chromium.org/chromium/src/media/gpu/dxva_video_decode_accelerator_... https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:520: frame_rate_ = framerate > 1 ? framerate : 1; On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > either: > std::max(1, framerate); > since it says what you mean, or: > framerate ? framerate : 1; > since framerate is unsigned Changed both lines: frame_rate_ = framerate ? framerate : 1; target_bitrate_ = bitrate ? bitrate : 1; Also fixed below to use frame_rate_ instead of framerate. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:531: hr &= imf_output_media_type->SetUINT32(MF_MT_AVG_BITRATE, target_bitrate_); On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > do you really want to invoke a method on |imf_output_media_type| if the call > above fails? Added an early return for that case. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.h (right): https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:5: #ifndef CONTENT_COMMON_GPU_MEDIA_MEDIA_FOUNDATION_VIDEO_ENCODE_ACCELERATOR_H_ On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > CONTENT_COMMON -> MEDIA > H_ -> WIN_H_ Done. I must have missed it in one of the rebases. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:25: : public media::VideoEncodeAccelerator { On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > nit: omit "media::" here and elsewhere since you're in the namespace. Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:70: std::unique_ptr<BitstreamBufferRef> buffer_ref); On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > #include <memory> > i see many other missing #includes. please review everything in this file for > iwyu Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:79: void RequestEncodingParametersChangeTask(uint32_t bitrate, On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > #include <stdint.h> for uint32_t Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:86: std::deque<std::unique_ptr<BitstreamBufferRef>> bitstream_buffer_queue_; On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > #include <deque> Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:99: const scoped_refptr<base::SingleThreadTaskRunner> client_task_runner_; On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > do you really require that the caller be bound to a specific thread rather than > to a sequence? consider making this SequencedTaskRunner and use > SequencedTaskRunnerHandle. It does not specify in the VideoEncodeAccelerator interface, and all the implementations have checks to be pinned to the single thread [0][1][2]. I guess that is fine, since only calls come from GpuVideoEncodeAccelerator on GPU process. [3] [0] https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_encode_accelerator.... [1] https://cs.chromium.org/chromium/src/media/gpu/vt_video_encode_accelerator_ma... [2] https://cs.chromium.org/chromium/src/media/gpu/vaapi_video_encode_accelerator... [3] https://cs.chromium.org/chromium/src/media/gpu/ipc/service/gpu_video_encode_a... However, looking at the interface, I think there is no particular reason to be single threaded. I can discuss with other owners and add a TODO to change all implementations to SequencedTaskRunner as you suggest. WDYT? https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:105: std::unique_ptr<base::WeakPtrFactory<Client>> client_ptr_factory_; On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > this seems backward. does base::CancelableCallback suit your needs? We receive client as a naked ptr in VideoEncodeAccelerator::Initialize(). We cast it to be a base::WeakPtrFactory to have WeakPtrs for posting tasks. There are multiple callbacks on client as well, so a single callback wouldn't be enough here. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:134: }; On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > nit: blank line after this Done. https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:135: } // namespace content On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > media Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.h (right): https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:99: const scoped_refptr<base::SingleThreadTaskRunner> client_task_runner_; On 2016/07/15 04:58:31, emircan wrote: > On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > > do you really require that the caller be bound to a specific thread rather > than > > to a sequence? consider making this SequencedTaskRunner and use > > SequencedTaskRunnerHandle. > > It does not specify in the VideoEncodeAccelerator interface, and all the > implementations have checks to be pinned to the single thread [0][1][2]. I guess > that is fine, since only calls come from GpuVideoEncodeAccelerator on GPU > process. [3] > > [0] > https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_encode_accelerator.... > [1] > https://cs.chromium.org/chromium/src/media/gpu/vt_video_encode_accelerator_ma... > [2] > https://cs.chromium.org/chromium/src/media/gpu/vaapi_video_encode_accelerator... > [3] > https://cs.chromium.org/chromium/src/media/gpu/ipc/service/gpu_video_encode_a... > > However, looking at the interface, I think there is no particular reason to be > single threaded. I can discuss with other owners and add a TODO to change all > implementations to SequencedTaskRunner as you suggest. WDYT? Whether or not other parts of the video pipeline require thread affinity doesn't need to come into play here. A SingleThreadTaskRunner is a kind of SequencedTaskRunner. If this class here doesn't need to know that the caller is single threaded, it can take the client runner as a SequencedTaskRunner and all is well. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:157: HRESULT hr = MFTEnum(MFT_CATEGORY_VIDEO_ENCODER, flags, NULL, &output_info, are we better off supporting asynchronous MFTs from the get-go? https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:164: since you're using COM on the encoder thread, call encoder_thread_.init_com_with_mta(false); before Start(). i assume you want to use COM in a single threaded apartment. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:278: DestroyTask(); you can't call this function from the client's thread since it DCHECKs that it runs on the encoder thread. is the only case where the encoder thread wouldn't be running one where Initialize() didn't succeed? in that case, I suggest that you make all failure paths out of Initialize() release any resources so that Destroy() doesn't need any special handling. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:280: } it seems to be the responsibility of the accelerator to delete itself in this method. are you missing a "delete this;" here? as an aside: why does the accelerator work this way? why not have the owner delete it when its done, and have it do its own cleanup in its dtor? https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:532: hr &= imf_output_media_type->SetUINT32(MF_MT_AVG_BITRATE, target_bitrate_); &= is a bitwise AND, not a logical AND. is this really what you want? if any call succeeds (hr == S_OK == 0), subsequent errors will be ignored, no? https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:542: encoder_.Release(); i think you should invalidate all weak ptrs here, too, no?
I reviewed the following files. I'm not familiar with other windows files in media/gpu. media/gpu/BUILD.gn media/gpu/ipc/service/gpu_video_encode_accelerator.h/cc media/gpu/video_encode_accelerator_unittest.cc https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:138: const char* g_default_in_parameters = "&320&192&1&out.h264&200000"; Why do we need to change from ":" to "&"? Several ChromeOS autotest pass this string and will break. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:140: const char* g_default_in_parameters = "&320&192&0&out.h264&200000"; Can we make these all const base::FilePath::CharType*? So we can use he same variable for OS_MACOSX and OS_WIN. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:373: test_stream->in_filename = fields[0]; I don't understand. Why we only set in_filename for OS_POSIX and OS_WIN? https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:375: test_stream->in_filename = base::WideToUTF8(fields[0]); Make string conversion a function. It can be reused by line 301 and 1080. Maybe line 395 can use the same function if we use template. For example, static std::string WideToUTFIfOSWin(std::string str) { #if defined(OS_WIN) return base::WideToUTF8(str); #endif return str; } Feel free to think of a better function name.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.h (right): https://codereview.chromium.org/2058413003/diff/300001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:99: const scoped_refptr<base::SingleThreadTaskRunner> client_task_runner_; On 2016/07/15 07:59:03, grt (UTC plus 2) wrote: > On 2016/07/15 04:58:31, emircan wrote: > > On 2016/07/14 20:07:25, grt (UTC plus 2) wrote: > > > do you really require that the caller be bound to a specific thread rather > > than > > > to a sequence? consider making this SequencedTaskRunner and use > > > SequencedTaskRunnerHandle. > > > > It does not specify in the VideoEncodeAccelerator interface, and all the > > implementations have checks to be pinned to the single thread [0][1][2]. I > guess > > that is fine, since only calls come from GpuVideoEncodeAccelerator on GPU > > process. [3] > > > > [0] > > > https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_encode_accelerator.... > > [1] > > > https://cs.chromium.org/chromium/src/media/gpu/vt_video_encode_accelerator_ma... > > [2] > > > https://cs.chromium.org/chromium/src/media/gpu/vaapi_video_encode_accelerator... > > [3] > > > https://cs.chromium.org/chromium/src/media/gpu/ipc/service/gpu_video_encode_a... > > > > However, looking at the interface, I think there is no particular reason to be > > single threaded. I can discuss with other owners and add a TODO to change all > > implementations to SequencedTaskRunner as you suggest. WDYT? > > Whether or not other parts of the video pipeline require thread affinity doesn't > need to come into play here. A SingleThreadTaskRunner is a kind of > SequencedTaskRunner. If this class here doesn't need to know that the caller is > single threaded, it can take the client runner as a SequencedTaskRunner and all > is well. Sounds good. I am going to change this to SequencedTaskRunner and |thread_checker_| to SequenceChecker. Also, updating the comments about threading of this class. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:157: HRESULT hr = MFTEnum(MFT_CATEGORY_VIDEO_ENCODER, flags, NULL, &output_info, On 2016/07/15 07:59:03, grt (UTC plus 2) wrote: > are we better off supporting asynchronous MFTs from the get-go? I found out that async is only available in the newer Intel HD products. We can support the synchronous MFTs as a baseline. I will have a follow-up patch where I add async events support. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:164: On 2016/07/15 07:59:03, grt (UTC plus 2) wrote: > since you're using COM on the encoder thread, call > encoder_thread_.init_com_with_mta(false); before Start(). i assume you want to > use COM in a single threaded apartment. Done. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:278: DestroyTask(); On 2016/07/15 07:59:04, grt (UTC plus 2) wrote: > you can't call this function from the client's thread since it DCHECKs that it > runs on the encoder thread. is the only case where the encoder thread wouldn't > be running one where Initialize() didn't succeed? in that case, I suggest that > you make all failure paths out of Initialize() release any resources so that > Destroy() doesn't need any special handling. Done. I moved the encoder_thread_.Start() to the beginning of Initialize() so that we don't create any resources in the case it fails. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:280: } On 2016/07/15 07:59:04, grt (UTC plus 2) wrote: > it seems to be the responsibility of the accelerator to delete itself in this > method. are you missing a "delete this;" here? > > as an aside: why does the accelerator work this way? why not have the owner > delete it when its done, and have it do its own cleanup in its dtor? Added delete. I also read through the interface, but couldn't understand why it exactly works this way. It might be related to the specifics of GPU process and GpuVideoEncodeAccelerator. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:532: hr &= imf_output_media_type->SetUINT32(MF_MT_AVG_BITRATE, target_bitrate_); On 2016/07/15 07:59:03, grt (UTC plus 2) wrote: > &= is a bitwise AND, not a logical AND. is this really what you want? if any > call succeeds (hr == S_OK == 0), subsequent errors will be ignored, no? Thanks for pointing out. Changing them to bitwise or. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:542: encoder_.Release(); On 2016/07/15 07:59:04, grt (UTC plus 2) wrote: > i think you should invalidate all weak ptrs here, too, no? Done. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:138: const char* g_default_in_parameters = "&320&192&1&out.h264&200000"; On 2016/07/15 13:39:40, wuchengli wrote: > Why do we need to change from ":" to "&"? Several ChromeOS autotest pass this > string and will break. In windows file paths contain ":", i.e. C:\Program Files. I spent a long time finding out the issue with this actually. I can keep ":" as the seperator and write some "#if WIN" code to handle it specifically. With "&", at least the parsing code is uniform. WDYT? https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:140: const char* g_default_in_parameters = "&320&192&0&out.h264&200000"; On 2016/07/15 13:39:40, wuchengli wrote: > Can we make these all const base::FilePath::CharType*? So we can use he same > variable for OS_MACOSX and OS_WIN. Done. https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:373: test_stream->in_filename = fields[0]; On 2016/07/15 13:39:40, wuchengli wrote: > I don't understand. Why we only set in_filename for OS_POSIX and OS_WIN? FilePath is defined specifically for POSIX and WIN. POSIX covers mac and cros. https://cs.chromium.org/chromium/src/base/files/file_path.h?rcl=0&l=147 https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:375: test_stream->in_filename = base::WideToUTF8(fields[0]); On 2016/07/15 13:39:40, wuchengli wrote: > Make string conversion a function. It can be reused by line 301 and 1080. Maybe > line 395 can use the same function if we use template. > > For example, > static std::string WideToUTFIfOSWin(std::string str) { > #if defined(OS_WIN) > return base::WideToUTF8(str); > #endif > return str; > } > > Feel free to think of a better function name. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm getting near the end of my review of media_foundation_video_encode_accelerator_win.* here. Please be sure to get an OWNER of VideoEncodeAccelerator to review it as well. Keep in mind that I have no background in MF, so I can't really comment on whether or not its use is correct. https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:42: static const wchar_t* const kMediaFoundationVideoEncoderDLLs[] = { nit: no "static" in the unnamed namespace. please use constexpr for all compile-time constants like this (https://google.github.io/styleguide/cppguide.html#Use_of_constexpr): constexpr const wchar_t* kMediaFoundationVideoEncoderDLLs[] = { https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:271: encoder_thread_task_runner_->PostTask( could there potentially be many tasks queued up on the encoder thread at this point? if so, do you want to skip them so that DestroyTask runs immediately after any current task completes? https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:284: ::LoadLibrary(mfdll); is there any use in logging failures here in debug builds? https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:314: RETURN_ON_HR_FAILURE(hr, "Couldn't set output params", false); |hr| may be gibberish by the time you reach here due to all of the ORing, no? see https://msdn.microsoft.com/library/windows/desktop/ff485842.aspx for some inspiration in making sequences of COM calls. https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:479: // Keep calling ProcessOutput recursively until MF_E_TRANSFORM_NEED_MORE_INPUT how deep could this recursion go? https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.h (right): https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:37: ~MediaFoundationVideoEncodeAccelerator() override; should this be private? the contract seems to be that the caller must call new->Initialize->whatever->Destroy. this is a really odd interface. please check with OWNERS of VideoEncodeAccelerator to see what they say.
https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:42: static const wchar_t* const kMediaFoundationVideoEncoderDLLs[] = { On 2016/07/18 08:35:04, grt (UTC plus 2) wrote: > nit: no "static" in the unnamed namespace. please use constexpr for all > compile-time constants like this > (https://google.github.io/styleguide/cppguide.html#Use_of_constexpr): > constexpr const wchar_t* kMediaFoundationVideoEncoderDLLs[] = { Done. https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:271: encoder_thread_task_runner_->PostTask( On 2016/07/18 08:35:04, grt (UTC plus 2) wrote: > could there potentially be many tasks queued up on the encoder thread at this > point? if so, do you want to skip them so that DestroyTask runs immediately > after any current task completes? I think it makes more sense to finish those tasks first. For instance, Encode() followed by Destroy(), I would expect encoded frame. https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:284: ::LoadLibrary(mfdll); On 2016/07/18 08:35:04, grt (UTC plus 2) wrote: > is there any use in logging failures here in debug builds? l.139 would add the logs indicating missing dll. https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:314: RETURN_ON_HR_FAILURE(hr, "Couldn't set output params", false); On 2016/07/18 08:35:04, grt (UTC plus 2) wrote: > |hr| may be gibberish by the time you reach here due to all of the ORing, no? > see https://msdn.microsoft.com/library/windows/desktop/ff485842.aspx for some > inspiration in making sequences of COM calls. I am removing all |='s and treat all HResult individually. I was trying to cut down RETURN_ON_HR_FAILURE but it makes sense that it wont lead to any meaningful result. https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:479: // Keep calling ProcessOutput recursively until MF_E_TRANSFORM_NEED_MORE_INPUT On 2016/07/18 08:35:04, grt (UTC plus 2) wrote: > how deep could this recursion go? ProcessOutput() might have 3 outcomes: - HR succeed. Means we have output and we should continue recursion until all output is flushed. In local tests I have seen up to 4 output being flushed in a row. - HR=MF_E_TRANSFORM_NEED_MORE_INPUT(l.433) means we need more input so stop recursion. - All other HR errors stop recursion as well. https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.h (right): https://codereview.chromium.org/2058413003/diff/340001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.h:37: ~MediaFoundationVideoEncodeAccelerator() override; On 2016/07/18 08:35:04, grt (UTC plus 2) wrote: > should this be private? the contract seems to be that the caller must call > new->Initialize->whatever->Destroy. this is a really odd interface. please check > with OWNERS of VideoEncodeAccelerator to see what they say. I will make it protected as the interface suggests. I will ask wuchengli@ or posciak@ to take a look as OWNERS as well.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
win stuff lgtm. please get OWNERS approval as requested.
On 2016/07/18 20:50:57, grt (UTC plus 2) wrote: > win stuff lgtm. please get OWNERS approval as requested. Thank you very much. wuchengli/posciak can you PTAL at GpuVideoEncodeAccelerator interface use in media/gpu? sandersd@ can you PTAL at the rest of /media as OWNERS? ananta@ can you PTAL at MEdiaFoundation usage in media_foundation_video_encode_accelerator_win.*?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:7: #if defined(OS_WIN) Remove if defined() This file should only be compiled for windows. https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:54: int size() const { return static_cast<int>(data_.size()); } newline between methods. https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:60: DISALLOW_COPY_AND_ASSIGN(EncodeOutput); newline here. https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:138: if (!dll) { nuke dll and just use if (!::GetModuleHandle(mfdll)) https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:382: if (!encoder_) Should this just be a DCHECK?. From the code it seems like the only way this could happen is if Encode is called after a failed Initialize or called without Initialize. The check should be in ::Encode? https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:418: base::Bind(&MediaFoundationVideoEncodeAccelerator::EncodeTask, Do we need a PostTask here?. Just calling ProcessOutput and then retrying the EncodeTask would be simpler? and would also ensure that we don't keep trying to ProcessInput when the encoder gets into a bad state?
media/ lgtm (excluding gpu/ which I did not fully review). https://codereview.chromium.org/2058413003/diff/360001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2058413003/diff/360001/media/base/media_switc... media/base/media_switches.cc:58: // Enables H264 HW encode acceleration using Media Foundation for Win. Windows
https://codereview.chromium.org/2058413003/diff/360001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2058413003/diff/360001/media/base/media_switc... media/base/media_switches.cc:58: // Enables H264 HW encode acceleration using Media Foundation for Win. On 2016/07/18 23:50:53, sandersd wrote: > Windows Done. https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:7: #if defined(OS_WIN) On 2016/07/18 23:26:07, ananta wrote: > Remove if defined() > This file should only be compiled for windows. Done. https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:54: int size() const { return static_cast<int>(data_.size()); } On 2016/07/18 23:26:08, ananta wrote: > newline between methods. Done. https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:60: DISALLOW_COPY_AND_ASSIGN(EncodeOutput); On 2016/07/18 23:26:08, ananta wrote: > newline here. Done. https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:138: if (!dll) { On 2016/07/18 23:26:08, ananta wrote: > nuke dll and just use if (!::GetModuleHandle(mfdll)) Done. https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:382: if (!encoder_) On 2016/07/18 23:26:08, ananta wrote: > Should this just be a DCHECK?. From the code it seems like the only way this > could happen is if Encode is called after a failed Initialize or called without > Initialize. > The check should be in ::Encode? This is for the case when EncodeTask() is scheduled after DestroyTask() on encoder thread, when l.416 posts. https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:418: base::Bind(&MediaFoundationVideoEncodeAccelerator::EncodeTask, On 2016/07/18 23:26:08, ananta wrote: > Do we need a PostTask here?. Just calling ProcessOutput and then retrying the > EncodeTask would be simpler? and would also ensure that we don't keep trying to > ProcessInput when the encoder gets into a bad state? We already do call ProcessOutput() and retry encoding on l.413-414. After doing once, if the encoder is still not accepting input, I post task to delay this operation and let encoder thread do other work, like below. https://cs.chromium.org/chromium/src/media/gpu/dxva_video_decode_accelerator_... Tbh, I didn't come across to this case even in VEAUnittest busy tests. However, I would try again if encoder returns MF_E_NOTACCEPTING rather than dropping like other hr failures.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2058413003/diff/320001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:138: const char* g_default_in_parameters = "&320&192&1&out.h264&200000"; On 2016/07/16 06:56:38, emircan wrote: > On 2016/07/15 13:39:40, wuchengli wrote: > > Why do we need to change from ":" to "&"? Several ChromeOS autotest pass this > > string and will break. > > In windows file paths contain ":", i.e. C:\Program Files. I spent a long time > finding out the issue with this actually. I can keep ":" as the seperator and > write some "#if WIN" code to handle it specifically. With "&", at least the > parsing code is uniform. WDYT? - Uniform parsing code is better. But we need to migrate ChromeOS autotest. One way is to use "&" for #if OS_WIN and ":" for OS_CHROMEOS in this CL. In this CL, also add an command line flag to use "&". After ChromeOS picks up this CL, we can migrate autotest to pass the flag and use "&". Finally we can remove "&" in video_encode_accelerator_unittest.cc. Before you change this, let me discuss with Pawel tomorrow. - With "&", the string is not easy to parse by eyes. Is there other separator we can use? Or we can switch "&" and ";". - I found we need to add "" if we use "&" separator. For example, it used to be ./video_encode_accelerator_unittest --test_stream_data=bear_320x192_40frames.yuv:320:192:1:out1.h264:200000:30:200000:30 Now it's ./video_encode_accelerator_unittest --test_stream_data="bear_320x192_40frames.yuv&320&192&1&out1.h264&200000&30&200000&30" - Can you make sure this won't break Mac? They may have some existing code that pass : as separator like ChromeOS. https://codereview.chromium.org/2058413003/diff/380001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2058413003/diff/380001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:138: const char* g_default_in_parameters = "&320&192&1&out.h264&200000"; Can we also use base::FilePath::CharType and FILE_PATH_LITERAL here? const base::FilePath::CharType* g_default_in_parameters = FILE_PATH_LITERAL("&320&192&1&out.h264&200000"); https://codereview.chromium.org/2058413003/diff/380001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:1804: INSTANTIATE_TEST_CASE_P( I'm curious. Mac cannot pass the above tests? https://codereview.chromium.org/2058413003/diff/380001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:1828: #if defined(OS_WIN) Can't Mac pass this test? It will be nice if we don't run different tests on each platform.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2058413003/diff/380001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2058413003/diff/380001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:138: const char* g_default_in_parameters = "&320&192&1&out.h264&200000"; On 2016/07/19 15:50:44, wuchengli wrote: > Can we also use base::FilePath::CharType and FILE_PATH_LITERAL here? > > const base::FilePath::CharType* g_default_in_parameters = > FILE_PATH_LITERAL("&320&192&1&out.h264&200000"); Done. https://codereview.chromium.org/2058413003/diff/380001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:1804: INSTANTIATE_TEST_CASE_P( On 2016/07/19 15:50:44, wuchengli wrote: > I'm curious. Mac cannot pass the above tests? Switching bitrate and fps is problematic in Mac, as VideoToolbox library doesn't exactly hit the intended bitrate. As a result WebRTC(inside third_party) has a BitrateController to measure the actual bitrate and overshoot/undershoot accordingly. I integrated this to Chrome's VideoToolbox encoder as well. Bitrate Adjuster has its own unit tests. WebRTC CL: https://codereview.webrtc.org/1660963002 Chrome integration: https://codereview.chromium.org/1818903004 https://codereview.chromium.org/2058413003/diff/380001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:1828: #if defined(OS_WIN) On 2016/07/19 15:50:44, wuchengli wrote: > Can't Mac pass this test? It will be nice if we don't run different tests on > each platform. I agree that it would be ideal if we could run all the tests on all platforms. As I explained above, bitrate is problematic in Mac due to library and custom BitrateAdjuster usage. Windows can pass ForceBitrate test, but fails mid-stream switches. I will continue investigating if it is related to MediaFoundation codec api when I introduce async api in the followups.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: [WIP: Waiting to solve SW H264 issues on Win for performance comparison] BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ========== to ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: https://docs.google.com/a/chromium.org/document/d/1vKBBdc2VsBrp38WHUTRHyeBWE4... BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ==========
https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:418: base::Bind(&MediaFoundationVideoEncodeAccelerator::EncodeTask, On 2016/07/19 03:09:20, emircan wrote: > On 2016/07/18 23:26:08, ananta wrote: > > Do we need a PostTask here?. Just calling ProcessOutput and then retrying the > > EncodeTask would be simpler? and would also ensure that we don't keep trying > to > > ProcessInput when the encoder gets into a bad state? > > We already do call ProcessOutput() and retry encoding on l.413-414. After doing > once, if the encoder is still not accepting input, I post task to delay this > operation and let encoder thread do other work, like below. > > https://cs.chromium.org/chromium/src/media/gpu/dxva_video_decode_accelerator_... > > Tbh, I didn't come across to this case even in VEAUnittest busy tests. However, > I would try again if encoder returns MF_E_NOTACCEPTING rather than dropping like > other hr failures. That code in dxva appears to be wrong. If we get MF_E_NOTACCEPTING twice we should bail possibly with a CHECK to see if this case appears in the wild
https://codereview.chromium.org/2058413003/diff/400001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2058413003/diff/400001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:113: // "in_filename&width&height&profile&out_filename&requested_bitrate Pawel, Kuang-che and I discussed. - Can we use "," as separator? It's easier to parse by eyes. - To easier migrate ChromeOS autotest, please accept both "," and ":" as separator in this CL. It can try parsing by "," first. If there is no ",", try parsing ":". After ChromeOS picks up this CL, we'll update autotest. Does Mac have existing code that pass ":" to VEA unittest? Please make sure this CL doesn't break it.
Description was changed from ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: https://docs.google.com/a/chromium.org/document/d/1vKBBdc2VsBrp38WHUTRHyeBWE4... BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ========== to ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: http://goo.gl/UCnwyA BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ==========
https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/360001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:418: base::Bind(&MediaFoundationVideoEncodeAccelerator::EncodeTask, On 2016/07/20 01:57:22, ananta wrote: > On 2016/07/19 03:09:20, emircan wrote: > > On 2016/07/18 23:26:08, ananta wrote: > > > Do we need a PostTask here?. Just calling ProcessOutput and then retrying > the > > > EncodeTask would be simpler? and would also ensure that we don't keep trying > > to > > > ProcessInput when the encoder gets into a bad state? > > > > We already do call ProcessOutput() and retry encoding on l.413-414. After > doing > > once, if the encoder is still not accepting input, I post task to delay this > > operation and let encoder thread do other work, like below. > > > > > https://cs.chromium.org/chromium/src/media/gpu/dxva_video_decode_accelerator_... > > > > Tbh, I didn't come across to this case even in VEAUnittest busy tests. > However, > > I would try again if encoder returns MF_E_NOTACCEPTING rather than dropping > like > > other hr failures. > > That code in dxva appears to be wrong. If we get MF_E_NOTACCEPTING twice we > should bail possibly with a CHECK to see if this case appears in the wild Done. I will notify of an error if we fail twice and remove post task. https://codereview.chromium.org/2058413003/diff/400001/media/gpu/video_encode... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2058413003/diff/400001/media/gpu/video_encode... media/gpu/video_encode_accelerator_unittest.cc:113: // "in_filename&width&height&profile&out_filename&requested_bitrate On 2016/07/20 09:22:18, wuchengli wrote: > Pawel, Kuang-che and I discussed. > - Can we use "," as separator? It's easier to parse by eyes. > - To easier migrate ChromeOS autotest, please accept both "," and ":" as > separator in this CL. It can try parsing by "," first. If there is no ",", try > parsing ":". After ChromeOS picks up this CL, we'll update autotest. > > Does Mac have existing code that pass ":" to VEA unittest? Please make sure this > CL doesn't break it. Done. Thanks for the suggestion. I took the second approach such that we check "," first and ":" after. This way nothing should break in the integration.
The CQ bit was checked by emircan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I updated the Design Doc that includes performance measurements if you want to take a look: http://goo.gl/UCnwyA
Description was changed from ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc: http://goo.gl/UCnwyA BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ========== to ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc(with perf measurements): http://goo.gl/UCnwyA BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2058413003/diff/420001/media/gpu/media_founda... File media/gpu/media_foundation_video_encode_accelerator_win.cc (right): https://codereview.chromium.org/2058413003/diff/420001/media/gpu/media_founda... media/gpu/media_foundation_video_encode_accelerator_win.cc:499: // If there is already EncodeOutput waiting, copy its output first. encoded output?
emircan@chromium.org changed reviewers: + avi@chromium.org
Thanks. avi@ can you PTAL at content/* as OWNERS?
content lgtm
lgtm I reviewed gpu_video_encode_accelerator.h gpu_video_encode_accelerator.cc video_encode_accelerator_unittest.cc
The CQ bit was checked by emircan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2058413003/#ps420001 (title: "wuchengli@ and ananta@ comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc(with perf measurements): http://goo.gl/UCnwyA BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ========== to ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc(with perf measurements): http://goo.gl/UCnwyA BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ==========
Message was sent while issue was closed.
Committed patchset #11 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc(with perf measurements): http://goo.gl/UCnwyA BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. ========== to ========== H264 HW encode using MediaFoundation This CL adds MediaFoundationVideoEncodeAccelerator which enables H264 encode support using MediaFoundation on Windows 8.1+. Also, it includes a refactor of common MediaFoundation classes under mf_helpers.*. Note that, this is the first CL and H264 codec is still behind a flag. Design Doc(with perf measurements): http://goo.gl/UCnwyA BUG=590060 TEST= Tested AppRTC loopback with Chrome flag "--enable-webrtc-hw-h264-encoding" and "--enable-mf-h264-encoding" on https://apprtc.appspot.com/?debug=loopback&vsc=h264 Also, added WIN specific sections at vea_unittests. Committed: https://crrev.com/e75bb4429ddcf2e79ae678a544064069718ee7a0 Cr-Commit-Position: refs/heads/master@{#406876} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/e75bb4429ddcf2e79ae678a544064069718ee7a0 Cr-Commit-Position: refs/heads/master@{#406876}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:420001) has been created in https://codereview.chromium.org/2172703002/ by jiayl@chromium.org. The reason for reverting is: Build break: https://bugs.chromium.org/p/chromium/issues/detail?id=630335.
Message was sent while issue was closed.
Patchset #12 (id:440001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
