Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1016773002:
MJPEG acceleration for video capture using VAAPI (Closed)
|
Created:
5 years, 9 months ago by kcwu Modified:
5 years, 7 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMJPEG acceleration for video capture using VAAPI
The latency and cpu usage (including sys) to decode 1280x720 image:
software decode: latency 10.2ms, cpu 28.2%
hardware decode: latency 5.5ms, cpu 26.7%
Tested on peppy chromebook (haswell) using apprtc.appspot.com
(not connected yet and no loopback)
Default cpu scaling governor.
BUG=335778
TEST=apprtc loopback
Patch Set 1 #Patch Set 2 : fix coded size, shm handle #
Total comments: 56
Patch Set 3 : fix many thread issues #
Total comments: 36
Patch Set 4 : address most comments #
Total comments: 130
Patch Set 5 : Move JPEG related code to separated file. And address some other comments #
Total comments: 47
Patch Set 6 : #
Total comments: 36
Patch Set 7 : #Patch Set 8 : rename GpuJpegDecodeAcceleratorAdapter to GpuJpegDecoder #
Total comments: 42
Patch Set 9 : support multiple jpeg decoder #
Total comments: 91
Patch Set 10 : (hide change of base/containers/scoped_ptr_hash_map.h in https://codereview.chromium.org/1099383002… #
Total comments: 22
Patch Set 11 : #Patch Set 12 : rebase #Patch Set 13 : fix AsPlatformHandle #Patch Set 14 : reuse VASurface #
Total comments: 53
Patch Set 15 : jpeg thread; no fallback; SendBuffer with bytes_used #Patch Set 16 : rebase #
Total comments: 56
Messages
Total messages: 36 (3 generated)
kcwu@chromium.org changed reviewers: + henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org
I'm still working on this CL (it's known to crash, leaks, etc.) If you want to review, please review GPU side first (vaapi_jpeg_decode_accelerator and gpu_jpeg_decode_accelerator). Browser side is still mess now.
https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.cc:172: int buffer_id) { Need to acquire |lock_|. Or how about merging this with GetBufferInfo and store the handle in media::VideoCaptureDevice::Client::Buffer? https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:85: int32_t bitstream_buffer_id) { Add DCHECK to make sure this runs on IO thread. Then you can call DoIncomingCapturedVideoFrameOnIOThread directly. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:86: auto map_it = jpeg_decode_task_map_.find(bitstream_buffer_id); Like you said, since you are blocking the v4l2 thread, you don't need a map. Let's assume the decode latency is low and we can block the v4l2 thread. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:160: OnIncomingCapturedData2(captured_data); Please measure the decode time of both cases and put it in the change description. The time between here and before calling DoIncomingCapturedVideoFrameOnIOThread. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:196: base::Closure()); Set framerate like line 387. frame->metadata()->SetDouble(media::VideoFrameMetadata::FRAME_RATE, https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:203: jpeg_decode_event_ = new base::WaitableEvent(false, false); Use scoped_ptr https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:209: &VideoCaptureDeviceClient::DecodeJpegOnIOThread, Post to JpegDecodeAccelerator::Decode directly. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_channel_host.h (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.h:162: GpuJpegDecodeAcceleratorHost* CreateJpegDecoder(); Return scoped_ptr<media::JpegDecodeAccelerator> to be consistent with CreateVideoDecoder and CreateVideoEncoder. Move this after CreateVideoEncoder. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:80: BrowserThread::PostTask( Looks like GJDAH is accessed entirely on IO thread. Right? So you should call GJDAH::Initialize also on IO thread. Then you don't need to PostTask here. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:805: (void)decoder; // it will delete itself?? No. I think you need some kind of DestructionObserver. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.h (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.h:188: void OnDevToolsStartEventsRecording(int32 route_id, bool* succeeded); What's this? Prefer not to include non-related changes. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:136: bool GpuJpegDecodeAccelerator::Initialize() { DCHECK this runs on child_message_loop_. Same for others. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:150: NOTIMPLEMENTED() << "HW JPEG decode acceleration not available."; This shouldn't be an error. DLOG or DVLOG https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:181: LOG(ERROR) << __func__; Add DCHECK to make sure this runs on IO thread. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:190: if (child_message_loop_->BelongsToCurrentThread()) { remove because this only runs on IO thread. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:209: if (!output_shm->Map(params.output_buffer_size)) { Can we do Map in VaapiJDA::DecodeTask so this won't slow down IO thread? https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:81: // The message filter to run VDA::Decode on IO thread if VDA supports it. update the comment https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:95: LOG(ERROR) << "Failed initializing VAAPI"; DLOG https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:273: DCHECK_EQ(message_loop_, base::MessageLoop::current()); Decode runs on IO thread. Right? DCHECK(io_message_loop_proxy_->BelongsToCurrentThread()); Move MapAndQueueNewInputBuffer to DecodeTask. We cannot slow down IO thread. https://codereview.chromium.org/1016773002/diff/20001/content/content_common.... File content/content_common.gypi (right): https://codereview.chromium.org/1016773002/diff/20001/content/content_common.... content/content_common.gypi:880: 'common/gpu/media/vaapi_jpeg_decode_accelerator.cc', Also add this content/common/BUILD.gn https://codereview.chromium.org/1016773002/diff/20001/media/video/capture/lin... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/1016773002/diff/20001/media/video/capture/lin... media/video/capture/linux/video_capture_device_linux.cc:367: if (!client_->InitializeJpegDecoder()) { How about calling this when |last_captured_pixel_format_| is first set to PIXEL_FORMAT_MJPEG in video_capture_device_client.cc? VideoCaptureDeviceClient should be the one that decides to use jpeg decoder. If Android wants to support jpeg decoder, they shouldn't need to modify video_capture_device_android.cc.
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Please clarify to which platforms/hardware is this CL applicable: from the code, I only see VCDLinux affected but the new code is generic. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.cc:172: int buffer_id) { For another purpose I'm about to start adding a VideoCaptureDevice::Client::GetHandle(), which would be a shared memory fd or a dma-buf fd depending on the platform capabilities etc. Depending on the platform, the BufferPool Buffers might not be backed by SharedMemory. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:65: if (jpeg_decoder_.get()) If |jpeg_decoder_| is created from the VCD, then it belongs to either Capture Thread or some other thread (in Linux/CrOS case, "V4L2 thread"). It cannot be destroyed here, which is IO Thread. You might need to go through some extra hoops and hold a ref to the actual capture thread. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:69: bool VideoCaptureDeviceClient::InitializeJpegDecoder() { This method looks convoluted and exposes too much info to VCD, plus creates a lot of state on VCDClient, which is supposed to be a shallow, near-stateless translation rig. Propose adding a parameter to the constructor, where the result of host->CreateJpegDecoder(); is plugged in at ctor time in VCManager. That way this decoder can be destructed properly on ~VideoCaptureDeviceClient() and on IO thread. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:70: LOG(ERROR) << __func__; Suggest s/LOG(ERROR)/DVLOG(1)/ https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:71: GpuChannelHost* host = GpuChannelHost* const https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:74: GpuJpegDecodeAcceleratorHost* decoder = host->CreateJpegDecoder(); * const https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:184: There's a lot of extra code to just maintain the illusion of synchronicity for the VCD. I particularly dislike having to Wait() on a lock (l.214). Since anyway you're already dealing with a pool Buffer, what you could do is: - allocate one Buffer here, with the exact size to hold the JPEG frame, and VideoFrame::JPEG type. - copy |data| inside it (using |out_buffer.data()|), - PostTask() out_buffer to the JPEG decoder on IO thread - return to VCD. After decode, on VideoFrameReady(), you use WrapExternalPackedMemory() to make a VideoFrame. The JPEG Buffer goes out of scope and is recycled. You might need _another_ Buffer, with I420 format + size. Since JPEG and I420 have quite different sizes, the recycling won't be optimal. You might need two pools.
> > > https://codereview.chromium.org/1016773002/diff/20001/ > content/browser/renderer_host/media/video_capture_device_ > client.cc#newcode184 > content/browser/renderer_host/media/video_capture_device_client.cc:184: > There's a lot of extra code to just maintain the illusion > of synchronicity for the VCD. I particularly dislike having > to Wait() on a lock (l.214). > > Since anyway you're already dealing with a pool Buffer, > what you could do is: > > - allocate one Buffer here, with the exact size to hold the JPEG > frame, and VideoFrame::JPEG type. > - copy |data| inside it (using |out_buffer.data()|), > - PostTask() out_buffer to the JPEG decoder on IO thread > - return to VCD. > > After decode, on VideoFrameReady(), you use > WrapExternalPackedMemory() to make a VideoFrame. > The JPEG Buffer goes out of scope and is recycled. > You might need _another_ Buffer, with I420 format + size. > > Since JPEG and I420 have quite different sizes, the recycling > won't be optimal. You might need two pools. > > Nice suggestions! I will make it async. BTW, I feel one buffer for jpeg decode is enough, no need to prepare a pool. (on link, software decode latency is about 6.8ms, vaapi decode latency is about 3.5ms) I haven't finished the modification yet, will upload tomorrow. -- kcwu To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I fixed all thread issues. But I haven't addressed all comments yet. To mcasas, yes the code is generic. Now it only works for Intel platform on chromeos (VAAPI). exynos platform (V4L2) is planned. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.cc:172: int buffer_id) { On 2015/03/23 06:30:14, wuchengli wrote: > Need to acquire |lock_|. > > Or how about merging this with GetBufferInfo and store the handle in > media::VideoCaptureDevice::Client::Buffer? Done. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.cc:172: int buffer_id) { On 2015/03/23 19:29:50, mcasas wrote: > For another purpose I'm about to start adding a > VideoCaptureDevice::Client::GetHandle(), which would > be a shared memory fd or a dma-buf fd depending on > the platform capabilities etc. > > Depending on the platform, the BufferPool Buffers might > not be backed by SharedMemory. Did you mean VideoCaptureDevice::Client::Buffer::GetHandle() ? Now I'm adding VideoCaptureDevice::Client::Buffer::handle(), which returns base::SharedMemoryHandle, based on wuchengli's suggestion. What should I do to fit your plan? https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:65: if (jpeg_decoder_.get()) On 2015/03/23 19:29:51, mcasas wrote: > If |jpeg_decoder_| is created from the VCD, then it > belongs to either Capture Thread or some other thread > (in Linux/CrOS case, "V4L2 thread"). It cannot be > destroyed here, which is IO Thread. You might need to > go through some extra hoops and hold a ref to the > actual capture thread. jpeg_decoder_ is scoped_ptr(with DefaultDeleter), so no need to call Destroy explicitly here. These two lines are removed. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:69: bool VideoCaptureDeviceClient::InitializeJpegDecoder() { On 2015/03/23 19:29:51, mcasas wrote: > This method looks convoluted and exposes too much > info to VCD, plus creates a lot of state on VCDClient, > which is supposed to be a shallow, near-stateless > translation rig. > > Propose adding a parameter to the constructor, > where the result of host->CreateJpegDecoder(); > is plugged in at ctor time in VCManager. That > way this decoder can be destructed properly > on ~VideoCaptureDeviceClient() and on IO thread. Using MJPEG or YUV formats is determined in VideoCaptureDeviceLinux::V4L2CaptureDelegate::AllocateAndStart This information is not yet available at ctor time. I modified GpuJpegDecodeAcceleratorHost, make it work on device thread. And InitializeJpegDecoder() is called inside OnIncomingCapturedData() https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:85: int32_t bitstream_buffer_id) { On 2015/03/23 06:30:14, wuchengli wrote: > Add DCHECK to make sure this runs on IO thread. Then you can call > DoIncomingCapturedVideoFrameOnIOThread directly. DCHECK added. But still PostTask because I want to release lock earlier. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:86: auto map_it = jpeg_decode_task_map_.find(bitstream_buffer_id); On 2015/03/23 06:30:14, wuchengli wrote: > Like you said, since you are blocking the v4l2 thread, you don't need a map. > Let's assume the decode latency is low and we can block the v4l2 thread. This is refactored to async code. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:184: On 2015/03/23 19:29:50, mcasas wrote: > There's a lot of extra code to just maintain the illusion > of synchronicity for the VCD. I particularly dislike having > to Wait() on a lock (l.214). > > Since anyway you're already dealing with a pool Buffer, > what you could do is: > > - allocate one Buffer here, with the exact size to hold the JPEG > frame, and VideoFrame::JPEG type. > - copy |data| inside it (using |out_buffer.data()|), > - PostTask() out_buffer to the JPEG decoder on IO thread > - return to VCD. > > After decode, on VideoFrameReady(), you use > WrapExternalPackedMemory() to make a VideoFrame. > The JPEG Buffer goes out of scope and is recycled. > You might need _another_ Buffer, with I420 format + size. > > Since JPEG and I420 have quite different sizes, the recycling > won't be optimal. You might need two pools. > > Done. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:196: base::Closure()); On 2015/03/23 06:30:14, wuchengli wrote: > Set framerate like line 387. > frame->metadata()->SetDouble(media::VideoFrameMetadata::FRAME_RATE, Done. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:203: jpeg_decode_event_ = new base::WaitableEvent(false, false); On 2015/03/23 06:30:14, wuchengli wrote: > Use scoped_ptr Code removed. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:209: &VideoCaptureDeviceClient::DecodeJpegOnIOThread, On 2015/03/23 06:30:14, wuchengli wrote: > Post to JpegDecodeAccelerator::Decode directly. Modified to call Decode() directly. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:80: BrowserThread::PostTask( On 2015/03/23 06:30:14, wuchengli wrote: > Looks like GJDAH is accessed entirely on IO thread. Right? So you should call > GJDAH::Initialize also on IO thread. Then you don't need to PostTask here. I modified it to AddRoute from caller instead of here. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:805: (void)decoder; // it will delete itself?? On 2015/03/23 06:30:14, wuchengli wrote: > No. I think you need some kind of DestructionObserver. Done. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:190: if (child_message_loop_->BelongsToCurrentThread()) { On 2015/03/23 06:30:15, wuchengli wrote: > remove because this only runs on IO thread. Done. https://codereview.chromium.org/1016773002/diff/20001/media/video/capture/lin... File media/video/capture/linux/video_capture_device_linux.cc (right): https://codereview.chromium.org/1016773002/diff/20001/media/video/capture/lin... media/video/capture/linux/video_capture_device_linux.cc:367: if (!client_->InitializeJpegDecoder()) { On 2015/03/23 06:30:15, wuchengli wrote: > How about calling this when |last_captured_pixel_format_| is first set to > PIXEL_FORMAT_MJPEG in video_capture_device_client.cc? VideoCaptureDeviceClient > should be the one that decides to use jpeg decoder. If Android wants to support > jpeg decoder, they shouldn't need to modify video_capture_device_android.cc. Done.
I'll finish the rest of the review soon. https://codereview.chromium.org/1016773002/diff/40001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1016773002/diff/40001/build/common.gypi#newco... build/common.gypi:3631: #'_DEBUG', revert https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:299: DVLOG(3) << __func__; remove https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:133: OnIncomingCapturedData2(jpeg_decode_task_.captured_data); Add a LOG(ERROR) and call VideoCaptureDeviceClient::OnError should be enough. We want to know why JPEG HW encoder fails. In the future if this stabilizes, we can set |external_jpeg_decoder_| to null and the next frame will fall back to software. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:239: // TODO fallback to software decode Add a LOG(ERROR) here. If we have GpuJpegDecoder external_jpeg_decoder_, we can set external_jpeg_decoder_ to null and it will fall back to software. |jpeg_failed_| won't be needed. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:254: base::AutoLock lock(jpeg_decode_task_.lock_); Try to hold the lock as minimum as possible so IO thread can be run faster. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:255: if (jpeg_decode_task_.decoding) { Checking out_frame == nullptr should be enough. Remove |decoding|. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:261: // TODO(kcwu): fallback to software decode Add a LOG(ERROR), call VideoCaptureDeviceClient::OnError, and remove TODO. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:272: void VideoCaptureDeviceClient::OnIncomingCapturedData2( The name OnIncomingCapturedData should be used for the original code. How about moving all the code about Jpeg to a new file content/browser/renderer_host/gpu_jpeg_decoder.cc (GpuJpegDecoder)? You can initialize a |external_jpeg_decoder_| in the constructor. Then you can call external_jpeg_decoder_->Decode in OnIncomingCapturedData. Add a bool GpuJpegDecoder::Supported, which returns true only for OS_CHROMEOS and ARCH_CPU_X86_FAMILY. So we don't add latency for those platforms that do not support JPEG acceleration. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:290: // XXX kcwu: why crop?? remove https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:326: LOG(ERROR) << "drop fps = " << drop_count / (base::Time::Now() - start_time).InSecondsF(); remove https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:24: struct CapturedData { Can this be moved to video_capture_device_client.cc? Same for JpegDecodeTask. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:32: // TODO(kcwu): convert to class Remove. Make it a class. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:33: struct JpegDecodeTask { s/JpegDecodeTask/JpegDecodeData/ is better. A task sounds like it can be run. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:42: scoped_ptr<base::SharedMemory> in_shared_memory; Need variable comments for JpegDecodeTask and CapturedData. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:104: bool jpeg_failed_; Remove this. Let's not do fallback now. If a platform supports Jpeg HW decode, it shouldn't fail. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:106: JpegDecodeTask jpeg_decode_task_; Put all jpeg variables together. Move these beside |next_bitstream_buffer_id_|. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:113: int32 next_bitstream_buffer_id_; Better to have jpeg in the name. s/next_bitstream_buffer_id_/next_jpeg_buffer_id_/? https://codereview.chromium.org/1016773002/diff/40001/content/common/gpu/clie... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1016773002/diff/40001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.cc:311: channel_filter_.get(), route_id, decoder->AsWeakPtr(), 80 char align?
https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:71: GpuChannelHost* host = On 2015/03/23 19:29:51, mcasas wrote: > GpuChannelHost* const Done. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:74: GpuJpegDecodeAcceleratorHost* decoder = host->CreateJpegDecoder(); On 2015/03/23 19:29:50, mcasas wrote: > * const Done. https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:160: OnIncomingCapturedData2(captured_data); On 2015/03/23 06:30:14, wuchengli wrote: > Please measure the decode time of both cases and put it in the change > description. The time between here and before calling > DoIncomingCapturedVideoFrameOnIOThread. Acknowledged. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_channel_host.h (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.h:162: GpuJpegDecodeAcceleratorHost* CreateJpegDecoder(); On 2015/03/23 06:30:14, wuchengli wrote: > Return scoped_ptr<media::JpegDecodeAccelerator> to be consistent with > CreateVideoDecoder and CreateVideoEncoder. Move this after CreateVideoEncoder. Done. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.h (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.h:188: void OnDevToolsStartEventsRecording(int32 route_id, bool* succeeded); On 2015/03/23 06:30:14, wuchengli wrote: > What's this? Prefer not to include non-related changes. Done. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:150: NOTIMPLEMENTED() << "HW JPEG decode acceleration not available."; On 2015/03/23 06:30:14, wuchengli wrote: > This shouldn't be an error. DLOG or DVLOG Done. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:181: LOG(ERROR) << __func__; On 2015/03/23 06:30:15, wuchengli wrote: > Add DCHECK to make sure this runs on IO thread. Done. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:209: if (!output_shm->Map(params.output_buffer_size)) { On 2015/03/23 06:30:15, wuchengli wrote: > Can we do Map in VaapiJDA::DecodeTask so this won't slow down IO thread? VaapiJDA::Decode expect VideoFrame, so we have to map here. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:81: // The message filter to run VDA::Decode on IO thread if VDA supports it. On 2015/03/23 06:30:15, wuchengli wrote: > update the comment Done. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:95: LOG(ERROR) << "Failed initializing VAAPI"; On 2015/03/23 06:30:15, wuchengli wrote: > DLOG Done. https://codereview.chromium.org/1016773002/diff/20001/content/content_common.... File content/content_common.gypi (right): https://codereview.chromium.org/1016773002/diff/20001/content/content_common.... content/content_common.gypi:880: 'common/gpu/media/vaapi_jpeg_decode_accelerator.cc', On 2015/03/23 06:30:15, wuchengli wrote: > Also add this content/common/BUILD.gn Done. https://codereview.chromium.org/1016773002/diff/40001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1016773002/diff/40001/build/common.gypi#newco... build/common.gypi:3631: #'_DEBUG', On 2015/04/14 09:41:43, wuchengli wrote: > revert Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:299: DVLOG(3) << __func__; On 2015/04/14 09:41:43, wuchengli wrote: > remove Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:254: base::AutoLock lock(jpeg_decode_task_.lock_); On 2015/04/14 09:41:44, wuchengli wrote: > Try to hold the lock as minimum as possible so IO thread can be run faster. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:290: // XXX kcwu: why crop?? On 2015/04/14 09:41:44, wuchengli wrote: > remove Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:326: LOG(ERROR) << "drop fps = " << drop_count / (base::Time::Now() - start_time).InSecondsF(); On 2015/04/14 09:41:44, wuchengli wrote: > remove Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:24: struct CapturedData { On 2015/04/14 09:41:44, wuchengli wrote: > Can this be moved to video_capture_device_client.cc? Same for JpegDecodeTask. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:32: // TODO(kcwu): convert to class On 2015/04/14 09:41:44, wuchengli wrote: > Remove. Make it a class. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:33: struct JpegDecodeTask { On 2015/04/14 09:41:44, wuchengli wrote: > s/JpegDecodeTask/JpegDecodeData/ is better. A task sounds like it can be run. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:106: JpegDecodeTask jpeg_decode_task_; On 2015/04/14 09:41:44, wuchengli wrote: > Put all jpeg variables together. Move these beside |next_bitstream_buffer_id_|. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:113: int32 next_bitstream_buffer_id_; On 2015/04/14 09:41:44, wuchengli wrote: > Better to have jpeg in the name. > s/next_bitstream_buffer_id_/next_jpeg_buffer_id_/? Done. https://codereview.chromium.org/1016773002/diff/40001/content/common/gpu/clie... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1016773002/diff/40001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.cc:311: channel_filter_.get(), route_id, decoder->AsWeakPtr(), On 2015/04/14 09:41:44, wuchengli wrote: > 80 char align? Done.
https://codereview.chromium.org/1016773002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:216: // Verify parameters from GPU thread. s/thread/process/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.cc:260: scoped_ptr<media::JpegDecodeAccelerator> GpuChannelHost::CreateJpegDecoder() { It looks OK to run CreateJpegDecoder on non-main thread. Let's confirm with piman when he reviews this. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.cc:267: scoped_refptr<base::MessageLoopProxy> io_loop = factory_->GetIOLoopProxy(); Add a comment to explain we want to run VideoFrameReady on IO thread. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:10: #include "base/synchronization/waitable_event.h" is this required? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:13: #include "content/common/view_messages.h" Is this required? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:38: DCHECK(CalledOnValidThread()); OnMessageReceived is called on IO thread, which is different from the device thread that runs Initialize. Is there a way to DCHECK on IO thread? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:41: IPC_MESSAGE_HANDLER(AcceleratedJpegDecoderHostMsg_VideoFrameReady, Indentation? Run git cl format. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:68: // this cannot on main thread or IO thread s/cannot on/cannon be/. Explain why this cannot be on main or IO thread. If this cannot be on main thread, we should document in gpu_jpeg_decode_accelerator_host.h and say this class cannot be used on main thread. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:91: DLOG(ERROR) << "Failed to duplicate buffer handler of BitstreamBuffer"; Use LOG(ERROR) because this shouldn't happen. s/handler/handle/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:98: << "Decode(): cannot output to frame not backed by shared memory"; Use LOG(ERROR) because this shouldn't happen. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:99: PostNotifyError(bitstream_buffer.id(), PLATFORM_FAILURE); Maybe INVALID_ARGUMENT? Why this is a PLATFORM_FAILURE? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:106: DLOG(ERROR) << "Decode(): failed to duplicate buffer handle of VideoFrame"; s/DLOG/LOG/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:135: DVLOG(2) << "PostNotifyDecodeResult(): error=" << error; s/PostNotifyDecodeResult/__func__? Also log |bitstream_buffer_id|. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:136: // Post the error notification back to this thread, to avoid re-entrancy. I don't understand. Can you explain? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:144: DVLOG(3) << __func__; Be consistent with the order DVLOG and DCHECK(CalledOnValidThread()). Choose one to be the first and make it the same in this file. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:154: DCHECK(CalledOnValidThread()); Change DCHECK to IO thread. Please enable the DCHECKs of your CL and make sure it doesn't hit them. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:162: DCHECK(CalledOnValidThread()); Change DCHECK to IO thread. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:170: std::swap(client, client_); Why do we need to swap? To avoid client->NotifyError being called again when GpuJpegDecodeAcceleratorHost::OnNotifyError is called more than once? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.h:237: scoped_ptr<content::GpuJpegDecodeAccelerator> jpeg_decoder_; Need a way to support multiple jpeg decoders here. Does ScopedPtrHashMap work? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:783: // Send destroy request to the decoder. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:792: Add a simple comment. // Report error condition. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:40: } // namespace base add a blank line https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:62: DVLOG(3) << __func__; The log in OnDecode should be enough. This can be removed. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:150: // This is called from JDA's decode thread. This should be called from IO thread to reduce the latency. DCHECK io thread. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:158: // SharedMemory as VideoFrame I don't understand. What do you mean? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:159: DVLOG(3) << "DecodeFinished"; __func__ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:169: DLOG(ERROR) << "BitstreamBuffer id " << params.input_buffer_id s/DLOG/LOG/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:183: DLOG(ERROR) << "Could not map output shared memory for input buffer id " s/DLOG/LOG/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:196: params.coded_size, We don't really care about the visible size here. Right? This is not passed back or anything. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:205: DLOG(ERROR) << "Could not create VideoFrame for input buffer id " s/DLOG/LOG/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:215: void GpuJpegDecodeAccelerator::OnDestroy() { DCHECK main thread https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:226: void GpuJpegDecodeAccelerator::Destroy() { Do we need this method? Can this be combined with OnDestroy? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:233: if (filter_.get()) { |filter_| should always exist after Initialize. This can be removed. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:238: channel_->RemoveRoute(host_route_id_); This can be called in ReleaseJpegDecoder. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:246: if (filter_.get() && io_message_loop_->BelongsToCurrentThread()) Remove filter_.get() check. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:29: // Each of the arguments to the constructor must outlive this object. Just say "|channel| must outlive this object". |host_route_id| doesn't apply. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:62: // GpuChannels destroy all the GpuCommandBufferStubs that they own when they update the comment https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:26: #define RETURN_AND_NOTIFY_ON_FAILURE(result, log, buffer_id, error_code, ret) \ This is only used once. Just use the following code directly. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:51: // Post Cleanup() as a task so we don't recursively acquire lock_. I don't understand why recursively acquire lock_ is possible. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:66: decoder_thread_("VaapiDecoderThread"), s/VaapiDecoderThread/VaapiJpegDecoderThread/ to distinguish from vaapi vda. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:129: memcpy(frame_mem, mem, frame_buffer_size); Can we import frame_mem to vaapi so we can get rid of this memcpy? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:134: client_->VideoFrameReady(input_id); This should be posted to io thread to reduce latency. See how v4l2_video_decode_accelerator.cc posts to io thread. V4l2VideoDecodeAccelerator has a |io_client| parameter and GpuVideoDecodeAccelerator has |weak_factory_for_io_|. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:151: base::AutoLock auto_lock(lock_); This can be moved to line 159. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:174: state_ = kDecoding; Why do we need kDecoding state? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:177: curr_input_buffer_ = input_buffers_.front(); curr_input_buffer_ doesn't have to be a class variable. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:183: base::AutoUnlock auto_unlock(lock_); I think adding a code block for line 167-178 and line 222 is clearer. |auto_unlock| block here can be removed. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:189: curr_input_buffer_->size, &parse_result)) { add a DLOG(ERROR) https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:198: vaapi_wrapper_->DestroySurfaces(); Do we need this? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:205: DLOG(ERROR) << "Decode failed"; Can this happen? s/DLOG/LOG/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:213: DLOG(ERROR) << "Output failed"; Can this happen? s/DLOG/LOG/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:229: // We got a new input buffer from the client, map it and queue for later use. s/client, map/client. Map/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:232: base::AutoLock auto_lock(lock_); The lock is not required here? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:242: if (state_ == kUninitialized || state_ == kDestroying) Why do we need kDestroying? https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:245: DVLOG(1) << "Destroying VAJDA"; VAJDA is a uncommon abbreviation. s/VAJDA/VaapiJpegDecodeAccelerator/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:254: input_ready_.Signal(); Not used. Also remove the comments. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:64: // Continue decoding given input buffers and sleep waiting for input/output There is no sleep waiting. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:65: // as needed. Will exit if a new set of surfaces or reset/flush/destroy Update the comment. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:72: // TODO(kcwu) fix comment Remove TODO and update the comment. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:93: // Protects input buffer and surface queues and state_. Please list all the exact variable names instead of description. For example, which surface queues? s/input buffer/input_buffers_|. s/state_/|state_|/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:105: remove blank line https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:112: // Signalled when input buffers are queued onto the input_buffers_ queue. s/input_buffers_/|input_buffers_|/. Same for others in this file. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:113: base::ConditionVariable input_ready_; not used. remove https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:121: // To expose client callbacks from VideoDecodeAccelerator. s/VideoDecodeAccelerator/JpegDecodeAccelerator/ https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:135: VASurface::ReleaseCB va_surface_release_cb_; not used https://codereview.chromium.org/1016773002/diff/60001/media/video/jpeg_decode... File media/video/jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/60001/media/video/jpeg_decode... media/video/jpeg_decode_accelerator.h:20: // keeps the ownership of them. Initialize and Destroy must be called on the same thread. Right? Document it if that's the case.
Hi, @mcasas, could you please take another look? Now most JPEG related stuff are moved out from video_capture_device_client.cc to gpu_jpeg_decode_accelerator_adapter.cc. Hope this make things more clear. Now OnIncomingCapturedData become async for HW accelerated case as your suggestion. For Tracker::handle(), I plan to let non-shared-memory backed tracker return NULLHandle. Do you have other suggestions? https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:70: LOG(ERROR) << __func__; On 2015/03/23 19:29:51, mcasas wrote: > Suggest s/LOG(ERROR)/DVLOG(1)/ Done. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:136: bool GpuJpegDecodeAccelerator::Initialize() { On 2015/03/23 06:30:15, wuchengli wrote: > DCHECK this runs on child_message_loop_. Same for others. Done. https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/20001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:273: DCHECK_EQ(message_loop_, base::MessageLoop::current()); On 2015/03/23 06:30:15, wuchengli wrote: > Decode runs on IO thread. Right? > DCHECK(io_message_loop_proxy_->BelongsToCurrentThread()); > > Move MapAndQueueNewInputBuffer to DecodeTask. We cannot slow down IO thread. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:133: OnIncomingCapturedData2(jpeg_decode_task_.captured_data); On 2015/04/14 09:41:44, wuchengli wrote: > Add a LOG(ERROR) and call VideoCaptureDeviceClient::OnError should be enough. We > want to know why JPEG HW encoder fails. In the future if this stabilizes, we can > set |external_jpeg_decoder_| to null and the next frame will fall back to > software. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:239: // TODO fallback to software decode On 2015/04/14 09:41:44, wuchengli wrote: > Add a LOG(ERROR) here. If we have GpuJpegDecoder external_jpeg_decoder_, we can > set external_jpeg_decoder_ to null and it will fall back to software. > |jpeg_failed_| won't be needed. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:255: if (jpeg_decode_task_.decoding) { On 2015/04/14 09:41:44, wuchengli wrote: > Checking out_frame == nullptr should be enough. Remove |decoding|. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:261: // TODO(kcwu): fallback to software decode On 2015/04/14 09:41:44, wuchengli wrote: > Add a LOG(ERROR), call VideoCaptureDeviceClient::OnError, and remove TODO. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:272: void VideoCaptureDeviceClient::OnIncomingCapturedData2( On 2015/04/14 09:41:44, wuchengli wrote: > The name OnIncomingCapturedData should be used for the original code. How about > moving all the code about Jpeg to a new file > content/browser/renderer_host/gpu_jpeg_decoder.cc (GpuJpegDecoder)? You can > initialize a |external_jpeg_decoder_| in the constructor. Then you can call > external_jpeg_decoder_->Decode in OnIncomingCapturedData. > > Add a bool GpuJpegDecoder::Supported, which returns true only for OS_CHROMEOS > and ARCH_CPU_X86_FAMILY. So we don't add latency for those platforms that do not > support JPEG acceleration. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:42: scoped_ptr<base::SharedMemory> in_shared_memory; On 2015/04/14 09:41:44, wuchengli wrote: > Need variable comments for JpegDecodeTask and CapturedData. Done. https://codereview.chromium.org/1016773002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:104: bool jpeg_failed_; On 2015/04/14 09:41:44, wuchengli wrote: > Remove this. Let's not do fallback now. If a platform supports Jpeg HW decode, > it shouldn't fail. Removed. But still do fallback. Some kind of fallback is necessary -- consider if an user pluged in an external camera and the hardware cannot handle the JPEG from the camera. https://codereview.chromium.org/1016773002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:216: // Verify parameters from GPU thread. On 2015/04/15 07:11:57, wuchengli wrote: > s/thread/process/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.cc:260: scoped_ptr<media::JpegDecodeAccelerator> GpuChannelHost::CreateJpegDecoder() { On 2015/04/15 07:11:57, wuchengli wrote: > It looks OK to run CreateJpegDecoder on non-main thread. Let's confirm with > piman when he reviews this. Acknowledged. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.cc:267: scoped_refptr<base::MessageLoopProxy> io_loop = factory_->GetIOLoopProxy(); On 2015/04/15 07:11:57, wuchengli wrote: > Add a comment to explain we want to run VideoFrameReady on IO thread. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:10: #include "base/synchronization/waitable_event.h" On 2015/04/15 07:11:57, wuchengli wrote: > is this required? Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:13: #include "content/common/view_messages.h" On 2015/04/15 07:11:57, wuchengli wrote: > Is this required? Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:41: IPC_MESSAGE_HANDLER(AcceleratedJpegDecoderHostMsg_VideoFrameReady, On 2015/04/15 07:11:57, wuchengli wrote: > Indentation? Run git cl format. Fixed. The indentation was broken due to git cl format. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:91: DLOG(ERROR) << "Failed to duplicate buffer handler of BitstreamBuffer"; On 2015/04/15 07:11:57, wuchengli wrote: > Use LOG(ERROR) because this shouldn't happen. s/handler/handle/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:98: << "Decode(): cannot output to frame not backed by shared memory"; On 2015/04/15 07:11:57, wuchengli wrote: > Use LOG(ERROR) because this shouldn't happen. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:99: PostNotifyError(bitstream_buffer.id(), PLATFORM_FAILURE); On 2015/04/15 07:11:57, wuchengli wrote: > Maybe INVALID_ARGUMENT? Why this is a PLATFORM_FAILURE? Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:106: DLOG(ERROR) << "Decode(): failed to duplicate buffer handle of VideoFrame"; On 2015/04/15 07:11:57, wuchengli wrote: > s/DLOG/LOG/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:135: DVLOG(2) << "PostNotifyDecodeResult(): error=" << error; On 2015/04/15 07:11:57, wuchengli wrote: > s/PostNotifyDecodeResult/__func__? Also log |bitstream_buffer_id|. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:144: DVLOG(3) << __func__; On 2015/04/15 07:11:57, wuchengli wrote: > Be consistent with the order DVLOG and DCHECK(CalledOnValidThread()). Choose one > to be the first and make it the same in this file. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:783: On 2015/04/15 07:11:57, wuchengli wrote: > // Send destroy request to the decoder. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:792: On 2015/04/15 07:11:57, wuchengli wrote: > Add a simple comment. > // Report error condition. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:40: } // namespace base On 2015/04/15 07:11:58, wuchengli wrote: > add a blank line Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:62: DVLOG(3) << __func__; On 2015/04/15 07:11:58, wuchengli wrote: > The log in OnDecode should be enough. This can be removed. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:159: DVLOG(3) << "DecodeFinished"; On 2015/04/15 07:11:58, wuchengli wrote: > __func__ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:169: DLOG(ERROR) << "BitstreamBuffer id " << params.input_buffer_id On 2015/04/15 07:11:58, wuchengli wrote: > s/DLOG/LOG/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:183: DLOG(ERROR) << "Could not map output shared memory for input buffer id " On 2015/04/15 07:11:58, wuchengli wrote: > s/DLOG/LOG/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:196: params.coded_size, On 2015/04/15 07:11:58, wuchengli wrote: > We don't really care about the visible size here. Right? This is not passed back > or anything. Yes https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:205: DLOG(ERROR) << "Could not create VideoFrame for input buffer id " On 2015/04/15 07:11:58, wuchengli wrote: > s/DLOG/LOG/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:29: // Each of the arguments to the constructor must outlive this object. On 2015/04/15 07:11:58, wuchengli wrote: > Just say "|channel| must outlive this object". |host_route_id| doesn't apply. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:26: #define RETURN_AND_NOTIFY_ON_FAILURE(result, log, buffer_id, error_code, ret) \ On 2015/04/15 07:11:59, wuchengli wrote: > This is only used once. Just use the following code directly. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:151: base::AutoLock auto_lock(lock_); On 2015/04/15 07:11:59, wuchengli wrote: > This can be moved to line 159. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:177: curr_input_buffer_ = input_buffers_.front(); On 2015/04/15 07:11:59, wuchengli wrote: > curr_input_buffer_ doesn't have to be a class variable. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:198: vaapi_wrapper_->DestroySurfaces(); On 2015/04/15 07:11:59, wuchengli wrote: > Do we need this? Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:205: DLOG(ERROR) << "Decode failed"; On 2015/04/15 07:11:59, wuchengli wrote: > Can this happen? s/DLOG/LOG/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:213: DLOG(ERROR) << "Output failed"; On 2015/04/15 07:11:59, wuchengli wrote: > Can this happen? s/DLOG/LOG/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:229: // We got a new input buffer from the client, map it and queue for later use. On 2015/04/15 07:11:59, wuchengli wrote: > s/client, map/client. Map/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:232: base::AutoLock auto_lock(lock_); On 2015/04/15 07:11:59, wuchengli wrote: > The lock is not required here? Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:245: DVLOG(1) << "Destroying VAJDA"; On 2015/04/15 07:11:58, wuchengli wrote: > VAJDA is a uncommon abbreviation. s/VAJDA/VaapiJpegDecodeAccelerator/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:254: input_ready_.Signal(); On 2015/04/15 07:11:58, wuchengli wrote: > Not used. Also remove the comments. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:64: // Continue decoding given input buffers and sleep waiting for input/output On 2015/04/15 07:11:59, wuchengli wrote: > There is no sleep waiting. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:65: // as needed. Will exit if a new set of surfaces or reset/flush/destroy On 2015/04/15 07:11:59, wuchengli wrote: > Update the comment. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:72: // TODO(kcwu) fix comment On 2015/04/15 07:11:59, wuchengli wrote: > Remove TODO and update the comment. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:105: On 2015/04/15 07:11:59, wuchengli wrote: > remove blank line Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:112: // Signalled when input buffers are queued onto the input_buffers_ queue. On 2015/04/15 07:11:59, wuchengli wrote: > s/input_buffers_/|input_buffers_|/. Same for others in this file. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:113: base::ConditionVariable input_ready_; On 2015/04/15 07:11:59, wuchengli wrote: > not used. remove Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:121: // To expose client callbacks from VideoDecodeAccelerator. On 2015/04/15 07:11:59, wuchengli wrote: > s/VideoDecodeAccelerator/JpegDecodeAccelerator/ Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:135: VASurface::ReleaseCB va_surface_release_cb_; On 2015/04/15 07:11:59, wuchengli wrote: > not used Done.
Did a partial review. Please revisit your comments, I find some of them hard to grok. I concentrated on the classes I know. You might like to take a look at the changes in http://crrev.com/1064963002 that are partially overlapping with these in video_capture_buffer_pool, albeit compatible. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:36: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO)); Please rephrase comment, I find it hard to read, and change DCHECK to DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)); https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:51: media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) { Prefer early termination; by the way shouldn't we return |false| if |bitstream_buffer_id| is kInvalidBitstreamBufferId ? Like: if (bitstream_buffer_id == media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) return false; LOG_IF(ERROR, !IsDecoding()) << "Got decode response while not decoding"; if (!IsDecoding()) return false; ... https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:74: DVLOG(3) << __func__; pro tip: In Linux you can use __PRETTY_FUNCTION__ ISO __func__ or __FUNCTION__. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:102: captured_data_.timestamp); If you discard |captured_data_|, you can use |out_frame_.timestamp()| https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:158: media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) { This condition is already covered inside IsExpectedDecodeResponse() I believe. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:185: gfx::Size dimensions = frame_format.frame_size; const? https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:186: scoped_refptr<media::VideoCaptureDevice::Client::Buffer> out_buffer = Suggest to directly allocate onto |out_buffer_| and in l.232 https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:201: size_t in_buffer_size = captured_data.length; Why an extra alias? Just rename the input param. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:205: // Swap out to local variable in order to reduce lock time in IO thread. s/in/on/ https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:211: in_buffer_size > in_shared_memory->mapped_size()) { I suspect this allocate-as-you-go scheme might end up in a series of initial allocations until we settle in a large enough size to hold incoming frames. Alternatively, consider pre allocating enough size to hold the largest possible JPEG frame, which should be the size of an uncompressed I420 frame, right? Then you should be able to recycle it instead of swapping it in and out (which might degrade performance as we discovered elsewhere). https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:220: if (!in_shared_memory->Map(in_buffer_size)) { Merge this with the previous error condition in l.213. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:260: decoder_->Decode(in_buffer_, out_frame_); IIUC, only |in_buffer_->id()| is really needed. Please consider only keeping id as a member variable for IsExpectedDecodeResponse(). https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:20: // Adapter to GpuJpegDecodeAccelerator for VideoDeviceDevice::Client. It takes s/VideoDeviceDevice/VideoCaptureDevice/ https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:21: // care GpuJpegDecodeAccelerator creation, shared memory, and thread issues. "care of" "threading issues" https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:22: // If decoding failed, it will fallback to software decode for current frame s/failed/fails/ https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:25: // This class is mainly accessed by device thread. The decoding responses Please also mention that it's created and destroyed on Capture Thread. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:65: // Fail state indicats to use software decode. See comment in s/indicats/indicates/ https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:87: void FallbackToSoftwareDecode(); I don't think this is really needed: If the current frame does not get decoded correctly, just discard it and mark the error condition. Moreover, that would allow you to not hold on to |captured_data_|, note that all VideoCaptureDevice assume that the Client is done with |data| when returning from OnIncomingCapturedXXX(), so |captured_data_.data| accessing should be avoided. In linux case in particular, this pointer is returned to the V4L2 driver. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:103: CapturedData captured_data_; See my comments above, I think we could get rid of this. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:92: external_jpeg_decoder_->DecodeCapturedData(data, length, frame_format, Add here DCHECK_EQ(frame_format.pixel_format, media::PIXEL_FORMAT_MJPEG); ? Remember that a client may change the passed format on a per frame basis.
Miguel, thanks for your review. Could you please take another look? Wu-cheng, I addressed all your comments except multiple jpeg decoder. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:38: DCHECK(CalledOnValidThread()); On 2015/04/15 07:11:57, wuchengli wrote: > OnMessageReceived is called on IO thread, which is different from the device > thread that runs Initialize. Is there a way to DCHECK on IO thread? Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:68: // this cannot on main thread or IO thread On 2015/04/15 07:11:57, wuchengli wrote: > s/cannot on/cannon be/. Explain why this cannot be on main or IO thread. If this > cannot be on main thread, we should document in > gpu_jpeg_decode_accelerator_host.h and say this class cannot be used on main > thread. Done. Recheck the code, it's okay on main thread. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:136: // Post the error notification back to this thread, to avoid re-entrancy. On 2015/04/15 07:11:57, wuchengli wrote: > I don't understand. Can you explain? This is copied from GVEAHost. I guess it try to avoid OnNotifyError handler doing something triggering error again. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:154: DCHECK(CalledOnValidThread()); On 2015/04/15 07:11:57, wuchengli wrote: > Change DCHECK to IO thread. Please enable the DCHECKs of your CL and make sure > it doesn't hit them. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:162: DCHECK(CalledOnValidThread()); On 2015/04/15 07:11:57, wuchengli wrote: > Change DCHECK to IO thread. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:170: std::swap(client, client_); On 2015/04/15 07:11:57, wuchengli wrote: > Why do we need to swap? To avoid client->NotifyError being called again when > GpuJpegDecodeAcceleratorHost::OnNotifyError is called more than once? Yes, I think so. This is copied from GVDAHost. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:150: // This is called from JDA's decode thread. On 2015/04/15 07:11:58, wuchengli wrote: > This should be called from IO thread to reduce the latency. DCHECK io thread. I don't think this will reduce latency. The flow is eventually passed from decode thread to IO thread. It doesn't matter that the caller of VideoFrameReady PostTask first or ChannelProxy Send() PostTask internally. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:158: // SharedMemory as VideoFrame On 2015/04/15 07:11:58, wuchengli wrote: > I don't understand. What do you mean? Done. Comment updated. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:215: void GpuJpegDecodeAccelerator::OnDestroy() { On 2015/04/15 07:11:58, wuchengli wrote: > DCHECK main thread Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:226: void GpuJpegDecodeAccelerator::Destroy() { On 2015/04/15 07:11:57, wuchengli wrote: > Do we need this method? Can this be combined with OnDestroy? There are two flows to destroy GJDA: 1. msg -> OnDestroy -> ... -> remove from gpu_channel 2. something wrong, GpuChannel want to destroy itself and also remove GJDA Destroy need to be a separate method for case 2 to call. I may revisit this when adding support for multiple jpeg decoders. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:233: if (filter_.get()) { On 2015/04/15 07:11:58, wuchengli wrote: > |filter_| should always exist after Initialize. This can be removed. This is necessary for safety if GpuJpegDecodeAccelerator created and destroyed without Initialize. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:238: channel_->RemoveRoute(host_route_id_); On 2015/04/15 07:11:58, wuchengli wrote: > This can be called in ReleaseJpegDecoder. AddRoute is in Initialize(). I'd like RemoveRoute is inside this class as well since they are paired. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:246: if (filter_.get() && io_message_loop_->BelongsToCurrentThread()) On 2015/04/15 07:11:58, wuchengli wrote: > Remove filter_.get() check. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:62: // GpuChannels destroy all the GpuCommandBufferStubs that they own when they On 2015/04/15 07:11:58, wuchengli wrote: > update the comment Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:51: // Post Cleanup() as a task so we don't recursively acquire lock_. On 2015/04/15 07:11:58, wuchengli wrote: > I don't understand why recursively acquire lock_ is possible. I don't, neither. I guess VAVDA's original Cleanup might trigger NotifyError. Remove this PostTask since we don't the issue here. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:66: decoder_thread_("VaapiDecoderThread"), On 2015/04/15 07:11:58, wuchengli wrote: > s/VaapiDecoderThread/VaapiJpegDecoderThread/ to distinguish from vaapi vda. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:129: memcpy(frame_mem, mem, frame_buffer_size); On 2015/04/15 07:11:59, wuchengli wrote: > Can we import frame_mem to vaapi so we can get rid of this memcpy? I don't know how. I will ask. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:134: client_->VideoFrameReady(input_id); On 2015/04/15 07:11:59, wuchengli wrote: > This should be posted to io thread to reduce latency. See how > v4l2_video_decode_accelerator.cc posts to io thread. V4l2VideoDecodeAccelerator > has a |io_client| parameter and GpuVideoDecodeAccelerator has > |weak_factory_for_io_|. I don't think this will reduce latency to post this to IO thread. The flow is eventually passed from decode thread to IO thread. It doesn't matter that PostTask here or ChannelProxy Send() PostTask internally. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:174: state_ = kDecoding; On 2015/04/15 07:11:59, wuchengli wrote: > Why do we need kDecoding state? Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:183: base::AutoUnlock auto_unlock(lock_); On 2015/04/15 07:11:58, wuchengli wrote: > I think adding a code block for line 167-178 and line 222 is clearer. > |auto_unlock| block here can be removed. Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:189: curr_input_buffer_->size, &parse_result)) { On 2015/04/15 07:11:59, wuchengli wrote: > add a DLOG(ERROR) Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:242: if (state_ == kUninitialized || state_ == kDestroying) On 2015/04/15 07:11:59, wuchengli wrote: > Why do we need kDestroying? Done. https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:93: // Protects input buffer and surface queues and state_. On 2015/04/15 07:11:59, wuchengli wrote: > Please list all the exact variable names instead of description. For example, > which surface queues? s/input buffer/input_buffers_|. s/state_/|state_|/ Done. https://codereview.chromium.org/1016773002/diff/60001/media/video/jpeg_decode... File media/video/jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/60001/media/video/jpeg_decode... media/video/jpeg_decode_accelerator.h:20: // keeps the ownership of them. On 2015/04/15 07:11:59, wuchengli wrote: > Initialize and Destroy must be called on the same thread. Right? Document it if > that's the case. Done. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:36: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO)); On 2015/04/16 23:55:13, mcasas wrote: > Please rephrase comment, I find it hard to read, and change DCHECK to > DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)); I thought again. Since IO thread is not possible blocking wait for capturing, the device thread is obviously not IO thread. No need to check (at least not here). So I removed this check. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:51: media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) { On 2015/04/16 23:55:13, mcasas wrote: > Prefer early termination; by the way shouldn't we return |false| if > |bitstream_buffer_id| is kInvalidBitstreamBufferId ? Like: > > if (bitstream_buffer_id == > media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) > return false; > LOG_IF(ERROR, !IsDecoding()) << "Got decode response while not decoding"; > if (!IsDecoding()) > return false; > ... kInvalidBitstreamBufferId is possible value for NotifyError. For example, GPU process crashed and the IPC channel disconnected. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:74: DVLOG(3) << __func__; On 2015/04/16 23:55:13, mcasas wrote: > pro tip: In Linux you can use __PRETTY_FUNCTION__ ISO __func__ or __FUNCTION__. Acknowledged. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:102: captured_data_.timestamp); On 2015/04/16 23:55:13, mcasas wrote: > If you discard |captured_data_|, you can use |out_frame_.timestamp()| out_frame_.timestamp() was zero. Can I merge captured timestamp into out_frame_? I guess there was a reason that they are separated. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:158: media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) { On 2015/04/16 23:55:13, mcasas wrote: > This condition is already covered inside IsExpectedDecodeResponse() > I believe. IsExpectedDecodeResponse returns true for this value and we have to do something here. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:185: gfx::Size dimensions = frame_format.frame_size; On 2015/04/16 23:55:13, mcasas wrote: > const? Done. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:186: scoped_refptr<media::VideoCaptureDevice::Client::Buffer> out_buffer = On 2015/04/16 23:55:13, mcasas wrote: > Suggest to directly allocate onto |out_buffer_| and in l.232 @wuchengli prefer to shorten lock time at the end of method. So allocate on stack first. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:201: size_t in_buffer_size = captured_data.length; On 2015/04/16 23:55:13, mcasas wrote: > Why an extra alias? Just rename the input param. I just want to keep the name of input param matching OnIncomingCapturedData. Do you still prefer rename it? https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:205: // Swap out to local variable in order to reduce lock time in IO thread. On 2015/04/16 23:55:13, mcasas wrote: > s/in/on/ Done. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:211: in_buffer_size > in_shared_memory->mapped_size()) { On 2015/04/16 23:55:14, mcasas wrote: > I suspect this allocate-as-you-go scheme might end up in a series of > initial allocations until we settle in a large enough size to hold > incoming frames. > Alternatively, consider pre allocating enough size to hold the > largest possible JPEG frame, which should be the size of > an uncompressed I420 frame, right? > > Then you should be able to recycle it instead of swapping > it in and out (which might degrade performance as we > discovered elsewhere). However JPEG does not guarantee compressed image is smaller than uncompressed image. And that wastes memory (virtually?). https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:220: if (!in_shared_memory->Map(in_buffer_size)) { On 2015/04/16 23:55:14, mcasas wrote: > Merge this with the previous error condition in l.213. Done. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:260: decoder_->Decode(in_buffer_, out_frame_); On 2015/04/16 23:55:13, mcasas wrote: > IIUC, only |in_buffer_->id()| is really needed. Please consider only > keeping id as a member variable for IsExpectedDecodeResponse(). in_* are for fallback. out_* are keep because we want to make VideoFrameReady (in IO thread) lighter weight. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:20: // Adapter to GpuJpegDecodeAccelerator for VideoDeviceDevice::Client. It takes On 2015/04/16 23:55:14, mcasas wrote: > s/VideoDeviceDevice/VideoCaptureDevice/ Done. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:21: // care GpuJpegDecodeAccelerator creation, shared memory, and thread issues. On 2015/04/16 23:55:14, mcasas wrote: > "care of" > "threading issues" Done. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:22: // If decoding failed, it will fallback to software decode for current frame On 2015/04/16 23:55:14, mcasas wrote: > s/failed/fails/ Done. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:25: // This class is mainly accessed by device thread. The decoding responses On 2015/04/16 23:55:14, mcasas wrote: > Please also mention that it's created and destroyed on Capture Thread. Done. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:65: // Fail state indicats to use software decode. See comment in On 2015/04/16 23:55:14, mcasas wrote: > s/indicats/indicates/ Done. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:87: void FallbackToSoftwareDecode(); On 2015/04/16 23:55:14, mcasas wrote: > I don't think this is really needed: If the current frame does not > get decoded correctly, just discard it and mark the error > condition. That means for certain device or (external) camera, the first frame is always got drop (and thus the latency is 33ms longer than others). Is this acceptable? > Moreover, that would allow you to not hold on to |captured_data_|, > note that all VideoCaptureDevice assume that the Client is done > with |data| when returning from OnIncomingCapturedXXX(), so > |captured_data_.data| accessing should be avoided. In linux case > in particular, this pointer is returned to the V4L2 driver. Before OnIncomingCapturedXXX() returns, the data is copied to |in_shared_memory_| and |captured_data_.data| is replaced by the pointer to |in_shared_memory_|. So accessing |captured_data_.data| is not a problem. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:92: external_jpeg_decoder_->DecodeCapturedData(data, length, frame_format, On 2015/04/16 23:55:14, mcasas wrote: > Add here DCHECK_EQ(frame_format.pixel_format, media::PIXEL_FORMAT_MJPEG); ? > Remember that a client may change the passed format on a per frame basis. Done. Since the format may change on a per frame basis, I make it as condition instead of CHECK.
Here are the comments so far. I'll finish the rest of the review later. https://codereview.chromium.org/1016773002/diff/100001/content/browser/media/... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/1016773002/diff/100001/content/browser/media/... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Please provide performance data of HW and SW JPEG decoders in the CL description. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc (right): https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:70: DVLOG(3) << __func__; Let's call DVLOG first so we can know what functions crash in the log easier. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h (right): https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:5: #ifndef CONTENT_BROWSER_RENDERER_HOST_GPU_JPEG_ACCELERATOR_ADAPTER_H_ I think the name GpuJpegDecoder is better. It's more consistent with the similar roles for video decoders like GpuVideoDecoder and RtcVideoDecoder. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:25: // This class is created, destroyed, and mainly accessed on device thread. The Device thread only applies for Linux. s/thread/thread for Linux/. "accessed" is vague. We should explain what functions should be called on what thread. Move "main accessed" to another sentence along with the rest of the paragraph. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:26: // decoding responses (JpegDecodeAccelerator::Client) are on IO thread. Since s/Client/Client functions/. s/are on/are run on/ https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:27: // it only decodes a frame at a time, the access is almost mutual exclusive: The access of what? almost? If it is "almost", it's not mutual exclusive. This mutual exclusive thing and member access are hard to understand. |lock_| is enough for synchronization. Right? Are you trying to say this is efficient? I'll discuss with you in person. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:33: // A lightweight check for caller to avoid IPC latency for known unsupported Add "True if JPEG hardware decoding is supported on this device." at the beginning. I think the original comments here should go to the implementation (gpu_jpeg_decode_accelerator_adapter.cc). It should be a TODO to be moved to GpuInfo. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:45: bool Initialize(); add a blank line https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:47: // VideoCaptureDevice::Client, should do software decoding by itself. What the caller actually does shouldn't be here. You should explain the state of GpuJpegDecodeAcceleratorAdapter and what should caller do with this object. For example, can the caller decode another Jpeg image? Should the caller destroy this object? https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:50: // Decode JPEG stream. Parameters are modeled after s/JPEG stream/a JPEG picture/. stream sounds like a series of images or a video stream. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:51: // VideoCaptureDeviceClient::OnIncomingCapturedData. Doesn't need to explain what this modeled from. Remove. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:59: // These will be called in IO thread. s/in/on/ https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:90: void FallbackToSoftwareDecode(); Fallback should be decided by the caller, not this class. Move this back to video_capture_device_client.cc. This class should handle hardware decoder related stuff only. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:95: scoped_refptr<base::MessageLoopProxy> device_thread_; Change the name. device thread only applies for Linux. This class should be generic. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:96: // All decoding requests are sent to |decoder_|. Add "The underlying JPEG decode accelerator."
Few comments so far. Will continue soon. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:51: media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) { On 2015/04/20 17:48:00, kcwu wrote: > On 2015/04/16 23:55:13, mcasas wrote: > > Prefer early termination; by the way shouldn't we return |false| if > > |bitstream_buffer_id| is kInvalidBitstreamBufferId ? Like: > > > > if (bitstream_buffer_id == > > media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) > > return false; > > LOG_IF(ERROR, !IsDecoding()) << "Got decode response while not decoding"; > > if (!IsDecoding()) > > return false; > > ... > > kInvalidBitstreamBufferId is possible value for NotifyError. For example, GPU > process crashed and the IPC channel disconnected. > The comment says "Check |bitstream_buffer_id| from GPU process is expected." so I'm not understanding how |media::JpegDecodeAccelerator::kInvalidBitstreamBufferId| is a valid BufferId. Perhaps this method needs revisiting or renaming. Perhaps NotifyError() is OK with |kInvalidBitstreamBufferId| but what about VideoFrameReady()? In any case, still consider unwarping the blocks: if (bitstream_buffer_id == media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) return true; LOG_IF(ERROR, !IsDecoding()) << "Got decode response while not decoding"; LOG_IF(ERROR, bitstream_buffer_id != in_buffer_.id() << ...; return IsDecoding() && bitstream_buffer_id == in_buffer_.id() https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc (right): https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:93: // Verify parameter from GPU process. Please remove superfluous comment. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:210: !in_shared_memory->Map(in_buffer_size)) { CreateAndMapAnonymous() [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/shared... https://codereview.chromium.org/1016773002/diff/100001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h (right): https://codereview.chromium.org/1016773002/diff/100001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h:65: scoped_refptr<base::MessageLoopProxy> io_message_loop_; const
@mcasas, I have some review questions in patchset 5 need your comments, especially the one on FallbackToSoftwareDecode. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:51: media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) { On 2015/04/22 01:23:52, mcasas wrote: > On 2015/04/20 17:48:00, kcwu wrote: > > On 2015/04/16 23:55:13, mcasas wrote: > > > Prefer early termination; by the way shouldn't we return |false| if > > > |bitstream_buffer_id| is kInvalidBitstreamBufferId ? Like: > > > > > > if (bitstream_buffer_id == > > > media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) > > > return false; > > > LOG_IF(ERROR, !IsDecoding()) << "Got decode response while not decoding"; > > > if (!IsDecoding()) > > > return false; > > > ... > > > > kInvalidBitstreamBufferId is possible value for NotifyError. For example, GPU > > process crashed and the IPC channel disconnected. > > > > The comment says > "Check |bitstream_buffer_id| from GPU process is expected." > so I'm not understanding how > |media::JpegDecodeAccelerator::kInvalidBitstreamBufferId| is > a valid BufferId. Perhaps this method needs revisiting or renaming. > Perhaps NotifyError() is OK with |kInvalidBitstreamBufferId| but > what about VideoFrameReady()? > > In any case, still consider unwarping the blocks: > > if (bitstream_buffer_id == > media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) > return true; > LOG_IF(ERROR, !IsDecoding()) << "Got decode response while not decoding"; > LOG_IF(ERROR, bitstream_buffer_id != in_buffer_.id() << ...; > return IsDecoding() && bitstream_buffer_id == in_buffer_.id() Done. Now I handle kInvalidBitstreamBufferId in NotifyError and return false here. I still don't use LOG_IF because I don't want to repeat these condition code twice. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc (right): https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:70: DVLOG(3) << __func__; On 2015/04/21 06:05:37, wuchengli wrote: > Let's call DVLOG first so we can know what functions crash in the log easier. Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:93: // Verify parameter from GPU process. On 2015/04/22 01:23:52, mcasas wrote: > Please remove superfluous comment. Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:210: !in_shared_memory->Map(in_buffer_size)) { On 2015/04/22 01:23:53, mcasas wrote: > CreateAndMapAnonymous() [1] > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/shared... Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h (right): https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:5: #ifndef CONTENT_BROWSER_RENDERER_HOST_GPU_JPEG_ACCELERATOR_ADAPTER_H_ On 2015/04/21 06:05:37, wuchengli wrote: > I think the name GpuJpegDecoder is better. It's more consistent with the similar > roles for video decoders like GpuVideoDecoder and RtcVideoDecoder. Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:25: // This class is created, destroyed, and mainly accessed on device thread. The On 2015/04/21 06:05:37, wuchengli wrote: > Device thread only applies for Linux. s/thread/thread for Linux/. > > "accessed" is vague. We should explain what functions should be called on what > thread. Move "main accessed" to another sentence along with the rest of the > paragraph. I rewrote the comment and removed the confusing sentences. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:26: // decoding responses (JpegDecodeAccelerator::Client) are on IO thread. Since On 2015/04/21 06:05:37, wuchengli wrote: > s/Client/Client functions/. s/are on/are run on/ Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:27: // it only decodes a frame at a time, the access is almost mutual exclusive: On 2015/04/21 06:05:37, wuchengli wrote: > The access of what? > > almost? If it is "almost", it's not mutual exclusive. This mutual exclusive > thing and member access are hard to understand. |lock_| is enough for > synchronization. Right? Are you trying to say this is efficient? I'll discuss > with you in person. I rewrote the comment and removed the confusing sentences. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:33: // A lightweight check for caller to avoid IPC latency for known unsupported On 2015/04/21 06:05:37, wuchengli wrote: > Add "True if JPEG hardware decoding is supported on this device." at the > beginning. I think the original comments here should go to the implementation > (gpu_jpeg_decode_accelerator_adapter.cc). It should be a TODO to be moved to > GpuInfo. Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:45: bool Initialize(); On 2015/04/21 06:05:37, wuchengli wrote: > add a blank line Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:47: // VideoCaptureDevice::Client, should do software decoding by itself. On 2015/04/21 06:05:37, wuchengli wrote: > What the caller actually does shouldn't be here. You should explain the state of > GpuJpegDecodeAcceleratorAdapter and what should caller do with this object. For > example, can the caller decode another Jpeg image? Should the caller destroy > this object? Remove that sentence and explain DecodeCaptureData() behavior in its comment. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:50: // Decode JPEG stream. Parameters are modeled after On 2015/04/21 06:05:37, wuchengli wrote: > s/JPEG stream/a JPEG picture/. stream sounds like a series of images or a video > stream. Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:51: // VideoCaptureDeviceClient::OnIncomingCapturedData. On 2015/04/21 06:05:37, wuchengli wrote: > Doesn't need to explain what this modeled from. Remove. Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:59: // These will be called in IO thread. On 2015/04/21 06:05:37, wuchengli wrote: > s/in/on/ Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:90: void FallbackToSoftwareDecode(); On 2015/04/21 06:05:37, wuchengli wrote: > Fallback should be decided by the caller, not this class. Move this back to > video_capture_device_client.cc. This class should handle hardware decoder > related stuff only. How about modify the name to NotifyClientDecodeFailed, and replace "device_client_->OnIncomingCapturedData" with |decode_failed_cb_|? |decode_failed_cb_| is got from the ctor. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:95: scoped_refptr<base::MessageLoopProxy> device_thread_; On 2015/04/21 06:05:37, wuchengli wrote: > Change the name. device thread only applies for Linux. This class should be > generic. Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:96: // All decoding requests are sent to |decoder_|. On 2015/04/21 06:05:37, wuchengli wrote: > Add "The underlying JPEG decode accelerator." Done. https://codereview.chromium.org/1016773002/diff/100001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h (right): https://codereview.chromium.org/1016773002/diff/100001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h:65: scoped_refptr<base::MessageLoopProxy> io_message_loop_; On 2015/04/22 01:23:53, mcasas wrote: > const Done.
https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:19: #include "ui/gl/gl_bindings.h" not used? https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:86: CHECK(decoder_thread_.Start()); Log an error and return false here. Let's not crash the entire GPU process when only JDA is not usable. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:134: const scoped_refptr<media::VideoFrame>& video_frame) { This function and Decode are both small and the prototype are the same. Move the code to Decode() and remove this function. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:204: base::AutoLock auto_lock(lock_); no need to acquire |lock_| https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:213: Check the state? Otherwise, all access of |state_| is on the child thread and won't need a lock. if (state_ != kInitialized) return; https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:6: // that utilizes hardware video decoder present on Intel CPUs. This is already explained in the class documentation. Remove. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:11: #include <map> not used https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:13: #include <utility> not used? https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:14: #include <vector> not used https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:16: #include "base/logging.h" Move to .cc? https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:18: #include "base/memory/shared_memory.h" not used? https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:21: #include "base/synchronization/condition_variable.h" not used https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:64: // Process one debuging request in the queue. s/debuging/decode/ https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:70: // Puts contents of |va_surface| into given |video_frame|, releases the s/Puts/Put/. s/releases/release/. s/passes/pass/ to be consistent with other comments. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:71: // surface and passes the resulting picture to client for output. s/passes the resulting picture/passes the buffer id of the resulting picture/ https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:73: int32_t input_id, s/input_id/input_buffer_id/ to be more descriptive https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:77: enum State { Use a boolean if you only need two states. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:88: // An input buffer awaiting consumption, provided by the client. |video_frame| is the output, so this is not only input. How about "An input buffer and the corresponding output video frame."? https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:90: DecodeRequest(media::BitstreamBuffer bitstream_buffer, const & https://codereview.chromium.org/1016773002/diff/140001/media/video/jpeg_decod... File media/video/jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/140001/media/video/jpeg_decod... media/video/jpeg_decode_accelerator.cc:7: #include "base/logging.h" remove https://codereview.chromium.org/1016773002/diff/140001/media/video/jpeg_decod... File media/video/jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/140001/media/video/jpeg_decod... media/video/jpeg_decode_accelerator.h:20: // keeps the ownership of them. Document "Initialize and Destroy must be called on the same thread."
https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.h (right): https://codereview.chromium.org/1016773002/diff/60001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.h:237: scoped_ptr<content::GpuJpegDecodeAccelerator> jpeg_decoder_; On 2015/04/15 07:11:57, wuchengli wrote: > Need a way to support multiple jpeg decoders here. Does ScopedPtrHashMap work? Done. ScopedPtrHashMap need to support custom deleter. scoped_ptr_hash_map.h is under review here https://codereview.chromium.org/1099383002/ I will rebase once it committed.
https://codereview.chromium.org/1016773002/diff/160001/base/containers/scoped... File base/containers/scoped_ptr_hash_map.h (right): https://codereview.chromium.org/1016773002/diff/160001/base/containers/scoped... base/containers/scoped_ptr_hash_map.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. change of scoped_ptr_hash_map* is under review here https://codereview.chromium.org/1099383002/
https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:20: // requires an IPC. s/IPC/IPC even for platforms that do not support HW decoder/ https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:21: // TODO(kcwu): move this logic to GpuInfo. s/logic/information/ https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:130: // thread, the next frame will call DecodeCapturedData() and drop immediately Hmm. If we may still drop a frame, I prefer dropping this frame and we don't need to remember all these |captured_data|. https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:80: if (!external_jpeg_decoder_->Initialize()) { We'll try this for every frame if this fails. Can we do external_jpeg_decoder_.reset(new GpuJpegDecoder(this)); in the constructor if GpuJpegDecoder::Supported() is true? https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:92: !external_jpeg_decoder_->IsFailed()) { Let's have a failure callback from GpuJpegDecoder and clears |external_jpeg_decoder_| in the callback. A simple NotifyError or base::Callback<void(Status status)> DecodeCB; will be enough. FallbackToSoftwareDecode should also be handled there. https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:93: external_jpeg_decoder_->DecodeCapturedData(data, length, frame_format, DecodeCapturedData can return a value to indicate the failure. If failed, just continue OnIncomingCapturedData and it's falling back to software. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:263: // This function is called from GpuJpegDecoder's main task thread, which is Just call it task thread. main task thread sounds confusing. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:272: // usually blocking wait for next frame, so it expects decoder responses are s/blocking wait/waiting/. Here it refers to the main task thread, which doesn't expect anything. I think this is clearer. s/it expects decoder responses are send/decoder responses are sent/. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:278: decoder->AsWeakPtr(), reply_loop)); Why not just use |io_loop|? reply_loop is always IO thread. GpuJpegDecoder::Initialize also hard-codes the IO thread there. DCHECK in VideoFrameReady should make sure it's the right thread. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:10: #include "base/synchronization/waitable_event.h" not used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:56: DCHECK(CalledOnValidThread()); io thread? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:86: DCHECK(CalledOnValidThread()); Isn't this io thread? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:138: DCHECK(CalledOnValidThread()); This is io thread. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:139: // Post the error notification back to this thread, to avoid re-entrancy. I still don't understand how re-entrancy is possible. I know OnNotifyError better not to be called directly because Client::NotifyError() may Destroy() |this|. If you cannot figure out the call flow of re-entrancy, I suggest change the comments to the reason I mentioned. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:140: base::MessageLoopProxy::current()->PostTask( If InvalidateWeakPtrs is guaranteed to run on io thread as DCHECK shows in OnNotifyError. We should use io_message_loop_ here to make sure the weak pointer is dereferenced on io thread. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:151: DLOG(ERROR) << "Send(" << message_type << ") failed"; use message->type() directly https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:158: remove blank line https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:166: if (!client_) I'm not sure if this is OK to access |client_| from different threads in OnNotifyError and Destroy. Please look at the code and explain why this is OK. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h:27: // |io_message_loop| is IO thread loop for thread checking. The variable name is obvious it's IO thread. "for thread checking" can be moved to variable declaration. Remove. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h:64: // IO message loop. Add "Used for thread checking.". https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_channel.cc:16: #include "base/macros.h" not used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_channel.cc:25: #include "content/common/gpu/media/gpu_jpeg_decode_accelerator.h" gpu_channel.h already has this https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_channel.cc:812: ignore_result(decoder.release()); not needed? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... File content/common/gpu/gpu_channel.h (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_channel.h:238: base::ScopedPtrHashMap<int32, content::GpuJpegDecodeAccelerator> Add a comment to say int32 is route id. For example, "A map from route id to Jpeg decoder." https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:789: // These messages are sent from the GPU process to Browser process. add a blank line https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:792: int32) /* Bitstream buffer ID */ s/Bitstream buffer ID/bitstream_buffer_id/ to be consistent https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:796: int32_t, /* bitstream_buffer_id */ s/int32_t/int32/. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:8: #include <vector> not used https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:11: #include "base/command_line.h" not used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:14: #include "base/stl_util.h" not used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:17: #include "content/public/common/content_switches.h" not used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:18: #include "gpu/command_buffer/common/command_buffer.h" not used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:20: #include "ipc/ipc_message_utils.h" is this used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:22: #include "media/base/limits.h" not used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:56: // This will delete |owner_| and |this|. This doesn't directly delete |owner_| anymore. I think this can be removed. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:120: DLOG(ERROR) << "GpuJpegDecodeAccelerator::Initialize(): " AddRoute only fails when id is duplicated. If that happens, it's a bug somewhere. So this can be changed to LOG(ERROR). https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:128: // When add more platforms, GpuJpegDecodeAcceleratorAdapter::Supported need s/add/adding/ https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:138: if (!jpeg_decode_accelerator_->Initialize(this)) { return jpeg_decode_accelerator_->Initialize(this) directly is enough. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:155: Send(new AcceleratedJpegDecoderHostMsg_VideoFrameReady(host_route_id_, I remember now. ChannelProxy::Send requires to be called on the child thread. That's why a filter is used in GpuVideoDecodeAccelerator to send IPC on IO thread. See DCHECK in line 454. https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_pr... Same for Send in NotifyError if it is called on decode thread. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:227: // We're destroying; cancel all callbacks. We don't see we are cancelling any callback? Maybe just remove. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:231: void GpuJpegDecodeAccelerator::Destroy() { Add DCHECK(child_message_loop_->BelongsToCurrentThread());. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:8: #include "base/compiler_specific.h" I forgot. Is this used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:10: #include "base/memory/shared_memory.h" not used? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:16: struct AcceleratedJpegDecoderMsg_Decode_Params; This is not in content namespace? https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:48: bool Initialize(); Add function comments. Put this together with Destroy().
Hi @mcasas, when you have time, could you please reply my questions in patchset 5 first? especially the question on FallbackToSoftwareDecode. Depending on your answer, so I know how to revise my CL. Thanks.
Some more review. Apologies for the delay. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:102: captured_data_.timestamp); On 2015/04/20 17:48:00, kcwu wrote: > On 2015/04/16 23:55:13, mcasas wrote: > > If you discard |captured_data_|, you can use |out_frame_.timestamp()| > > out_frame_.timestamp() was zero. Can I merge captured timestamp into out_frame_? > I guess there was a reason that they are separated. > IIRC, VideoFrame is created in VideoCaptureDeviceClient but the timestamp was not plugged into it, it was sent via IPC instead, for no particular reason IMHO. (I'm ad libbing a bit). https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:87: void FallbackToSoftwareDecode(); On 2015/04/20 17:48:00, kcwu wrote: > On 2015/04/16 23:55:14, mcasas wrote: > > I don't think this is really needed: If the current frame does not > > get decoded correctly, just discard it and mark the error > > condition. > That means for certain device or (external) camera, the first frame is always > got drop (and thus the latency is 33ms longer than others). Is this acceptable? > IIUC, the drop-first-frame would be an error condition on those devices, correct? IOW if the decoder accelerator works, that frame would not be lost. So dropping the first frame is a corner case of a particular hardware support, and then it doesn't matter much if the delay is increased by a frame, and it simplifies the code. > > > Moreover, that would allow you to not hold on to |captured_data_|, > > note that all VideoCaptureDevice assume that the Client is done > > with |data| when returning from OnIncomingCapturedXXX(), so > > |captured_data_.data| accessing should be avoided. In linux case > > in particular, this pointer is returned to the V4L2 driver. > > Before OnIncomingCapturedXXX() returns, the data is copied to > |in_shared_memory_| and |captured_data_.data| is replaced by the pointer to > |in_shared_memory_|. So accessing |captured_data_.data| is not a problem. So |captured_data|.data| points to two different locations? I'll take a look on the latest patchset so you don't have to come back in history to this PS. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:70: memset(&captured_data_, 0, sizeof(captured_data_)); Is this needed or is |captured_data_| set before use? (Assuming FallbackToSoftwareDecode() is removed.) https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:95: void GpuJpegDecoder::VideoFrameReady( VideoFrameReadyOnIOThread()? https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:103: device_client_->OnIncomingCapturedVideoFrame(out_buffer_, out_frame_, DecodeDone() in l.106 is executed in mutual exclusion thanks to |lock_|, but here we use |out_buffer_| and |out_frame_| out of the |lock_|. Correct? https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:113: DCHECK_EQ(BrowserThread::CurrentlyOn(BrowserThread::IO), IsDecoding()); These chained DCHECKs are a symptom of excessive complexity, I'd say. +1 for removing FallbackToSoftwareDecode(): if GpuJpegDecoder() has failed, VideoCaptureDeviceClient should never jump into this class. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:181: { l.181-185: if (!IsFailed()) return; https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:185: } There is quite a lot of sequential locking-unlocking, e.g. here you call IsDecoding() in l.177 then lock() again in l.182 etc. DecodeDone() and IsFailed() also use |lock_|. Too many mutual exclusion areas in code is a symptom of complex signalling between threads: hard to maintain etc. Maybe you could get the fail/ok state into a local class ThreadSafeState{ public: enum State {OK, FAIL} ThreadSafeState(State state) : state_(state) {} bool state() const { base::AutoLock lock(lock_); return state_; } void set_state() { base::AutoLock lock(lock_); state_ = state; } private: base::AutoLock lock_; State state_; }; And at least that part of the state tracking would clutter less the specifics of GpuJpegDecoder -- and it would use it own |lock_| which IIUC is actually independent from the locking to be done in data specifics. I'm not super sure about that class anyway, WDYT? In any case please have another go at the locking- modifying-unlocking and refactor. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:253: base::AutoLock lock(lock_); l.253-255: if(!IsFailed()) return; https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:32: // Return true if JPEG hardware decoding is supported on this device. s/Return/Returns/ (Comments are descriptive rather than imperative [1) [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:56: virtual void VideoFrameReady(int32_t buffer_id) override; Suggest s/VideoFrameReady/VideoFrameReadyOnIOThread/ since this class seems to live mostly on Device Thread. Same s/NotifyError/NotifiyErrorOnIOThread/ https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:91: const scoped_refptr<base::MessageLoopProxy> main_task_runner_; Hmm |main| can have another meaning in the Browser process. Change it to "Device Thread", this thread exists on all platforms, although in most it is shared with "Audio Thread" and is labeled as such. Device Thread is also called "Capture Thread", so I suggest |capture_task_runner_|. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:61: base::SharedMemoryHandle* handle); You'll have to rebase this area quite a bit. Basically extending BufferHandle [1] and implementing it in SimpleBufferHandle [2]. I'd like to suggest AsPlatformHandle() as the new virtual method name, then GpuMemoryBufferBufferHandle [3] would implement it as gmb_->GetHandle(); [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... [3] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
I uploaded my progress so far for you to review.I will continue to fix remain issues next week (this Friday is holiday in Taiwan). All comments are addressed except one: - wucheng's comment for GpuJpegDecodeAccelerator::VideoFrameReady. p.s. I just rebased code for BufferHandle and make it compile, but I haven't run it yet. Not sure it works or not, sorry. https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:20: // requires an IPC. On 2015/04/24 05:54:23, wuchengli wrote: > s/IPC/IPC even for platforms that do not support HW decoder/ Done. https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:21: // TODO(kcwu): move this logic to GpuInfo. On 2015/04/24 05:54:23, wuchengli wrote: > s/logic/information/ Done. https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:130: // thread, the next frame will call DecodeCapturedData() and drop immediately On 2015/04/24 05:54:23, wuchengli wrote: > Hmm. If we may still drop a frame, I prefer dropping this frame and we don't > need to remember all these |captured_data|. the behavior changed to always drop current frame if anything failed. https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:80: if (!external_jpeg_decoder_->Initialize()) { On 2015/04/24 05:54:23, wuchengli wrote: > We'll try this for every frame if this fails. Can we do > external_jpeg_decoder_.reset(new GpuJpegDecoder(this)); in the constructor if > GpuJpegDecoder::Supported() is true? Done. https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:92: !external_jpeg_decoder_->IsFailed()) { On 2015/04/24 05:54:23, wuchengli wrote: > Let's have a failure callback from GpuJpegDecoder and clears > |external_jpeg_decoder_| in the callback. A simple NotifyError or > base::Callback<void(Status status)> DecodeCB; will be enough. > FallbackToSoftwareDecode should also be handled there. Since |external_jpeg_decoder_| is created in the constructor (IO thread). To make things simpler, |external_jpeg_decoder_| will only enter fail state, never delete during capturing. https://codereview.chromium.org/1016773002/diff/160001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:93: external_jpeg_decoder_->DecodeCapturedData(data, length, frame_format, On 2015/04/24 05:54:23, wuchengli wrote: > DecodeCapturedData can return a value to indicate the failure. If failed, just > continue OnIncomingCapturedData and it's falling back to software. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:263: // This function is called from GpuJpegDecoder's main task thread, which is On 2015/04/24 05:54:23, wuchengli wrote: > Just call it task thread. main task thread sounds confusing. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:272: // usually blocking wait for next frame, so it expects decoder responses are On 2015/04/24 05:54:23, wuchengli wrote: > s/blocking wait/waiting/. > > Here it refers to the main task thread, which doesn't expect anything. I think > this is clearer. s/it expects decoder responses are send/decoder responses are > sent/. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:278: decoder->AsWeakPtr(), reply_loop)); On 2015/04/24 05:54:23, wuchengli wrote: > Why not just use |io_loop|? reply_loop is always IO thread. > GpuJpegDecoder::Initialize also hard-codes the IO thread there. DCHECK in > VideoFrameReady should make sure it's the right thread. I want to make these thread logic stay in GpuJpegDecoder as much as possible. Make GpuChannelHost neutral. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:10: #include "base/synchronization/waitable_event.h" On 2015/04/24 05:54:23, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:56: DCHECK(CalledOnValidThread()); On 2015/04/24 05:54:23, wuchengli wrote: > io thread? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:86: DCHECK(CalledOnValidThread()); On 2015/04/24 05:54:23, wuchengli wrote: > Isn't this io thread? No, Decode() is called by GpuJpegDecoder::DecodeCapturedData() directly on device thread. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:138: DCHECK(CalledOnValidThread()); On 2015/04/24 05:54:23, wuchengli wrote: > This is io thread. I modified to make PostNotifyError is only invoked in device thread and post task to io thread. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:139: // Post the error notification back to this thread, to avoid re-entrancy. On 2015/04/24 05:54:23, wuchengli wrote: > I still don't understand how re-entrancy is possible. I know OnNotifyError > better not to be called directly because Client::NotifyError() may Destroy() > |this|. If you cannot figure out the call flow of re-entrancy, I suggest change > the comments to the reason I mentioned. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:140: base::MessageLoopProxy::current()->PostTask( On 2015/04/24 05:54:23, wuchengli wrote: > If InvalidateWeakPtrs is guaranteed to run on io thread as DCHECK shows in > OnNotifyError. We should use io_message_loop_ here to make sure the weak pointer > is dereferenced on io thread. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:151: DLOG(ERROR) << "Send(" << message_type << ") failed"; On 2015/04/24 05:54:23, wuchengli wrote: > use message->type() directly Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:158: On 2015/04/24 05:54:23, wuchengli wrote: > remove blank line Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:166: if (!client_) On 2015/04/24 05:54:23, wuchengli wrote: > I'm not sure if this is OK to access |client_| from different threads in > OnNotifyError and Destroy. Please look at the code and explain why this is OK. Removed code about |client_| in Destroy. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h:27: // |io_message_loop| is IO thread loop for thread checking. On 2015/04/24 05:54:23, wuchengli wrote: > The variable name is obvious it's IO thread. "for thread checking" can be moved > to variable declaration. Remove. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h:64: // IO message loop. On 2015/04/24 05:54:24, wuchengli wrote: > Add "Used for thread checking.". Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_channel.cc:16: #include "base/macros.h" On 2015/04/24 05:54:24, wuchengli wrote: > not used? For ignore_result() https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_channel.cc:25: #include "content/common/gpu/media/gpu_jpeg_decode_accelerator.h" On 2015/04/24 05:54:24, wuchengli wrote: > gpu_channel.h already has this Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_channel.cc:812: ignore_result(decoder.release()); On 2015/04/24 05:54:24, wuchengli wrote: > not needed? GpuChannel::ReleaseJpegDecoder is called from GpuJpegDecodeAccelerator::Destroy, where decoder will delete itself. This function just release the pointer and don't delete it. If without this line, decoder will be deleted here. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... File content/common/gpu/gpu_channel.h (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_channel.h:238: base::ScopedPtrHashMap<int32, content::GpuJpegDecodeAccelerator> On 2015/04/24 05:54:24, wuchengli wrote: > Add a comment to say int32 is route id. For example, "A map from route id to > Jpeg decoder." Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:789: // These messages are sent from the GPU process to Browser process. On 2015/04/24 05:54:24, wuchengli wrote: > add a blank line Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:792: int32) /* Bitstream buffer ID */ On 2015/04/24 05:54:24, wuchengli wrote: > s/Bitstream buffer ID/bitstream_buffer_id/ to be consistent Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:796: int32_t, /* bitstream_buffer_id */ On 2015/04/24 05:54:24, wuchengli wrote: > s/int32_t/int32/. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:8: #include <vector> On 2015/04/24 05:54:25, wuchengli wrote: > not used Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:11: #include "base/command_line.h" On 2015/04/24 05:54:25, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:14: #include "base/stl_util.h" On 2015/04/24 05:54:24, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:17: #include "content/public/common/content_switches.h" On 2015/04/24 05:54:25, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:18: #include "gpu/command_buffer/common/command_buffer.h" On 2015/04/24 05:54:24, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:20: #include "ipc/ipc_message_utils.h" On 2015/04/24 05:54:24, wuchengli wrote: > is this used? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:22: #include "media/base/limits.h" On 2015/04/24 05:54:24, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:56: // This will delete |owner_| and |this|. On 2015/04/24 05:54:24, wuchengli wrote: > This doesn't directly delete |owner_| anymore. I think this can be removed. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:120: DLOG(ERROR) << "GpuJpegDecodeAccelerator::Initialize(): " On 2015/04/24 05:54:24, wuchengli wrote: > AddRoute only fails when id is duplicated. If that happens, it's a bug > somewhere. So this can be changed to LOG(ERROR). Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:128: // When add more platforms, GpuJpegDecodeAcceleratorAdapter::Supported need On 2015/04/24 05:54:24, wuchengli wrote: > s/add/adding/ Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:138: if (!jpeg_decode_accelerator_->Initialize(this)) { On 2015/04/24 05:54:24, wuchengli wrote: > return jpeg_decode_accelerator_->Initialize(this) directly is enough. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:227: // We're destroying; cancel all callbacks. On 2015/04/24 05:54:24, wuchengli wrote: > We don't see we are cancelling any callback? Maybe just remove. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:231: void GpuJpegDecodeAccelerator::Destroy() { On 2015/04/24 05:54:24, wuchengli wrote: > Add DCHECK(child_message_loop_->BelongsToCurrentThread());. Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:8: #include "base/compiler_specific.h" On 2015/04/24 05:54:25, wuchengli wrote: > I forgot. Is this used? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:10: #include "base/memory/shared_memory.h" On 2015/04/24 05:54:25, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:16: struct AcceleratedJpegDecoderMsg_Decode_Params; On 2015/04/24 05:54:25, wuchengli wrote: > This is not in content namespace? GPU msg structs are in global namespace. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:48: bool Initialize(); On 2015/04/24 05:54:25, wuchengli wrote: > Add function comments. Put this together with Destroy(). Done. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:70: memset(&captured_data_, 0, sizeof(captured_data_)); On 2015/04/28 00:04:49, mcasas wrote: > Is this needed or is |captured_data_| set before use? > (Assuming FallbackToSoftwareDecode() is removed.) Done. Not needed. Removed. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:95: void GpuJpegDecoder::VideoFrameReady( On 2015/04/28 00:04:49, mcasas wrote: > VideoFrameReadyOnIOThread()? This name is inheritance from media::JpegDecodeAccelerator::Client https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:103: device_client_->OnIncomingCapturedVideoFrame(out_buffer_, out_frame_, On 2015/04/28 00:04:49, mcasas wrote: > DecodeDone() in l.106 is executed in mutual exclusion > thanks to |lock_|, but here we use |out_buffer_| > and |out_frame_| out of the |lock_|. Correct? Done. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:113: DCHECK_EQ(BrowserThread::CurrentlyOn(BrowserThread::IO), IsDecoding()); On 2015/04/28 00:04:49, mcasas wrote: > These chained DCHECKs are a symptom of excessive complexity, > I'd say. +1 for removing FallbackToSoftwareDecode(): if > GpuJpegDecoder() has failed, VideoCaptureDeviceClient > should never jump into this class. Done. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:181: { On 2015/04/28 00:04:50, mcasas wrote: > l.181-185: > > if (!IsFailed()) > return; Done. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:185: } On 2015/04/28 00:04:49, mcasas wrote: > There is quite a lot of sequential locking-unlocking, e.g. > here you call IsDecoding() in l.177 then lock() again in l.182 > etc. DecodeDone() and IsFailed() also use |lock_|. Too > many mutual exclusion areas in code is a symptom of > complex signalling between threads: hard to maintain > etc. Maybe you could get the fail/ok state into a local > > class ThreadSafeState{ > public: > enum State {OK, FAIL} > ThreadSafeState(State state) : state_(state) {} > > bool state() const { > base::AutoLock lock(lock_); > return state_; > } > void set_state() { > base::AutoLock lock(lock_); > state_ = state; > } > > private: > base::AutoLock lock_; > State state_; > }; > > And at least that part of the state tracking would > clutter less the specifics of GpuJpegDecoder -- and > it would use it own |lock_| which IIUC is > actually independent from the locking to be > done in data specifics. > > I'm not super sure about that class anyway, WDYT? > In any case please have another go at the locking- > modifying-unlocking and refactor. Locking is refactored now. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:253: base::AutoLock lock(lock_); On 2015/04/28 00:04:50, mcasas wrote: > l.253-255: > > if(!IsFailed()) > return; Not sure what do you mean. Please take a look anyway since the locking is revised. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:32: // Return true if JPEG hardware decoding is supported on this device. On 2015/04/28 00:04:50, mcasas wrote: > s/Return/Returns/ > (Comments are descriptive rather than imperative [1) > > [1] > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... Done. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:56: virtual void VideoFrameReady(int32_t buffer_id) override; On 2015/04/28 00:04:50, mcasas wrote: > Suggest s/VideoFrameReady/VideoFrameReadyOnIOThread/ > since this class seems to live mostly on Device Thread. > > Same s/NotifyError/NotifiyErrorOnIOThread/ These names are inheritance from JpegDecodeAccelerator::Client and thus cannot be changed. Or do you have suggestions to deal that? https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:91: const scoped_refptr<base::MessageLoopProxy> main_task_runner_; On 2015/04/28 00:04:50, mcasas wrote: > Hmm |main| can have another meaning in the Browser process. > Change it to "Device Thread", this thread > exists on all platforms, although in most it is shared with > "Audio Thread" and is labeled as such. Device Thread is also > called "Capture Thread", so I suggest |capture_task_runner_|. Done. https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/1016773002/diff/180001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:61: base::SharedMemoryHandle* handle); On 2015/04/28 00:04:50, mcasas wrote: > You'll have to rebase this area quite a bit. Basically extending > BufferHandle [1] and implementing it in SimpleBufferHandle [2]. > I'd like to suggest AsPlatformHandle() as the new virtual > method name, then GpuMemoryBufferBufferHandle [3] > would implement it as gmb_->GetHandle(); > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... > [3] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... Done. Thanks for your kind help. Please take a look. I'm not sure should they return "gfx::GpuMemoryBufferHandle" everywhere or not.
Hi, Miguel Please take a look. Now AsPlatformHandle works.
https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:135: // Since IsDecoding_Locked() is false here and we only decode a frame at a This is not right. Suppose DecodeCapturedData is called twice in a row. The first call reaches line 193 and the second call is here. The second call will see IsDecoding_Locked==true here. Can we move line 121-130 to line 190? So we don't have the above problem and we don't need to check |failed_| twice. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. I know the current patchset has a bug and the performance data is not final. But please add what you have to the change description so people know the rough figure. We need two numbers -- CPU usage and latency. Should we use Broadwell or Haswell Chromebooks for benchmark? https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:48: const DecodeDoneCB& decode_done_cb); Can GpuJpegDecoder call device_client_->OnIncomingCapturedVideoFrame() and let OnIncomingCapturedVideoFrame call DoIncomingCapturedVideoFrameOnIOThread directly if it is on IO thread? First, we already pass media::VideoCaptureDevice::Client to the constructor and it's convenient to use it. Second, passing a VideoCaptureController and |controller_| is harder to think of the lifecycle. For example, VideoCaptureController is one layer further and is |controller_| always valid when GpuJpegDecoder calls it? https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:51: // Creates and intializes decoder in GPU side. Do nothing if already "Do nothing if already initialized" is strange. The knowledge should be maintained in the client. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:52: // initialized. Return falses if failed. s/Return falses/Returns false/ https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:79: // The capture thread. Add documentation what a capture thread is. Without some context, a developer (like me) does not know what that is. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:89: bool failed_; Add a status callback so you don't need |failed_|. See |status_cb| of GpuVideoDecoder::Initialize. The client should decide what to do if decode fails. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:231: ignore_result(external_jpeg_decoder_->Initialize()); Why not set |external_jpeg_decoder_| to NULL if Initialize fails? GpuJpegDecoder won't need |initialized_|.
https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:130: return false; Can we return a enum from this function? SUCCESS (true), FAILED (rotation != 0), and FATAL (failed_==true, the client can clear |external_jpeg_decoder_|). https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:89: bool failed_; On 2015/05/04 14:14:41, wuchengli wrote: > Add a status callback so you don't need |failed_|. See |status_cb| of > GpuVideoDecoder::Initialize. The client should decide what to do if decode > fails. See the comment in DecodeCapturedData. If we return an enum from DecodeCapturedData, we don't need the status callback and video_capture_device_client.cc can clean |external_jpeg_decoder_| if returned status is FATAL_ERROR. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:231: ignore_result(external_jpeg_decoder_->Initialize()); On 2015/05/04 14:14:41, wuchengli wrote: > Why not set |external_jpeg_decoder_| to NULL if Initialize fails? GpuJpegDecoder > won't need |initialized_|. As discussed, if Initialize fails, there will be no any callback. So it's safe to clear |external_jpeg_decoder_| here. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:263: // This function is called from GpuJpegDecoder's capture task thread, which is s/which is// https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... File content/common/gpu/client/gpu_channel_host.h (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.h:152: // Creates a JPEG decoder in the GPU process. Also explain what |reply_loop| is. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:81: base::AutoLock lock(channel_lock_); As discussed, dealing with synchronization in GpuJpegDecodeAcceleratorHost is complex and annoying. Try adding a Jpeg or media thread. See if it simplifies both GpuJpegDecodeAcceleratorHost and GpuJpegDecoder.
A few more comments in // with wuchengli@'s. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:124: // fallback to software decoding) before the former frame. Wait, is there still fallback to software decoding? https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:230: external_jpeg_decoder_) With this condition, we would be calling external_jpeg_decoder_->Initialize() on every incoming MJPEG frame, is that what we want? https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:238: external_jpeg_decoder_) { nit: Merge all conditions in lines l.237-238. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:22: #if defined(ARCH_CPU_X86_FAMILY) #if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:109: DVLOG(3) << __func__; As we approach something stable you should remove these __func__ printouts. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:129: return false; Move these |return false| further up in the method, i.e. why AddRoute() and AddFilter() if the platform does not support jpeg_decode_accelerator_ anyway? https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:152: // function is to just keep reference of |shm| to make sure it live util s/live/lives/ https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:153: // decode finished. s/finished/finishes/ https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:72: // The message filter to run JDA::Decode on IO thread. No abbreviations please. (Rationale: sometimes we want to grep/sed for JpegDecodeAccelerator and would like all this lines to show up). https://codereview.chromium.org/1016773002/diff/260001/media/video/capture/vi... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/1016773002/diff/260001/media/video/capture/vi... media/video/capture/video_capture_device.h:206: virtual base::SharedMemoryHandle AsPlatformHandle() = 0; Can you use base::PlatformFile instead of base::SharedMemoryHandle. The implementation would be something like: base::PlatformFile AsPlatformFile() override { #if defined(OS_POSIX) return shared_memory_.handle().fd; #else return shared_memory_.handle(); #endif } The point being that, in the immediate future, not all Buffers will be memory backed, some will be Ozone Native Buffers, and those have no SharedMemoryHandle. See [1]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... https://codereview.chromium.org/1016773002/diff/260001/media/video/jpeg_decod... File media/video/jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/260001/media/video/jpeg_decod... media/video/jpeg_decode_accelerator.cc:18: void DefaultDeleter<media::JpegDecodeAccelerator>::operator()( Do we still need this generic DefaultDeleter or should we specialize it for VaapiJpegDecodeAccelerator instead?
https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:124: // fallback to software decoding) before the former frame. On 2015/05/07 00:59:03, mcasas wrote: > Wait, is there still fallback to software decoding? I discussed with Kuang-che. He'll remove the fallback for now. There are all kinds of external cameras. It's possible HW decoder cannot decode some of them. So we should fallback. Otherwise, those cameras won't work at all. On the other hand, we want the test engineers and dogfooders to report bugs when something is broken. If we fallback even when GPU process crashes, it's hard to know when a bug happens. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:230: external_jpeg_decoder_) On 2015/05/07 00:59:03, mcasas wrote: > With this condition, we would be calling > external_jpeg_decoder_->Initialize() on every > incoming MJPEG frame, is that what we want? Not every incoming frame. Only when format changes. I also asked Kuang-che to clear |external_jpeg_decoder_| when initialization fails. So it won't try to initialize again next time the format changes.
Major changes in this patchset. 1. Use jpeg thread. GpuJpegDecoder and GpuJpegDecodeAcceleratorHost are much simplified. (no IO thread optimization yet) 2. No software fallback at all. 3. to @mcasas, I added bytes_used back to V4L2CaptureDelegate::SendBuffer. after that, GpuJpegDecoder can allocate and memcpy (10x) less. 4. to @wuchengli, with 3, the performance is much better. (near but not yet as good as several weeks ago). The current performance number on peppy is updated to the CL description. 5. and addressed all comments. p.s. latency on link was 3.5ms at Mar 24. 5.5 ms last week. 4.3 ms now. I will try do things on IO thread directly and measure the performance again. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:102: captured_data_.timestamp); On 2015/04/28 00:04:49, mcasas wrote: > On 2015/04/20 17:48:00, kcwu wrote: > > On 2015/04/16 23:55:13, mcasas wrote: > > > If you discard |captured_data_|, you can use |out_frame_.timestamp()| > > > > out_frame_.timestamp() was zero. Can I merge captured timestamp into > out_frame_? > > I guess there was a reason that they are separated. > > > > IIRC, VideoFrame is created in VideoCaptureDeviceClient but > the timestamp was not plugged into it, it was sent via IPC > instead, for no particular reason IMHO. (I'm ad libbing a bit). Acknowledged. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.cc:211: in_buffer_size > in_shared_memory->mapped_size()) { On 2015/04/20 17:48:00, kcwu wrote: > On 2015/04/16 23:55:14, mcasas wrote: > > I suspect this allocate-as-you-go scheme might end up in a series of > > initial allocations until we settle in a large enough size to hold > > incoming frames. > > Alternatively, consider pre allocating enough size to hold the > > largest possible JPEG frame, which should be the size of > > an uncompressed I420 frame, right? > > > > Then you should be able to recycle it instead of swapping > > it in and out (which might degrade performance as we > > discovered elsewhere). > > However JPEG does not guarantee compressed image is smaller than uncompressed > image. And that wastes memory (virtually?). Now I allocate 2x input size at first time. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h (right): https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:87: void FallbackToSoftwareDecode(); On 2015/04/28 00:04:49, mcasas wrote: > On 2015/04/20 17:48:00, kcwu wrote: > > On 2015/04/16 23:55:14, mcasas wrote: > > > I don't think this is really needed: If the current frame does not > > > get decoded correctly, just discard it and mark the error > > > condition. > > That means for certain device or (external) camera, the first frame is always > > got drop (and thus the latency is 33ms longer than others). Is this > acceptable? > > > > IIUC, the drop-first-frame would be an error condition on those > devices, correct? IOW if the decoder accelerator works, that > frame would not be lost. So dropping the first frame is a corner > case of a particular hardware support, and then it doesn't matter > much if the delay is increased by a frame, and it simplifies the > code. > > > > > > Moreover, that would allow you to not hold on to |captured_data_|, > > > note that all VideoCaptureDevice assume that the Client is done > > > with |data| when returning from OnIncomingCapturedXXX(), so > > > |captured_data_.data| accessing should be avoided. In linux case > > > in particular, this pointer is returned to the V4L2 driver. > > > > Before OnIncomingCapturedXXX() returns, the data is copied to > > |in_shared_memory_| and |captured_data_.data| is replaced by the pointer to > > |in_shared_memory_|. So accessing |captured_data_.data| is not a problem. > > So |captured_data|.data| points to two different locations? > I'll take a look on the latest patchset so you don't have to > come back in history to this PS. Acknowledged. https://codereview.chromium.org/1016773002/diff/80001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decode_accelerator_adapter.h:103: CapturedData captured_data_; On 2015/04/16 23:55:14, mcasas wrote: > See my comments above, I think we could get rid of this. Done. https://codereview.chromium.org/1016773002/diff/100001/content/browser/media/... File content/browser/media/capture/web_contents_video_capture_device_unittest.cc (right): https://codereview.chromium.org/1016773002/diff/100001/content/browser/media/... content/browser/media/capture/web_contents_video_capture_device_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/04/21 06:05:36, wuchengli wrote: > Please provide performance data of HW and SW JPEG decoders in the CL > description. Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:19: #include "ui/gl/gl_bindings.h" On 2015/04/23 09:35:18, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:86: CHECK(decoder_thread_.Start()); On 2015/04/23 09:35:18, wuchengli wrote: > Log an error and return false here. Let's not crash the entire GPU process when > only JDA is not usable. Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:134: const scoped_refptr<media::VideoFrame>& video_frame) { On 2015/04/23 09:35:18, wuchengli wrote: > This function and Decode are both small and the prototype are the same. Move the > code to Decode() and remove this function. Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:204: base::AutoLock auto_lock(lock_); On 2015/04/23 09:35:18, wuchengli wrote: > no need to acquire |lock_| Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:213: On 2015/04/23 09:35:18, wuchengli wrote: > Check the state? Otherwise, all access of |state_| is on the child thread and > won't need a lock. > > if (state_ != kInitialized) > return; Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:6: // that utilizes hardware video decoder present on Intel CPUs. On 2015/04/23 09:35:19, wuchengli wrote: > This is already explained in the class documentation. Remove. Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:11: #include <map> On 2015/04/23 09:35:19, wuchengli wrote: > not used Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:13: #include <utility> On 2015/04/23 09:35:19, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:14: #include <vector> On 2015/04/23 09:35:19, wuchengli wrote: > not used Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:16: #include "base/logging.h" On 2015/04/23 09:35:19, wuchengli wrote: > Move to .cc? Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:18: #include "base/memory/shared_memory.h" On 2015/04/23 09:35:18, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:21: #include "base/synchronization/condition_variable.h" On 2015/04/23 09:35:19, wuchengli wrote: > not used Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:64: // Process one debuging request in the queue. On 2015/04/23 09:35:18, wuchengli wrote: > s/debuging/decode/ Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:70: // Puts contents of |va_surface| into given |video_frame|, releases the On 2015/04/23 09:35:19, wuchengli wrote: > s/Puts/Put/. s/releases/release/. s/passes/pass/ to be consistent with other > comments. Done. Change others instead. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:71: // surface and passes the resulting picture to client for output. On 2015/04/23 09:35:19, wuchengli wrote: > s/passes the resulting picture/passes the buffer id of the resulting picture/ Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:73: int32_t input_id, On 2015/04/23 09:35:19, wuchengli wrote: > s/input_id/input_buffer_id/ to be more descriptive Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:77: enum State { On 2015/04/23 09:35:18, wuchengli wrote: > Use a boolean if you only need two states. Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:88: // An input buffer awaiting consumption, provided by the client. On 2015/04/23 09:35:19, wuchengli wrote: > |video_frame| is the output, so this is not only input. How about "An input > buffer and the corresponding output video frame."? Done. https://codereview.chromium.org/1016773002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:90: DecodeRequest(media::BitstreamBuffer bitstream_buffer, On 2015/04/23 09:35:19, wuchengli wrote: > const & Done. https://codereview.chromium.org/1016773002/diff/140001/media/video/jpeg_decod... File media/video/jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/140001/media/video/jpeg_decod... media/video/jpeg_decode_accelerator.cc:7: #include "base/logging.h" On 2015/04/23 09:35:19, wuchengli wrote: > remove Done. https://codereview.chromium.org/1016773002/diff/140001/media/video/jpeg_decod... File media/video/jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/140001/media/video/jpeg_decod... media/video/jpeg_decode_accelerator.h:20: // keeps the ownership of them. On 2015/04/23 09:35:19, wuchengli wrote: > Document "Initialize and Destroy must be called on the same thread." Done. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:124: // fallback to software decoding) before the former frame. On 2015/05/07 06:23:14, wuchengli wrote: > On 2015/05/07 00:59:03, mcasas wrote: > > Wait, is there still fallback to software decoding? > I discussed with Kuang-che. He'll remove the fallback for now. > > There are all kinds of external cameras. It's possible HW decoder > cannot decode some of them. So we should fallback. Otherwise, those > cameras won't work at all. On the other hand, we want the test > engineers and dogfooders to report bugs when something is broken. > If we fallback even when GPU process crashes, it's hard to know > when a bug happens. Done. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:130: return false; On 2015/05/06 07:59:18, wuchengli wrote: > Can we return a enum from this function? SUCCESS (true), FAILED (rotation != 0), > and FATAL (failed_==true, the client can clear |external_jpeg_decoder_|). Acknowledged. Revised to use jpeg thread. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:135: // Since IsDecoding_Locked() is false here and we only decode a frame at a On 2015/05/04 14:14:41, wuchengli wrote: > This is not right. Suppose DecodeCapturedData is called twice in a row. The > first call reaches line 193 and the second call is here. The second call will > see IsDecoding_Locked==true here. > > Can we move line 121-130 to line 190? So we don't have the above problem and we > don't need to check |failed_| twice. Acknowledged. Revised to use jpeg thread. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/05/04 14:14:41, wuchengli wrote: > I know the current patchset has a bug and the performance data is not final. But > please add what you have to the change description so people know the rough > figure. We need two numbers -- CPU usage and latency. Should we use Broadwell or > Haswell Chromebooks for benchmark? Done. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:48: const DecodeDoneCB& decode_done_cb); On 2015/05/04 14:14:41, wuchengli wrote: > Can GpuJpegDecoder call device_client_->OnIncomingCapturedVideoFrame() and let > OnIncomingCapturedVideoFrame call DoIncomingCapturedVideoFrameOnIOThread > directly if it is on IO thread? > > First, we already pass media::VideoCaptureDevice::Client to the constructor and > it's convenient to use it. Second, passing a VideoCaptureController and > |controller_| is harder to think of the lifecycle. For example, > VideoCaptureController is one layer further and is |controller_| always valid > when GpuJpegDecoder calls it? I found ReserveOutputBuffer can be done outside of DecodeCapturedData, so we don't need to pass device_client into GpuJpegDecoder. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:51: // Creates and intializes decoder in GPU side. Do nothing if already On 2015/05/04 14:14:41, wuchengli wrote: > "Do nothing if already initialized" is strange. The knowledge should be > maintained in the client. Done. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:52: // initialized. Return falses if failed. On 2015/05/04 14:14:41, wuchengli wrote: > s/Return falses/Returns false/ Done. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:79: // The capture thread. On 2015/05/04 14:14:41, wuchengli wrote: > Add documentation what a capture thread is. Without some context, a developer > (like me) does not know what that is. Done. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:89: bool failed_; On 2015/05/06 07:59:18, wuchengli wrote: > On 2015/05/04 14:14:41, wuchengli wrote: > > Add a status callback so you don't need |failed_|. See |status_cb| of > > GpuVideoDecoder::Initialize. The client should decide what to do if decode > > fails. > See the comment in DecodeCapturedData. If we return an enum from > DecodeCapturedData, we don't need the status callback and > video_capture_device_client.cc can clean |external_jpeg_decoder_| if returned > status is FATAL_ERROR. > Acknowledged. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:89: bool failed_; On 2015/05/04 14:14:41, wuchengli wrote: > Add a status callback so you don't need |failed_|. See |status_cb| of > GpuVideoDecoder::Initialize. The client should decide what to do if decode > fails. Done. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:230: external_jpeg_decoder_) On 2015/05/07 00:59:03, mcasas wrote: > With this condition, we would be calling > external_jpeg_decoder_->Initialize() on every > incoming MJPEG frame, is that what we want? This only happens when pixel_format becomes from non-MJPEG to MJPEG. Not every frame. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:230: external_jpeg_decoder_) On 2015/05/07 06:23:14, wuchengli wrote: > On 2015/05/07 00:59:03, mcasas wrote: > > With this condition, we would be calling > > external_jpeg_decoder_->Initialize() on every > > incoming MJPEG frame, is that what we want? > Not every incoming frame. Only when format changes. > > I also asked Kuang-che to clear |external_jpeg_decoder_| when initialization > fails. So it won't try to initialize again next time the format changes. Acknowledged. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:231: ignore_result(external_jpeg_decoder_->Initialize()); On 2015/05/04 14:14:41, wuchengli wrote: > Why not set |external_jpeg_decoder_| to NULL if Initialize fails? GpuJpegDecoder > won't need |initialized_|. Done. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:231: ignore_result(external_jpeg_decoder_->Initialize()); On 2015/05/06 07:59:18, wuchengli wrote: > On 2015/05/04 14:14:41, wuchengli wrote: > > Why not set |external_jpeg_decoder_| to NULL if Initialize fails? > GpuJpegDecoder > > won't need |initialized_|. > As discussed, if Initialize fails, there will be no any callback. So it's safe > to clear |external_jpeg_decoder_| here. Done. https://codereview.chromium.org/1016773002/diff/260001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:238: external_jpeg_decoder_) { On 2015/05/07 00:59:03, mcasas wrote: > nit: Merge all conditions in lines l.237-238. Acknowledged. This is changed to no fallback. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:263: // This function is called from GpuJpegDecoder's capture task thread, which is On 2015/05/06 07:59:18, wuchengli wrote: > s/which is// Acknowledged. Revised to use jpeg thread. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... File content/common/gpu/client/gpu_channel_host.h (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.h:152: On 2015/05/06 07:59:18, wuchengli wrote: > // Creates a JPEG decoder in the GPU process. > Also explain what |reply_loop| is. Acknowledged. Revised to use jpeg thread and no reply_loop. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:81: base::AutoLock lock(channel_lock_); On 2015/05/06 07:59:18, wuchengli wrote: > As discussed, dealing with synchronization in GpuJpegDecodeAcceleratorHost is > complex and annoying. Try adding a Jpeg or media thread. See if it simplifies > both GpuJpegDecodeAcceleratorHost and GpuJpegDecoder. Done. Looks good after using jpeg thread. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:22: #if defined(ARCH_CPU_X86_FAMILY) On 2015/05/07 00:59:03, mcasas wrote: > #if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) Done. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:109: DVLOG(3) << __func__; On 2015/05/07 00:59:03, mcasas wrote: > As we approach something stable you should remove > these __func__ printouts. Done. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:129: return false; On 2015/05/07 00:59:03, mcasas wrote: > Move these |return false| further up in the method, > i.e. why AddRoute() and AddFilter() if the platform > does not support jpeg_decode_accelerator_ anyway? Done. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:152: // function is to just keep reference of |shm| to make sure it live util On 2015/05/07 00:59:03, mcasas wrote: > s/live/lives/ Done. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:153: // decode finished. On 2015/05/07 00:59:03, mcasas wrote: > s/finished/finishes/ Done. https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/260001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:72: // The message filter to run JDA::Decode on IO thread. On 2015/05/07 00:59:03, mcasas wrote: > No abbreviations please. > (Rationale: sometimes we want to grep/sed for > JpegDecodeAccelerator and would like all this lines > to show up). Done. https://codereview.chromium.org/1016773002/diff/260001/media/video/capture/vi... File media/video/capture/video_capture_device.h (right): https://codereview.chromium.org/1016773002/diff/260001/media/video/capture/vi... media/video/capture/video_capture_device.h:206: virtual base::SharedMemoryHandle AsPlatformHandle() = 0; On 2015/05/07 00:59:03, mcasas wrote: > Can you use base::PlatformFile instead of base::SharedMemoryHandle. > > The implementation would be something like: > base::PlatformFile AsPlatformFile() override { > #if defined(OS_POSIX) > return shared_memory_.handle().fd; > #else > return shared_memory_.handle(); > #endif > } > > The point being that, in the immediate future, not all Buffers > will be memory backed, some will be Ozone Native Buffers, > and those have no SharedMemoryHandle. See [1]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... Done. I'm not sure my GpuMemoryBufferBufferHandle::AsPlatformHandle() is correct or not. Please take a look. https://codereview.chromium.org/1016773002/diff/260001/media/video/jpeg_decod... File media/video/jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/260001/media/video/jpeg_decod... media/video/jpeg_decode_accelerator.cc:18: void DefaultDeleter<media::JpegDecodeAccelerator>::operator()( On 2015/05/07 00:59:03, mcasas wrote: > Do we still need this generic DefaultDeleter or > should we specialize it for VaapiJpegDecodeAccelerator > instead? media::JpegDecodeAccelerator::Destroy is modeled after media::VideoDecodeAccelerator::Destroy and I want to keep them consistent.
https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:178: error_cb_.Run(base::StringPrintf("CreateAndMapAnonymous failed, size=%ld", There is a warning in arm platform. Please change %ld to %d.
wuchengli@chromium.org changed reviewers: + piman@chromium.org, xhwang@chromium.org
Adding JPEG thread simplifies the code a lot! This CL is ready for owner review. piman: owner review for content/ xhwang: owner review for media/ mcasas: PTAL Thanks. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:44: DCHECK(CalledOnValidThread()); We need a GpuJpegDecoder::Destroy to stop jpeg thread and destroy |decoder_|. So we don't access the member variables from VideoFrameReady when |this| is being destructed. GpuJpegDecoder::Destroy should be called in ~VideoCaptureDeviceClient(). VideoCaptureDeviceClient owns GpuJpegDecoder. So the lifecycle should be the same. When VideoCaptureDeviceClient is gone, GpuJpegDecoder shouldn't be alive. We don't need to use WeakPtr for VideoCaptureDeviceClient. Using Unretained is enough. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:48: DCHECK(CalledOnValidThread()); DCHECK(jpeg_thread_proxy_->BelongsToCurrentThread()); https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:94: media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) When will this happen? If this is an error, remove this and just use line 102. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:34: // All public methods, if not specified, are called on the same thread. s/are/should/. All public methods except JpegDecodeAccelerator::Client ones should be called on the same thread. They are trampolined to an internal JPEG thread. JpegDecodeAccelerator::Client methods also run on the JPEG thread. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:74: void CreateDecoderOnJpegThread(); s/CreateDecoderOnJpegThread/InitializeOnJpegThread/? CreateDecoderOnJpegThread also initializes the decoder. It's more consistent with DecodeCapturedDataOnJpegThread. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:85: base::WaitableEvent decode_event_; |create_event_| is only used once. Let's combine these two. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:90: // The callback when decode succeed. s/succeed/succeeds/. s/callback/callback to run/ https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:93: // The callback when error. s/callback/callback to run/. s/error/an error occurs/. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:352: external_jpeg_decoder_) { && !flip https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.h:77: // Whether initialized |external_jpeg_decoder_|. Whether |external_jpeg_decoder_| has been initialized. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.h:78: bool initialized_external_jpeg_decoder_; external_jpeg_decoder_initialized_ https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:9: #include "base/metrics/histogram.h" not used? https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:15: #include "media/base/bind_to_current_loop.h" not used? https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:18: #include "media/video/picture.h" not used? https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:20: static void ReportVaapiError() { Add a TODO to add UMA https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:44: Cleanup(); Move this after line 50. Otherwise, Cleanup will reset WeakPtrFactory and NotifyError won't be callbed. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:106: << " into video_frame associate to input buffer id " s/associate to/associated with/ https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:133: if (client_) |client_| can only be dereferenced on the child thread. Post a task to do this in the child thread. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:178: // crosbug.com/p/39584 Remove the leak comment because it's being fixed. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:192: LOG(ERROR) << "Decode failed"; Decode JPEG failed https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:200: LOG(ERROR) << "Output failed"; Output picture failed https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:205: } while (0); There's no code after while. Remove do/while and replace break with return. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:215: if (!initialized_) Move auto_lock before this. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:65: enum State { remove https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:98: // NOTE: all calls to these objects *MUST* be executed on message_loop_. s/message_loop_/|message_loop_|/ https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:113: // vaapi_wrapper_ is destroyed. s/vaapi_wrapper_/|vaapi_wrapper_|/ https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:122: gfx::Size last_coded_size_; Add variable comment for these two variables.
This is a very large CL. Can you split it in 2 parts, one that adds the GpuJpegDecodeAccelerator infrastructure (content/common/gpu process parts) and another part that makes use of it for the capture code (the content/browser/renderer_host and I guess media/video/capture parts) ?
On 2015/05/13 22:59:28, piman (Very slow to review) wrote: > This is a very large CL. > > Can you split it in 2 parts, one that adds the GpuJpegDecodeAccelerator > infrastructure (content/common/gpu process parts) and another part that makes > use of it for the capture code (the content/browser/renderer_host and I guess > media/video/capture parts) ? Kuang-che is OOO. He'll split the CL next week.
This CL is split to three smaller ones. https://codereview.chromium.org/1147693002/ https://codereview.chromium.org/1132683004/ https://codereview.chromium.org/1124423008/ This one will be closed. https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/160001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:155: Send(new AcceleratedJpegDecoderHostMsg_VideoFrameReady(host_route_id_, On 2015/04/24 05:54:24, wuchengli wrote: > I remember now. ChannelProxy::Send requires to be called on the child thread. > That's why a filter is used in GpuVideoDecodeAccelerator to send IPC on IO > thread. > > See DCHECK in line 454. > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_pr... > > Same for Send in NotifyError if it is called on decode thread. Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:48: DCHECK(CalledOnValidThread()); On 2015/05/13 15:04:05, wuchengli wrote: > DCHECK(jpeg_thread_proxy_->BelongsToCurrentThread()); Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:94: media::JpegDecodeAccelerator::kInvalidBitstreamBufferId) On 2015/05/13 15:04:05, wuchengli wrote: > When will this happen? If this is an error, remove this and just use line 102. Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:178: error_cb_.Run(base::StringPrintf("CreateAndMapAnonymous failed, size=%ld", On 2015/05/12 09:42:03, henryhsu wrote: > There is a warning in arm platform. Please change %ld to %d. Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:34: // All public methods, if not specified, are called on the same thread. On 2015/05/13 15:04:06, wuchengli wrote: > s/are/should/. > > All public methods except JpegDecodeAccelerator::Client ones should be called > on the same thread. They are trampolined to an internal JPEG thread. > JpegDecodeAccelerator::Client methods also run on the JPEG thread. Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:74: void CreateDecoderOnJpegThread(); On 2015/05/13 15:04:05, wuchengli wrote: > s/CreateDecoderOnJpegThread/InitializeOnJpegThread/? CreateDecoderOnJpegThread > also initializes the decoder. It's more consistent with > DecodeCapturedDataOnJpegThread. Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:85: base::WaitableEvent decode_event_; On 2015/05/13 15:04:06, wuchengli wrote: > |create_event_| is only used once. Let's combine these two. Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:90: // The callback when decode succeed. On 2015/05/13 15:04:05, wuchengli wrote: > s/succeed/succeeds/. s/callback/callback to run/ Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:93: // The callback when error. On 2015/05/13 15:04:05, wuchengli wrote: > s/callback/callback to run/. s/error/an error occurs/. Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:352: external_jpeg_decoder_) { On 2015/05/13 15:04:06, wuchengli wrote: > && !flip Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.h:77: // Whether initialized |external_jpeg_decoder_|. On 2015/05/13 15:04:06, wuchengli wrote: > Whether |external_jpeg_decoder_| has been initialized. Done. https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.h:78: bool initialized_external_jpeg_decoder_; On 2015/05/13 15:04:06, wuchengli wrote: > external_jpeg_decoder_initialized_ Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:9: #include "base/metrics/histogram.h" On 2015/05/13 15:04:07, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:15: #include "media/base/bind_to_current_loop.h" On 2015/05/13 15:04:07, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:18: #include "media/video/picture.h" On 2015/05/13 15:04:07, wuchengli wrote: > not used? Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:20: static void ReportVaapiError() { On 2015/05/13 15:04:06, wuchengli wrote: > Add a TODO to add UMA Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:44: Cleanup(); On 2015/05/13 15:04:06, wuchengli wrote: > Move this after line 50. Otherwise, Cleanup will reset WeakPtrFactory and > NotifyError won't be callbed. Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:106: << " into video_frame associate to input buffer id " On 2015/05/13 15:04:06, wuchengli wrote: > s/associate to/associated with/ Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:133: if (client_) On 2015/05/13 15:04:06, wuchengli wrote: > |client_| can only be dereferenced on the child thread. Post a task to do this > in the child thread. Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:178: // crosbug.com/p/39584 On 2015/05/13 15:04:07, wuchengli wrote: > Remove the leak comment because it's being fixed. Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:192: LOG(ERROR) << "Decode failed"; On 2015/05/13 15:04:07, wuchengli wrote: > Decode JPEG failed Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:200: LOG(ERROR) << "Output failed"; On 2015/05/13 15:04:06, wuchengli wrote: > Output picture failed Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:205: } while (0); On 2015/05/13 15:04:06, wuchengli wrote: > There's no code after while. Remove do/while and replace break with return. Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:215: if (!initialized_) On 2015/05/13 15:04:06, wuchengli wrote: > Move auto_lock before this. Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:65: enum State { On 2015/05/13 15:04:07, wuchengli wrote: > remove Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:98: // NOTE: all calls to these objects *MUST* be executed on message_loop_. On 2015/05/13 15:04:07, wuchengli wrote: > s/message_loop_/|message_loop_|/ Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:113: // vaapi_wrapper_ is destroyed. On 2015/05/13 15:04:07, wuchengli wrote: > s/vaapi_wrapper_/|vaapi_wrapper_|/ Done. https://codereview.chromium.org/1016773002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:122: gfx::Size last_coded_size_; On 2015/05/13 15:04:07, wuchengli wrote: > Add variable comment for these two variables. Done.
https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1016773002/diff/300001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:44: DCHECK(CalledOnValidThread()); On 2015/05/13 15:04:05, wuchengli wrote: > We need a GpuJpegDecoder::Destroy to stop jpeg thread and destroy |decoder_|. So > we don't access the member variables from VideoFrameReady when |this| is being > destructed. GpuJpegDecoder::Destroy should be called in > ~VideoCaptureDeviceClient(). > > VideoCaptureDeviceClient owns GpuJpegDecoder. So the lifecycle should be the > same. When VideoCaptureDeviceClient is gone, GpuJpegDecoder shouldn't be alive. > We don't need to use WeakPtr for VideoCaptureDeviceClient. Using Unretained is > enough. Acknowledged. piman raised the same issue in https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... I will address it there. |
