Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
826663002:
Support multiple video decoders and encoders (Closed)
|
Created:
5 years, 12 months ago by henryhsu Modified:
5 years, 11 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org, piman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport multiple video decoders and encoders
Some platforms have multiple video decoders and encoders.
For decode and encoder, return the first succeed initialized VDA and VEA
from all possible platforms. GetSupportedProfile return all possible profiles
from all encoders.
BUG=445016
TEST=Tested on squawks. For decoder, make sure V4L2 initialization
failed and VAAPI successed. For encoder, test on extension.
Committed: https://crrev.com/fce5ccd14c3676b960917c095a050b7a76b15227
Cr-Commit-Position: refs/heads/master@{#310232}
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix nits #
Total comments: 34
Patch Set 3 : address Pawel's review comments #
Total comments: 11
Patch Set 4 : address wucheng's review comments #
Total comments: 39
Patch Set 5 : address patch set 4 review comments #
Total comments: 4
Patch Set 6 : split CreateEncode/CreateDecoder for each platform #Patch Set 7 : Use callback function for decoders #
Total comments: 33
Patch Set 8 : address patch set 7 review comments #
Total comments: 2
Patch Set 9 : rebase #
Total comments: 1
Patch Set 10 : fix ozone #Patch Set 11 : apply Pawel's patch #Patch Set 12 : fix compile errors on some trybots #Patch Set 13 : fix trybot failure #
Total comments: 4
Patch Set 14 : Address piman's review comments #Messages
Total messages: 31 (3 generated)
henryhsu@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org
PTAL
+piman FYI https://chromiumcodereview.appspot.com/826663002/diff/1/content/common/gpu/me... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/826663002/diff/1/content/common/gpu/me... content/common/gpu/media/gpu_video_decode_accelerator.h:74: bool InitialDecoder(const media::VideoCodecProfile profile); s/Initial/Initialize/ s/const// https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:234: stub_->channel()->AddFilter(filter_.get()); Unless I'm missing something, if vda->Initialize() l.237 fails, we call this twice in a row, filter_ gets freed, but we pass it as raw pointer to channel, so channel keeps a dangling pointer, and also it doesn't need two filters? We don't need to call this twice. https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:309: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) && defined(USE_X11) What's the latency of failing to create a V4L2 decoder and retrying with VAAPI? Can we measure please? Perhaps we need to keep a cache of which profiles each of the decoders supports? https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.h:74: bool InitialDecoder(const media::VideoCodecProfile profile); Please explain the return value and more of what this does. https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.h:75: s/Initial/Initialize/ Also, no need for const. https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:100: if (!encoder_list_.size()) { if (encoder_list_.empty()) https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:108: input_visible_size, Indent. https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:112: encoder_ = encoder_list_[i]; Do we need to keep all the encoders, if we only use one? https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:174: ScopedVector<media::VideoEncodeAccelerator> Is the newline needed? https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:176: if (!encoder_list.size()) if empty() https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:181: profiles.insert(profiles.end(), vea_profiles.begin(), vea_profiles.end()); This may result in profiles containing duplicates, which is not preferable... https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... File content/common/gpu/media/gpu_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.h:70: // Create the appropriate platform-specific VEA. Please update comments. https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.h:71: static ScopedVector<media::VideoEncodeAccelerator> CreateEncoder(); CreateEncoders https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.h:100: // Owned pointer to all VideoEncodeAccelerator. s/pointer/pointers/ s/VideoEncodeAccelerator/VideoEncodeAccelerators/ https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.h:101: ScopedVector<media::VideoEncodeAccelerator> encoder_list_; s/encoder_list_/encoders_/, since it's not a list. https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.h:102: // Pointer to valid VideoEncodeAccelerator. What does "valid" mean here? https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/826663002/diff/20001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:601: if ((size_t)decoder_current_bitstream_buffer_->bytes_used == Please not c-style casts. Instead, please change bytes_used to size_t. https://chromiumcodereview.appspot.com/826663002/diff/20001/media/video/video... File media/video/video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/826663002/diff/20001/media/video/video... media/video/video_encode_accelerator.h:147: // Do not delete directly; use Destroy() or own it with a scoped_ptr, which Please don't remove protected.
henryhsu@chromium.org changed reviewers: + piman@chromium.org
all done. PTAL https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:234: stub_->channel()->AddFilter(filter_.get()); On 2014/12/26 01:11:35, Pawel Osciak wrote: > Unless I'm missing something, if vda->Initialize() l.237 fails, we call this > twice in a row, filter_ gets freed, but we pass it as raw pointer to channel, so > channel keeps a dangling pointer, and also it doesn't need two filters? > > We don't need to call this twice. I'd like to move AddFilter after VDA initialize. Then it will be called one time once vda initialize successfully. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:309: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) && defined(USE_X11) On 2014/12/26 01:11:35, Pawel Osciak wrote: > What's the latency of failing to create a V4L2 decoder and retrying with VAAPI? > Can we measure please? > Perhaps we need to keep a cache of which profiles each of the decoders supports? I tested on Squawks and the latency is 149 microseconds. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.h:74: bool InitialDecoder(const media::VideoCodecProfile profile); On 2014/12/26 01:11:35, Pawel Osciak wrote: > Please explain the return value and more of what this does. Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.h:75: On 2014/12/26 01:11:35, Pawel Osciak wrote: > s/Initial/Initialize/ > > Also, no need for const. Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:100: if (!encoder_list_.size()) { On 2014/12/26 01:11:35, Pawel Osciak wrote: > if (encoder_list_.empty()) Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:108: input_visible_size, On 2014/12/26 01:11:36, Pawel Osciak wrote: > Indent. Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:112: encoder_ = encoder_list_[i]; On 2014/12/26 01:11:36, Pawel Osciak wrote: > Do we need to keep all the encoders, if we only use one? Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:174: ScopedVector<media::VideoEncodeAccelerator> On 2014/12/26 01:11:36, Pawel Osciak wrote: > Is the newline needed? Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:176: if (!encoder_list.size()) On 2014/12/26 01:11:36, Pawel Osciak wrote: > if empty() Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:181: profiles.insert(profiles.end(), vea_profiles.begin(), vea_profiles.end()); On 2014/12/26 01:11:36, Pawel Osciak wrote: > This may result in profiles containing duplicates, which is not preferable... How to decide which profile should be used when VideoCodecProfile are the same but framerate and max_resolution are different. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... File content/common/gpu/media/gpu_video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.h:70: // Create the appropriate platform-specific VEA. On 2014/12/26 01:11:36, Pawel Osciak wrote: > Please update comments. Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.h:71: static ScopedVector<media::VideoEncodeAccelerator> CreateEncoder(); On 2014/12/26 01:11:36, Pawel Osciak wrote: > CreateEncoders Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.h:100: // Owned pointer to all VideoEncodeAccelerator. On 2014/12/26 01:11:36, Pawel Osciak wrote: > s/pointer/pointers/ > s/VideoEncodeAccelerator/VideoEncodeAccelerators/ encoder_list is not used. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.h:101: ScopedVector<media::VideoEncodeAccelerator> encoder_list_; On 2014/12/26 01:11:36, Pawel Osciak wrote: > s/encoder_list_/encoders_/, since it's not a list. Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.h:102: // Pointer to valid VideoEncodeAccelerator. On 2014/12/26 01:11:36, Pawel Osciak wrote: > What does "valid" mean here? Done. https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/20001/content/common/gpu/media... content/common/gpu/media/v4l2_video_decode_accelerator.cc:601: if ((size_t)decoder_current_bitstream_buffer_->bytes_used == On 2014/12/26 01:11:36, Pawel Osciak wrote: > Please not c-style casts. > Instead, please change bytes_used to size_t. Done. https://codereview.chromium.org/826663002/diff/20001/media/video/video_encode... File media/video/video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/20001/media/video/video_encode... media/video/video_encode_accelerator.h:147: // Do not delete directly; use Destroy() or own it with a scoped_ptr, which On 2014/12/26 01:11:36, Pawel Osciak wrote: > Please don't remove protected. oh ok. I removed this is due to ScopedVector. Now I use another implementation.
Supporting multiple decoders and encoders only works on x86 CrOS in this CL. Change the title and update the change description. I'll leave the rest of the review to Pawel. No need to wait for my review. https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:230: bool GpuVideoDecodeAccelerator::InitializeDecoder( Use the same order as the header file. Move this before OnDecode. https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:243: void GpuVideoDecodeAccelerator::Initialize( This function is getting big. But I don't see a way to extract some code to another function... https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:277: #elif defined(OS_CHROMEOS) && defined(USE_X11) Are there others CPU other than ARCH_CPU_ARMEL and ARCH_CPU_X86_FAMILY? If yes, this will incorrectly apply on those CPUs. If no, the include in line 35 can be extracted with ARM. https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:115: SendCreateEncoderReply(init_done_msg, true); You need to stop initializing and destroy other encoders. Otherwise, you'll send more than one reply. https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... File content/common/gpu/media/gpu_video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.h:11: #include "base/memory/scoped_vector.h" not used https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/v4l2_video_decode_accelerator.cc:77: size_t bytes_used; why this is changed?
all done. https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:230: bool GpuVideoDecodeAccelerator::InitializeDecoder( On 2014/12/26 09:26:25, wuchengli wrote: > Use the same order as the header file. Move this before OnDecode. Done. https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:277: #elif defined(OS_CHROMEOS) && defined(USE_X11) On 2014/12/26 09:26:25, wuchengli wrote: > Are there others CPU other than ARCH_CPU_ARMEL and ARCH_CPU_X86_FAMILY? If yes, > this will incorrectly apply on those CPUs. If no, the include in line 35 can be > extracted with ARM. Done. https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:115: SendCreateEncoderReply(init_done_msg, true); On 2014/12/26 09:26:25, wuchengli wrote: > You need to stop initializing and destroy other encoders. Otherwise, you'll send > more than one reply. Done. https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... File content/common/gpu/media/gpu_video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.h:11: #include "base/memory/scoped_vector.h" On 2014/12/26 09:26:25, wuchengli wrote: > not used Done. https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/40001/content/common/gpu/media... content/common/gpu/media/v4l2_video_decode_accelerator.cc:77: size_t bytes_used; On 2014/12/26 09:26:25, wuchengli wrote: > why this is changed? Compile warning when comparing bytes_used and size.
https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:230: void GpuVideoDecodeAccelerator::Initialize( Please document the behavior of this method. https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:265: (defined(ARCH_CPU_X86_FAMILY) || defined(ARCH_CPU_ARMEL)) I don't think it would hurt to not check for arch at all for v4l2vda. Instead, we should just make sure we have EGL (like we do with GLX for vavda). https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:295: if (InitializeDecoder(profile)) { I think the code would be simpler and more extensible (not just for this one "v4l2 and maybe vaapi after" case, but any number of decoders) if you just had a decoder scoped vector and pushed_back to it in the above code. Then just have a loop over the vector and try to initialize one by one and return if successful, dropping the rest. https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:301: // X86 platforms try V4L2 device first. If V4L2 device initialization fails, s/X86/x86/ s/V4L2 device/V4L2 VDA/ https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:302: // try VAAPI device again. s/VAAPI device/VAAPI VDA/ s/ again// https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:326: if (video_decode_accelerator_->CanDecodeOnIOThread()) { if (!vda_.get() || !vda_->Initialize()) return false; if (vda_->CDOIT()) { ... } return true; https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.h:62: // Initialize the accelerator with the given profile and send the Please update this to reflect the new behavior. https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.h:73: // Initialize VDA by |profile|. Pass filter to GpuCommandBufferStub s/by/for/ s/filter/filter_/ Also please mention that the filter is passed only if we can decode on IO. https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.h:74: // channel and return true when initialization successed. s/successed/succeeded/ https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:106: for (size_t i = 0; i < encoders.size(); ++i) { Please add comments what's going on here. https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:112: encoder_.reset(encoders[i]); We keep looping even if successful and overwrite the previous successful encoder? Is this what we prefer? https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:113: input_format_ = input_format; This could still stay outside of the loop. https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:116: encoders[i]->Destroy(); This assumes Destroy() deletes this, which it does now, but this is too shaky, it can change in the future and we have no protection against it. https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:175: GpuVideoEncodeAccelerator::GetSupportedProfiles() { Can we keep the profile list for later in a singleton and not recreate it every time? https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:177: std::vector<media::VideoEncodeAccelerator*> encoders = CreateEncoders(); Scopers please. https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:187: encoders[i]->Destroy(); Please don't depend on delete this in Destroy(). https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... File content/common/gpu/media/gpu_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.h:69: // Create and return the pointers of the appropriate platform-specific VEAs. s/of/to/ https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.h:70: static std::vector<media::VideoEncodeAccelerator*> CreateEncoders(); This makes me worried. Who owns the pointers? How do we ensure we free them properly in all cases? Why not a ScopedVector? https://chromiumcodereview.appspot.com/826663002/diff/60001/media/video/video... File media/video/video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/826663002/diff/60001/media/video/video... media/video/video_encode_accelerator.h:32: return profile < other.profile; This is not enough. If encoder A comes first, but supports a smaller range of resolutions than encoder B, we don't want to dismiss B's capabilities. Same for framerate. What we should do is look for profile among the already found profiles, if we find one, construct a superset of other values and update it. I'm not sure about having a '<' operator for this, it's is not a behavior one would expect. I think a regular vector would be small enough to not have a set at all.
all done. PTAL https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:230: void GpuVideoDecodeAccelerator::Initialize( On 2014/12/28 23:28:01, Pawel Osciak wrote: > Please document the behavior of this method. described in header file. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:265: (defined(ARCH_CPU_X86_FAMILY) || defined(ARCH_CPU_ARMEL)) On 2014/12/28 23:28:02, Pawel Osciak wrote: > I don't think it would hurt to not check for arch at all for v4l2vda. > Instead, we should just make sure we have EGL (like we do with GLX for vavda). How to check we have EGL? I think we cannot use 'gfx::GetGLImplementation() == gfx::kGLImplementationEGLGLES2'. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:295: if (InitializeDecoder(profile)) { On 2014/12/28 23:28:02, Pawel Osciak wrote: > I think the code would be simpler and more extensible (not just for this one > "v4l2 and maybe vaapi after" case, but any number of decoders) if you just had a > decoder scoped vector and pushed_back to it in the above code. > > Then just have a loop over the vector and try to initialize one by one and > return if successful, dropping the rest. yes. But ScopedVector does not support DefaultDeleter. We should call Destroy() by ourselves if we use ScopedVector. I use an array of scoped_ptr. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:301: // X86 platforms try V4L2 device first. If V4L2 device initialization fails, On 2014/12/28 23:28:02, Pawel Osciak wrote: > s/X86/x86/ > s/V4L2 device/V4L2 VDA/ Done. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:302: // try VAAPI device again. On 2014/12/28 23:28:01, Pawel Osciak wrote: > s/VAAPI device/VAAPI VDA/ > s/ again// Done. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.cc:326: if (video_decode_accelerator_->CanDecodeOnIOThread()) { On 2014/12/28 23:28:01, Pawel Osciak wrote: > if (!vda_.get() || !vda_->Initialize()) > return false; > > if (vda_->CDOIT()) { > ... > } > > return true; Done. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.h:62: // Initialize the accelerator with the given profile and send the On 2014/12/28 23:28:02, Pawel Osciak wrote: > Please update this to reflect the new behavior. Done. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.h:73: // Initialize VDA by |profile|. Pass filter to GpuCommandBufferStub On 2014/12/28 23:28:02, Pawel Osciak wrote: > s/by/for/ > s/filter/filter_/ > Also please mention that the filter is passed only if we can decode on IO. Done. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.h:74: // channel and return true when initialization successed. On 2014/12/28 23:28:02, Pawel Osciak wrote: > s/successed/succeeded/ Done. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:106: for (size_t i = 0; i < encoders.size(); ++i) { On 2014/12/28 23:28:03, Pawel Osciak wrote: > Please add comments what's going on here. Done. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:112: encoder_.reset(encoders[i]); On 2014/12/28 23:28:03, Pawel Osciak wrote: > We keep looping even if successful and overwrite the previous successful > encoder? Is this what we prefer? ok. let us keep the first successful encoder. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:113: input_format_ = input_format; On 2014/12/28 23:28:03, Pawel Osciak wrote: > This could still stay outside of the loop. will return directly if initialization succeeded. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:116: encoders[i]->Destroy(); On 2014/12/28 23:28:03, Pawel Osciak wrote: > This assumes Destroy() deletes this, which it does now, but this is too shaky, > it can change in the future and we have no protection against it. back to scoped_ptr https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:175: GpuVideoEncodeAccelerator::GetSupportedProfiles() { On 2014/12/28 23:28:03, Pawel Osciak wrote: > Can we keep the profile list for later in a singleton and not recreate it every > time? GetSupportedProfiels is only called one time when GPU process created. Do we still need singleton to save it? https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:177: std::vector<media::VideoEncodeAccelerator*> encoders = CreateEncoders(); On 2014/12/28 23:28:03, Pawel Osciak wrote: > Scopers please. SocopedVector doesn't support DefaultDeleter. I'll use scoped_ptr to split CreateEncoders. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:187: encoders[i]->Destroy(); On 2014/12/28 23:28:03, Pawel Osciak wrote: > Please don't depend on delete this in Destroy(). Done. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... File content/common/gpu/media/gpu_video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.h:69: // Create and return the pointers of the appropriate platform-specific VEAs. On 2014/12/28 23:28:03, Pawel Osciak wrote: > s/of/to/ Done. https://codereview.chromium.org/826663002/diff/60001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.h:70: static std::vector<media::VideoEncodeAccelerator*> CreateEncoders(); On 2014/12/28 23:28:03, Pawel Osciak wrote: > This makes me worried. Who owns the pointers? How do we ensure we free them > properly in all cases? > > Why not a ScopedVector? ScopedVector doesn't support DefaultDeleter. As discussed, I'll keep to use scoped_ptr and duplicate CreateEncoder in profile and initialize function. https://codereview.chromium.org/826663002/diff/60001/media/video/video_encode... File media/video/video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/60001/media/video/video_encode... media/video/video_encode_accelerator.h:32: return profile < other.profile; On 2014/12/28 23:28:03, Pawel Osciak wrote: > This is not enough. If encoder A comes first, but supports a smaller range of > resolutions than encoder B, we don't want to dismiss B's capabilities. Same for > framerate. > > What we should do is look for profile among the already found profiles, if we > find one, construct a superset of other values and update it. > > I'm not sure about having a '<' operator for this, it's is not a behavior one > would expect. I think a regular vector would be small enough to not have a set > at all. as discussed, save a list of all profiles if some parameters are different.
https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/826663002/diff/60001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.cc:265: (defined(ARCH_CPU_X86_FAMILY) || defined(ARCH_CPU_ARMEL)) On 2014/12/29 09:43:26, henryhsu wrote: > On 2014/12/28 23:28:02, Pawel Osciak wrote: > > I don't think it would hurt to not check for arch at all for v4l2vda. > > Instead, we should just make sure we have EGL (like we do with GLX for vavda). > > How to check we have EGL? I think we cannot use 'gfx::GetGLImplementation() == > gfx::kGLImplementationEGLGLES2'. We'll have a separate CL for this, so we can skip it for now. https://chromiumcodereview.appspot.com/826663002/diff/80001/content/common/gp... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/826663002/diff/80001/content/common/gp... content/common/gpu/media/gpu_video_decode_accelerator.h:63: // |init_done_msg| when initialize one of accelerators with |profile| We send the message always, just with a different result depending on the outcome. https://chromiumcodereview.appspot.com/826663002/diff/80001/content/common/gp... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/826663002/diff/80001/content/common/gp... content/common/gpu/media/gpu_video_encode_accelerator.cc:100: CreateDefaultEncoder(), CreateVaapiEncoder() Oh, this is not a bad idea! But I'd instead have a vector of callback/factory methods, to which you could push back and run them in a loop. Maybe this could work: typedef base::Callback<scoped_ptr<media::VideoEncodeAccelerator>(void)> CreateVEACb; std::vector<CreateVEACb> create_vea_cbs; #if foo create_vea_cbs.push_back(&CreateV4L2VEA) #else if bar create_vea_cbs.push_back(&CreateVAVEA) #... #endif etc. and then for () { encoder_ = create_vea_cbs[i].Run(); if (encoder_->Initialize()) { etc... } } And we could do the same for VDAs.
PTAL https://codereview.chromium.org/826663002/diff/80001/content/common/gpu/media... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/80001/content/common/gpu/media... content/common/gpu/media/gpu_video_decode_accelerator.h:63: // |init_done_msg| when initialize one of accelerators with |profile| On 2014/12/30 06:14:38, Pawel Osciak wrote: > We send the message always, just with a different result depending on the > outcome. Done. https://codereview.chromium.org/826663002/diff/80001/content/common/gpu/media... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/80001/content/common/gpu/media... content/common/gpu/media/gpu_video_encode_accelerator.cc:100: CreateDefaultEncoder(), CreateVaapiEncoder() On 2014/12/30 06:14:38, Pawel Osciak wrote: > Oh, this is not a bad idea! But I'd instead have a vector of callback/factory > methods, to which you could push back and run them in a loop. Maybe this could > work: > > typedef base::Callback<scoped_ptr<media::VideoEncodeAccelerator>(void)> > CreateVEACb; > > std::vector<CreateVEACb> create_vea_cbs; > > #if foo > create_vea_cbs.push_back(&CreateV4L2VEA) > #else if bar > create_vea_cbs.push_back(&CreateVAVEA) > #... > #endif > > etc. > > and then > > for () { > encoder_ = create_vea_cbs[i].Run(); > if (encoder_->Initialize()) { > etc... > } > } > > And we could do the same for VDAs. Good idea!
decoders is also using callback function now.
https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:253: if (create_vda_cbs.empty()) { I think not needed, since the for loop will not do anything and we'll return false at the end? https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:260: if (!video_decode_accelerator_.get() || IIRC, we don't need get()s on scoped_ptr? https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:278: #if defined(OS_WIN) Technically, we don't need all the #ifs here. The callbacks will return empty pointers and we'll save some #if ugliness. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:312: } else Per coding style both branches need a brace if one has it. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:34: typedef base::Callback<scoped_ptr<media::VideoDecodeAccelerator>(void)> This could be private? https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:65: // Prepare all appropriate platform-specific VDAs and initialize VDAs with The final effect is only one initialized VDA, perhaps: "Initialize VDAs from the set of VDAs supported for current platform until one of them succeeds for given |profile|." https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:74: // Create the appropriate platform-specific VDA callback functions. Return a set of VDA Create callback functions applicable to the current platform. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:76: // Create DXVA VDA. These comments are probably unnecessary, method names are self-explanatory. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:77: scoped_ptr<media::VideoDecodeAccelerator> CreateDXVADecoder(); Could we please s/Decoder/VDA/ everywhere? https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:101: if (create_vea_cbs.empty()) { Not needed? https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:178: if (create_vea_cbs.empty()) Not needed, for() handles this? https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:218: #if defined(OS_CHROMEOS) && defined(USE_X11) Same as in gvda.cc https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.h:34: typedef base::Callback<scoped_ptr<media::VideoEncodeAccelerator>(void)> private please. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.h:72: // Create the appropriate platform-specific VEA callback functions. Same comments as in gvda.h please. https://codereview.chromium.org/826663002/diff/120001/content/content_common.... File content/content_common.gypi (right): https://codereview.chromium.org/826663002/diff/120001/content/content_common.... content/content_common.gypi:766: 'common/gpu/media/tegra_v4l2_video_device.cc', Hm I think we may want to get this out to an arm conditional. https://codereview.chromium.org/826663002/diff/120001/media/video/video_encod... File media/video/video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/120001/media/video/video_encod... media/video/video_encode_accelerator.h:31: bool operator< (const SupportedProfile& other) const { Leftover?
all done. PTAL https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:253: if (create_vda_cbs.empty()) { On 2014/12/31 07:55:59, Pawel Osciak wrote: > I think not needed, since the for loop will not do anything and we'll return > false at the end? Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:260: if (!video_decode_accelerator_.get() || On 2014/12/31 07:55:59, Pawel Osciak wrote: > IIRC, we don't need get()s on scoped_ptr? yes. We don't need. This references other codes in this file for consistency. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:278: #if defined(OS_WIN) On 2014/12/31 07:55:59, Pawel Osciak wrote: > Technically, we don't need all the #ifs here. The callbacks will return empty > pointers and we'll save some #if ugliness. Sure. Then we will have many callback functions with empty pointers. I think it is ok. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:312: } else On 2014/12/31 07:55:59, Pawel Osciak wrote: > Per coding style both branches need a brace if one has it. Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:34: typedef base::Callback<scoped_ptr<media::VideoDecodeAccelerator>(void)> On 2014/12/31 07:55:59, Pawel Osciak wrote: > This could be private? Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:65: // Prepare all appropriate platform-specific VDAs and initialize VDAs with On 2014/12/31 07:55:59, Pawel Osciak wrote: > The final effect is only one initialized VDA, perhaps: > > "Initialize VDAs from the set of VDAs supported for current platform until one > of them succeeds for given |profile|." Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:74: // Create the appropriate platform-specific VDA callback functions. On 2014/12/31 07:55:59, Pawel Osciak wrote: > Return a set of VDA Create callback functions applicable to the current > platform. Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:76: // Create DXVA VDA. On 2014/12/31 07:55:59, Pawel Osciak wrote: > These comments are probably unnecessary, method names are self-explanatory. Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:77: scoped_ptr<media::VideoDecodeAccelerator> CreateDXVADecoder(); On 2014/12/31 07:55:59, Pawel Osciak wrote: > Could we please s/Decoder/VDA/ everywhere? Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:101: if (create_vea_cbs.empty()) { On 2014/12/31 07:56:00, Pawel Osciak wrote: > Not needed? Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:178: if (create_vea_cbs.empty()) On 2014/12/31 07:56:00, Pawel Osciak wrote: > Not needed, for() handles this? Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:218: #if defined(OS_CHROMEOS) && defined(USE_X11) On 2014/12/31 07:56:00, Pawel Osciak wrote: > Same as in gvda.cc Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.h:34: typedef base::Callback<scoped_ptr<media::VideoEncodeAccelerator>(void)> On 2014/12/31 07:56:00, Pawel Osciak wrote: > private please. Done. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.h:72: // Create the appropriate platform-specific VEA callback functions. On 2014/12/31 07:56:00, Pawel Osciak wrote: > Same comments as in gvda.h please. Done. https://codereview.chromium.org/826663002/diff/120001/content/content_common.... File content/content_common.gypi (right): https://codereview.chromium.org/826663002/diff/120001/content/content_common.... content/content_common.gypi:766: 'common/gpu/media/tegra_v4l2_video_device.cc', On 2014/12/31 07:56:00, Pawel Osciak wrote: > Hm I think we may want to get this out to an arm conditional. Done. https://codereview.chromium.org/826663002/diff/120001/media/video/video_encod... File media/video/video_encode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/120001/media/video/video_encod... media/video/video_encode_accelerator.h:31: bool operator< (const SupportedProfile& other) const { On 2014/12/31 07:56:00, Pawel Osciak wrote: > Leftover? ok. as discussed, return full set of profiles regardless of duplicated or not.
lgtm % nits Also, I'd suggest changing the CL description to not specifically mention CrOS x86 and the order in which we try codecs; this now allows having multiple codecs on all platforms and we don't really have an explicit order now. https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/120001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:260: if (!video_decode_accelerator_.get() || On 2014/12/31 08:47:53, henryhsu wrote: > On 2014/12/31 07:55:59, Pawel Osciak wrote: > > IIRC, we don't need get()s on scoped_ptr? > > yes. We don't need. This references other codes in this file for consistency. Not sure if we need this kind of consistency ;) But we can have a sweep over this file some other time. https://codereview.chromium.org/826663002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/826663002/diff/140001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.h:64: // done. filter_ is passed to GpuCommandBufferStube channel only if we can s/Stube/Stub/ s/we/the chosen VDA/ https://codereview.chromium.org/826663002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/140001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:206: create_vea_cbs.push_back(base::Bind(&CreateV4L2VEA)); Nit: please prepend with GpuVideoEncodeAccelerator::
Ah, and please also test encoder on WebRTC (apprtc is fine).
And it looks like we need a rebase too.
On 2015/01/02 02:19:16, Pawel Osciak wrote: > And it looks like we need a rebase too. Oh, and please also update content/common/BUILD.gn accordingly.
https://chromiumcodereview.appspot.com/826663002/diff/160001/content/common/g... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/826663002/diff/160001/content/common/g... content/common/gpu/media/gpu_video_decode_accelerator.cc:358: #if defined(USE_OZONE) This fails on freon boards. It should be: #if !defined(OS_CHROMEOS) && defined(USE_OZONE)
apprtc works well on peach_pit and squawks. In squawks, decoder initialize shows "HW video decode acceleration not available." is expected.
Hi piman, PTAL
https://codereview.chromium.org/826663002/diff/240001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/240001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:273: std::vector<GpuVideoDecodeAccelerator::CreateVDACb> create_vda_cbs; This seems overkill to allocate a vector and fill it up with callbacks. Can't we simply have a static array of pointer-to-member-functions? Something along the lines of: typedef void (GpuVideoDecodeAccelerator:*CreateVDA)(); static const CreateVDA kCreateVDAs[] = { &GpuVideoDecodeAccelerator::CreateDXVAVDA, &GpuVideoDecodeAccelerator::CreateV4L2VDA, [...] }; Then you just call this->*kCreateVDAs[i]() on line 254. (using arraysize instead of vector size). Bonus points if the array only includes the relevant factory methods (use ifdefs to avoid code bloat) https://codereview.chromium.org/826663002/diff/240001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/240001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:202: std::vector<GpuVideoEncodeAccelerator::CreateVEACb> create_vea_cbs; Same wrt static array. They're all static too, so base::Bind really doesn't make much sense.
all done. PTAL https://codereview.chromium.org/826663002/diff/240001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/240001/content/common/gpu/medi... content/common/gpu/media/gpu_video_decode_accelerator.cc:273: std::vector<GpuVideoDecodeAccelerator::CreateVDACb> create_vda_cbs; On 2015/01/05 23:50:05, piman (Very slow to review) wrote: > This seems overkill to allocate a vector and fill it up with callbacks. > Can't we simply have a static array of pointer-to-member-functions? > > Something along the lines of: > > typedef void (GpuVideoDecodeAccelerator:*CreateVDA)(); > static const CreateVDA kCreateVDAs[] = { > &GpuVideoDecodeAccelerator::CreateDXVAVDA, > &GpuVideoDecodeAccelerator::CreateV4L2VDA, > [...] > }; > > Then you just call this->*kCreateVDAs[i]() on line 254. (using arraysize instead > of vector size). > > Bonus points if the array only includes the relevant factory methods (use ifdefs > to avoid code bloat) CreateDXVAVDA functions are private and not static. So I remove callback and keep to use a function to return a set of function pointers. https://codereview.chromium.org/826663002/diff/240001/content/common/gpu/medi... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/826663002/diff/240001/content/common/gpu/medi... content/common/gpu/media/gpu_video_encode_accelerator.cc:202: std::vector<GpuVideoEncodeAccelerator::CreateVEACb> create_vea_cbs; On 2015/01/05 23:50:06, piman (Very slow to review) wrote: > Same wrt static array. They're all static too, so base::Bind really doesn't make > much sense. For consistency, I remove callback function and return a set of function pointers as decoder.
LGTM, but I would still prefer if we didn't add to the list the factory functions that are noop (with ifdefs).
On 2015/01/06 17:48:50, piman (Very slow to review) wrote: > LGTM, but I would still prefer if we didn't add to the list the factory > functions that are noop (with ifdefs).
The CQ bit was checked by henryhsu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/826663002/260001
On 2015/01/06 17:48:50, piman (Very slow to review) wrote: > LGTM, but I would still prefer if we didn't add to the list the factory > functions that are noop (with ifdefs). It's not ideal, but I'd prefer not having ifdefs in two places (i.e. in GpuVideoDecodeAccelerator::CreateVDAFps and in each creation method). More opportunity to mess things up later on if we need to keep both in sync.
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/fce5ccd14c3676b960917c095a050b7a76b15227 Cr-Commit-Position: refs/heads/master@{#310232}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/832383004/ by nhiroki@chromium.org. The reason for reverting is: This causes compile failure on "Linux ChromiumOS Ozone Builder Build" http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2.... |
