Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1124423008:
MJPEG acceleration for video capture using VAAPI, the GPU and IPC part (Closed)
|
Created:
5 years, 7 months ago by kcwu Modified:
5 years, 1 month ago Reviewers:
jschuh, piman, wuchengli, nasko, palmer, jln (very slow on Chromium), xhwang, Pawel Osciak 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, henryhsu, mcasas Base URL:
https://chromium.googlesource.com/chromium/src.git@mjpeg-1-media Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMJPEG acceleration for video capture using VAAPI, the GPU and IPC part
This is the GPU and IPC part of the acceleration. The acceleration is
not enabled yet with this CL.
Once enabled (together with other CLs), the performance numbers to
decode 1280x720 images:
Latency (percentile) 50% 99% max
software decode (libjpeg_turbo): 7.1ms 7.6ms 12.9ms
hardware decode (VAAPI): 4.5ms 5.0ms 15.7ms
CPU usage user sys total
software decode (libjpeg_turbo): 19.3% 2.1% 21.4%
hardware decode (VAAPI): 14.0% 4.7% 18.7%
Tested on peppy chromebook (haswell) using apprtc.appspot.com
(not connected yet and no loopback)
CPU scaling governor set to "performance".
"latency" means time from DQBUF to DoIncomingCapturedVideoFrameOnIOThread.
"CPU" is whole chrome usage, measured by mpstat.
BUG=335778
TEST=none
Committed: https://crrev.com/2bdb8a4d9f21dd1effcff644f182dff8eba179a3
Cr-Commit-Position: refs/heads/master@{#335008}
Patch Set 1 #
Total comments: 49
Patch Set 2 : #Patch Set 3 : gpu_jpeg_decode_accelerator.cc filter to dispatch decode task #
Total comments: 15
Patch Set 4 : #
Total comments: 68
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 29
Patch Set 8 : #Patch Set 9 : rebase due to VideoFrame refactoring #Patch Set 10 : #
Total comments: 10
Patch Set 11 : split out host part #
Total comments: 20
Patch Set 12 : rebase only #Patch Set 13 : #
Total comments: 9
Patch Set 14 : validate params #Patch Set 15 : fix compile #Patch Set 16 : fix compile for windows & gn #Patch Set 17 : rebase, fix nits #Patch Set 18 : add back jpeg_decode_accelerator.cc #Messages
Total messages: 84 (18 generated)
kcwu@chromium.org changed reviewers: + piman@chromium.org, posciak@chromium.org, wuchengli@chromium.org
@piman, please take a look. This is split from https://codereview.chromium.org/1016773002/ . This patch contains only GPU and IPC part. The last piece (to actually enable) is in https://codereview.chromium.org/1132683004 .
kcwu@chromium.org changed reviewers: + jln@chromium.org, xhwang@chromium.org
xhwang: owner review for media/ jln: security review for content/common/gpu/gpu_messages.h
media/ lgtm
https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:47: channel_ = NULL; nit: nullptr https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:62: return false; So, if I understand correctly, the client is responsible for checking for failure, and in that case call delete instead of Destroy. It seems error-prone (and is not documented). Could we instead make GpuChannelHost::CreateJpegDecoder also call Initialize, and be responsible for destroying GpuJpegDecodeAcceleratorHost in case of failure? That way also you wouldn't need this to reach into the channel_ to call RemoveRoute (which is inconsistent with where AddRoute is called) - that could be done inside GpuChannelHost::CreateJpegDecoder. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:87: OnNotifyError(bitstream_buffer.id(), INVALID_ARGUMENT); So, IIUC, on Windows, ShareToGpuProcess actually creates a handle in the GPU process. If we early return here, we would leak it. So, could we either: - check handle validity ahead of time before duplicating any handle or - simply DCHECK that they're valid? Is there a use case for passing an invalid/NULL handle here? https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:122: delete this; I'm not in love with this pattern, because it prevents users from using scoped_ptr - since they have to call Destroy instead of delete. Could we instead move all this logic into the destructor? https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h:25: // |channel_|.) I'm confused by this comment. Comments for channel_ mention it can be NULL, which would suggest |this| could outlive |channel_|. More generally, what guarantees that GpuJpegDecodeAcceleratorHost will be Destroy'ed before the channel is? In particular after a lost channel (e.g. GPU process dying), the GpuChannelHost could get destroyed as a side effect of context recreation - see RenderThreadImpl::EstablishGpuChannelSync / BrowserGpuChannelHostFactory::EstablishGpuChannel, and this might happen before this class had a chance to get the OnChannelError notification. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_chan... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_chan... content/common/gpu/gpu_channel.cc:797: jpeg_decoder_map_.set(route_id, decoder.Pass()); You don't want to do this when Initialize fails. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_chan... content/common/gpu/gpu_channel.cc:804: ignore_result(decoder.release()); Yeah, this is another reason the Destroy pattern is super confusing. If you kept it you should use a regular hash_map, not a ScopedPtrHashMap, since you never actually use the automatic destruction property. But I think it should be revisited. Instead you can keep the ScopedPtrHashMap and have GpuJpegDecodeAccelerator::OnDestroy call GpuChannel::ReleaseJpegDecoder directly, and move the rest of the Destroy code either here (e.g. RemoveRoute) or into the GpuJpegDecodeAccelerator destructor. No need for custom deleters. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_mess... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_mess... content/common/gpu/gpu_messages.h:800: media::JpegDecodeAccelerator::Error) /* error */ Can these 2 messages be combined into 1? This is really a single AcceleratedJpegDecoderMsg_DecodeAck, where you could signal either success or failure. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:37: class GpuJpegDecodeAccelerator::MessageFilter : public IPC::MessageFilter { I would like to avoid having a separate filter per decoder. Every filter has an extra cost on every single IPC message, maybe small but adds up (we fixed various performance issues relative to that). One per channel seems more reasonable. Instead you could have the filter maintain the routing list, handling all AcceleratedJpegDecoderMsg and GpuMsg_CreateJpegDecoder messages directly on the IO thread, and forwarding as appropriate? This would also avoid the need for the Wait in GpuJpegDecodeAccelerator::Destroy. When the main thread receives the OnDestroy, it would already have been removed from the map in the filter, ensuring no further call is made on it from the IO thread. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:111: // update as well. nit: indent comments https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:120: if (!channel_->AddRoute(host_route_id_, this)) { Actually, if the routing is done by the filter, you could skip this (provided the filter ensures the GpuJpegDecodeAccelerator are correctly destroyed as the filter is removed). https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:151: void GpuJpegDecodeAccelerator::OnDecode( This is called from the IO thread. Can you name it OnDecodeOnIO ? https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:32: if (task_runner_ != base::ThreadTaskRunnerHandle::Get()) { I don't like this pattern very much. The caller knows which thread it's on, i.e. whether it needs to post a task or not. Instead can you have a NotifyError that runs exclusively on the main thread, and a helper function e.g. NotifyErrorFromDecoderThread that unconditionally posts a task? https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:40: Cleanup(); Is it needed to do this here? It'll be done at Destroy() time. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:42: LOG(ERROR) << "Notifying of error " << error; Is this needed in release builds? It seems like every broken jpeg on the web would trigger this. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:54: decoder_thread_("VaapiJpegDecoderThread"), Can we share a single thread among all decoders? Because of the global VAAPI lock, you're probably not getting any scaling across threads anyway. Instead you can have a global thread with a refcount. The first VaapiJpegDecodeAccelerator creates the thread and each further VaapiJpegDecodeAccelerator would reuse the same thread, keeping a count. When the last one finishes, it would stop and join the thread. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:68: client_ = client_ptr_factory_->GetWeakPtr(); Why is a WeakPtr needed here? It seems like the client (GpuJpegDecodeAccelerator) always outlives |this|, since it owns it. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:83: LOG(ERROR) << "Failed to start decoding thread."; DLOG https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:114: void* mem; nit: = nullptr; https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:173: // This is not only optimization, but also to avoid libva resoruce leak. typo: s/resoruce/resource/ https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:211: if (!initialized_) This is used outside the lock. Even with a lock, I'm not convinced you don't have races. For example, initialized_ could be true here, but a task could be pending on the main thread to Cleanup, which will race against the DecodeTask on the decoder thread, so what is the point of checking initialized_?
https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:47: channel_ = NULL; On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > nit: nullptr Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:62: return false; On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > So, if I understand correctly, the client is responsible for checking for > failure, and in that case call delete instead of Destroy. It seems error-prone > (and is not documented). > > Could we instead make GpuChannelHost::CreateJpegDecoder also call Initialize, > and be responsible for destroying GpuJpegDecodeAcceleratorHost in case of > failure? That way also you wouldn't need this to reach into the channel_ to call > RemoveRoute (which is inconsistent with where AddRoute is called) - that could > be done inside GpuChannelHost::CreateJpegDecoder. Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:87: OnNotifyError(bitstream_buffer.id(), INVALID_ARGUMENT); On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > So, IIUC, on Windows, ShareToGpuProcess actually creates a handle in the GPU > process. If we early return here, we would leak it. > > So, could we either: > - check handle validity ahead of time before duplicating any handle or > - simply DCHECK that they're valid? Is there a use case for passing an > invalid/NULL handle here? Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:122: delete this; On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > I'm not in love with this pattern, because it prevents users from using > scoped_ptr - since they have to call Destroy instead of delete. > > Could we instead move all this logic into the destructor? This class implements media::JpegDecodeAccelerator interface, which modeled after media::VideoDecodeAccelerator. I would prefer that JpegDecodeAccelerator and VideoDecodeAccelerator are consistent in this case. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_mess... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_mess... content/common/gpu/gpu_messages.h:800: media::JpegDecodeAccelerator::Error) /* error */ On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > Can these 2 messages be combined into 1? This is really a single > AcceleratedJpegDecoderMsg_DecodeAck, where you could signal either success or > failure. It could be. However I prefer keep it aligned with media::JpegDecodeAccelerator interface, which distinguish success and failure cases. And semantically bitstream_buffer_id is always valid for VideoFrameReady while bitstream_buffer_id may be invalid for NotifyError. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:37: class GpuJpegDecodeAccelerator::MessageFilter : public IPC::MessageFilter { On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > I would like to avoid having a separate filter per decoder. Every filter has an > extra cost on every single IPC message, maybe small but adds up (we fixed > various performance issues relative to that). One per channel seems more > reasonable. > > Instead you could have the filter maintain the routing list, handling all > AcceleratedJpegDecoderMsg and GpuMsg_CreateJpegDecoder messages directly on the > IO thread, and forwarding as appropriate? > > This would also avoid the need for the Wait in > GpuJpegDecodeAccelerator::Destroy. When the main thread receives the OnDestroy, > it would already have been removed from the map in the filter, ensuring no > further call is made on it from the IO thread. I'm trying to implement your suggestion but not yet finished. And I don't understand why Wait in GpuJpegDecodeAccelerator::Destroy could be avoided. The problem is VaapiJpegDecodeAccelerator::Initialize (vaInitialize is slow) and VaapiJPegDecodeAccelerator::Destroy (waiting for decode thead stop) should not be called on the IO thread. Thus OnDestroy on main thread still need kind of wait for the filter in order to make sure VaapiJPegDecodeAccelerator::Destroy is called while VaapiJPegDecodeAccelerator::Decode is not running on the IO thread.
(haven't addressed all comments yet)
https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:122: delete this; On 2015/05/22 19:46:50, kcwu wrote: > On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > > I'm not in love with this pattern, because it prevents users from using > > scoped_ptr - since they have to call Destroy instead of delete. > > > > Could we instead move all this logic into the destructor? > > This class implements media::JpegDecodeAccelerator interface, which modeled > after media::VideoDecodeAccelerator. > I would prefer that JpegDecodeAccelerator and VideoDecodeAccelerator are > consistent in this case. Honestly, I don't think VideoDecodeAccelerator is a good pattern to follow. It's confusing, it prevents the use of standard helper classes, including STL ones once we have c++11. It's not consistent with the rest of Chrome. Makes everything more complicated. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_mess... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_mess... content/common/gpu/gpu_messages.h:800: media::JpegDecodeAccelerator::Error) /* error */ On 2015/05/22 19:46:51, kcwu wrote: > On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > > Can these 2 messages be combined into 1? This is really a single > > AcceleratedJpegDecoderMsg_DecodeAck, where you could signal either success or > > failure. > > It could be. However I prefer keep it aligned with media::JpegDecodeAccelerator > interface, which distinguish success and failure cases. And semantically > bitstream_buffer_id is always valid for VideoFrameReady while > bitstream_buffer_id may be invalid for NotifyError. I would prefer to keep a single message, it's simpler to audit and maintain. Semantics are very clear - for every AcceleratedJpegDecoderMsg_Decode there must be exactly one AcceleratedJpegDecoderMsg_DecodeAck, instead of the status quo which isn't clear if you should expect a AcceleratedJpegDecoderHostMsg_VideoFrameReady or not after a AcceleratedJpegDecoderHostMsg_NotifyError (think overlapping decodes). https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:37: class GpuJpegDecodeAccelerator::MessageFilter : public IPC::MessageFilter { On 2015/05/22 19:46:51, kcwu wrote: > On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > > I would like to avoid having a separate filter per decoder. Every filter has > an > > extra cost on every single IPC message, maybe small but adds up (we fixed > > various performance issues relative to that). One per channel seems more > > reasonable. > > > > Instead you could have the filter maintain the routing list, handling all > > AcceleratedJpegDecoderMsg and GpuMsg_CreateJpegDecoder messages directly on > the > > IO thread, and forwarding as appropriate? > > > > This would also avoid the need for the Wait in > > GpuJpegDecodeAccelerator::Destroy. When the main thread receives the > OnDestroy, > > it would already have been removed from the map in the filter, ensuring no > > further call is made on it from the IO thread. > > I'm trying to implement your suggestion but not yet finished. And I don't > understand why Wait in GpuJpegDecodeAccelerator::Destroy could be avoided. > > The problem is VaapiJpegDecodeAccelerator::Initialize (vaInitialize is slow) and > VaapiJPegDecodeAccelerator::Destroy (waiting for decode thead stop) should not > be called on the IO thread. Agreed on that. > Thus OnDestroy on main thread still need kind of > wait for the filter in order to make sure VaapiJPegDecodeAccelerator::Destroy is > called while VaapiJPegDecodeAccelerator::Decode is not running on the IO thread. The filter can ensure ordering. Once it receives the AcceleratedJpegDecoderMsg_Destroy it removes the routing from the map, ensuring no further Decode would happen, then posts to the main thread. The main thread task is guaranteed to happen after all Decodes, and you can safely destroy the VaapiJpegDecodeAccelerator then.
gpu_jpeg_decode_accelerator.cc revised. PTAL. thanks. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:122: delete this; On 2015/05/22 22:27:48, piman (Very slow to review) wrote: > On 2015/05/22 19:46:50, kcwu wrote: > > On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > > > I'm not in love with this pattern, because it prevents users from using > > > scoped_ptr - since they have to call Destroy instead of delete. > > > > > > Could we instead move all this logic into the destructor? > > > > This class implements media::JpegDecodeAccelerator interface, which modeled > > after media::VideoDecodeAccelerator. > > I would prefer that JpegDecodeAccelerator and VideoDecodeAccelerator are > > consistent in this case. > > Honestly, I don't think VideoDecodeAccelerator is a good pattern to follow. It's > confusing, it prevents the use of standard helper classes, including STL ones > once we have c++11. It's not consistent with the rest of Chrome. Makes > everything more complicated. Done. Destroy methods are merged into destructors. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_chan... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_chan... content/common/gpu/gpu_channel.cc:797: jpeg_decoder_map_.set(route_id, decoder.Pass()); On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > You don't want to do this when Initialize fails. Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_chan... content/common/gpu/gpu_channel.cc:797: jpeg_decoder_map_.set(route_id, decoder.Pass()); On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > You don't want to do this when Initialize fails. Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_chan... content/common/gpu/gpu_channel.cc:804: ignore_result(decoder.release()); On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > Yeah, this is another reason the Destroy pattern is super confusing. > > If you kept it you should use a regular hash_map, not a ScopedPtrHashMap, since > you never actually use the automatic destruction property. But I think it should > be revisited. > > Instead you can keep the ScopedPtrHashMap and have > GpuJpegDecodeAccelerator::OnDestroy call GpuChannel::ReleaseJpegDecoder > directly, and move the rest of the Destroy code either here (e.g. RemoveRoute) > or into the GpuJpegDecodeAccelerator destructor. No need for custom deleters. Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_mess... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/gpu_mess... content/common/gpu/gpu_messages.h:800: media::JpegDecodeAccelerator::Error) /* error */ On 2015/05/22 22:27:48, piman (Very slow to review) wrote: > On 2015/05/22 19:46:51, kcwu wrote: > > On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > > > Can these 2 messages be combined into 1? This is really a single > > > AcceleratedJpegDecoderMsg_DecodeAck, where you could signal either success > or > > > failure. > > > > It could be. However I prefer keep it aligned with > media::JpegDecodeAccelerator > > interface, which distinguish success and failure cases. And semantically > > bitstream_buffer_id is always valid for VideoFrameReady while > > bitstream_buffer_id may be invalid for NotifyError. > > I would prefer to keep a single message, it's simpler to audit and maintain. > Semantics are very clear - for every AcceleratedJpegDecoderMsg_Decode there must > be exactly one AcceleratedJpegDecoderMsg_DecodeAck, instead of the status quo > which isn't clear if you should expect a > AcceleratedJpegDecoderHostMsg_VideoFrameReady or not after a > AcceleratedJpegDecoderHostMsg_NotifyError (think overlapping decodes). Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:37: class GpuJpegDecodeAccelerator::MessageFilter : public IPC::MessageFilter { On 2015/05/22 22:27:48, piman (Very slow to review) wrote: > On 2015/05/22 19:46:51, kcwu wrote: > > On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > > > I would like to avoid having a separate filter per decoder. Every filter has > > an > > > extra cost on every single IPC message, maybe small but adds up (we fixed > > > various performance issues relative to that). One per channel seems more > > > reasonable. > > > > > > Instead you could have the filter maintain the routing list, handling all > > > AcceleratedJpegDecoderMsg and GpuMsg_CreateJpegDecoder messages directly on > > the > > > IO thread, and forwarding as appropriate? > > > > > > This would also avoid the need for the Wait in > > > GpuJpegDecodeAccelerator::Destroy. When the main thread receives the > > OnDestroy, > > > it would already have been removed from the map in the filter, ensuring no > > > further call is made on it from the IO thread. > > > > I'm trying to implement your suggestion but not yet finished. And I don't > > understand why Wait in GpuJpegDecodeAccelerator::Destroy could be avoided. > > > > The problem is VaapiJpegDecodeAccelerator::Initialize (vaInitialize is slow) > and > > VaapiJPegDecodeAccelerator::Destroy (waiting for decode thead stop) should not > > be called on the IO thread. > > Agreed on that. > > > Thus OnDestroy on main thread still need kind of > > wait for the filter in order to make sure VaapiJPegDecodeAccelerator::Destroy > is > > called while VaapiJPegDecodeAccelerator::Decode is not running on the IO > thread. > > The filter can ensure ordering. Once it receives the > AcceleratedJpegDecoderMsg_Destroy it removes the routing from the map, ensuring > no further Decode would happen, then posts to the main thread. The main thread > task is guaranteed to happen after all Decodes, and you can safely destroy the > VaapiJpegDecodeAccelerator then. Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:111: // update as well. On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > nit: indent comments This was indented by 'git cl format'. Would you like to follow it? https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:120: if (!channel_->AddRoute(host_route_id_, this)) { On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > Actually, if the routing is done by the filter, you could skip this (provided > the filter ensures the GpuJpegDecodeAccelerator are correctly destroyed as the > filter is removed). Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:151: void GpuJpegDecodeAccelerator::OnDecode( On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > This is called from the IO thread. Can you name it OnDecodeOnIO ? Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:32: if (task_runner_ != base::ThreadTaskRunnerHandle::Get()) { On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > I don't like this pattern very much. The caller knows which thread it's on, i.e. > whether it needs to post a task or not. > > Instead can you have a NotifyError that runs exclusively on the main thread, and > a helper function e.g. NotifyErrorFromDecoderThread that unconditionally posts a > task? Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:40: Cleanup(); On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > Is it needed to do this here? It'll be done at Destroy() time. Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:42: LOG(ERROR) << "Notifying of error " << error; On 2015/05/18 22:46:20, piman (Very slow to review) wrote: > Is this needed in release builds? It seems like every broken jpeg on the web > would trigger this. Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:68: client_ = client_ptr_factory_->GetWeakPtr(); On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > Why is a WeakPtr needed here? It seems like the client > (GpuJpegDecodeAccelerator) always outlives |this|, since it owns it. Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:83: LOG(ERROR) << "Failed to start decoding thread."; On 2015/05/18 22:46:20, piman (Very slow to review) wrote: > DLOG Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:114: void* mem; On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > nit: = nullptr; Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:173: // This is not only optimization, but also to avoid libva resoruce leak. On 2015/05/18 22:46:20, piman (Very slow to review) wrote: > typo: s/resoruce/resource/ Done. https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:211: if (!initialized_) On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > This is used outside the lock. > > Even with a lock, I'm not convinced you don't have races. For example, > initialized_ could be true here, but a task could be pending on the main thread > to Cleanup, which will race against the DecodeTask on the decoder thread, so > what is the point of checking initialized_? I will add a checking in DecodeTask. The check here could save a PostTask.
https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:54: decoder_thread_("VaapiJpegDecoderThread"), On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > Can we share a single thread among all decoders? > Because of the global VAAPI lock, you're probably not getting any scaling across > threads anyway. > Instead you can have a global thread with a refcount. The first > VaapiJpegDecodeAccelerator creates the thread and each further > VaapiJpegDecodeAccelerator would reuse the same thread, keeping a count. When > the last one finishes, it would stop and join the thread. This only happens if both external and internal cameras are in use, which is unlikely. And I feel this (passing thread from GpuJpegDecodeAccelerator) exposed implementation detail of each accelerator to GpuJpegDecodeAccelerator. Since I cannot think of any reasonable use cases (use both cameras at the same time), I would prefer not to optimize for them now.
On Tue, May 26, 2015 at 7:51 AM, <kcwu@chromium.org> wrote: > > > https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... > File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): > > > https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va... > content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:54: > decoder_thread_("VaapiJpegDecoderThread"), > On 2015/05/18 22:46:19, piman (Very slow to review) wrote: > >> Can we share a single thread among all decoders? >> Because of the global VAAPI lock, you're probably not getting any >> > scaling across > >> threads anyway. >> Instead you can have a global thread with a refcount. The first >> VaapiJpegDecodeAccelerator creates the thread and each further >> VaapiJpegDecodeAccelerator would reuse the same thread, keeping a >> > count. When > >> the last one finishes, it would stop and join the thread. >> > > This only happens if both external and internal cameras are in use, > which is unlikely. And I feel this (passing thread from > GpuJpegDecodeAccelerator) exposed implementation detail of each > accelerator to GpuJpegDecodeAccelerator. > > Since I cannot think of any reasonable use cases (use both cameras at > the same time), I would prefer not to optimize for them now. > I'm not sure this has much to do with cameras - even though this is the only current user, this seems useful for other things, e.g. renderer decoding jpegs. At which point we don't want to burn one thread for each client. > > https://codereview.chromium.org/1124423008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/clie... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:83: DCHECK(base::SharedMemory::IsHandleValid(input_handle)); ShareToGpuProcess can legitimately fail, at least if the GPU channel is lost. https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:805: #endif nit: remove https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.h (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.h:175: #endif nit: remove. https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:66: MessageFilter(GpuJpegDecodeAccelerator* owner) : owner_(owner) {} nit: explicit. https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:115: base::Unretained(owner_), *route_id)); The GpuJpegDecodeAccelerator could be destroyed before the filter, so dereferencing owner_ can be racy (and using Unretained unsafe). If that happens, sending notification messages/removing clients is unneeded though. So what you can do is: 1- grab the task runners in the constructor, and keep a direct reference to them. Use them to post the task / check the threads. 2- keep a WeakPtr to the GpuJpegDecodeAccelerator. You can't dereference the weak ptr on the IO thread, but it's valid to post a task with it. The task will be cancelled on the target thread if the weak ptr is NULL (which is safe). Actually, I think you also need to keep ownership of GpuJpegDecodeAccelerator::Client on this thread (otherwise there is a race there too). The advantage is that you don't necessarily need to keep a map in the GpuJpegDecodeAccelerator (you can just keep a client count to remove the filter). You probably do need to post a task to the main thread to destroy the accelerator though. https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:64: base::Lock lock_; I think you can remove this lock altogether, if you pass the DecodeRequest explicitly in the PostTask, instead of keeping it in a queue. initialized_ is set before Decode is ever called, so it's safe to read without lock.
I will continue remain work tomorrow. https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/clie... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/clie... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:83: DCHECK(base::SharedMemory::IsHandleValid(input_handle)); On 2015/05/26 23:31:52, piman (Very slow to review) wrote: > ShareToGpuProcess can legitimately fail, at least if the GPU channel is lost. Done. https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.cc (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.cc:805: #endif On 2015/05/26 23:31:53, piman (Very slow to review) wrote: > nit: remove Done. https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel.h (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/gpu_... content/common/gpu/gpu_channel.h:175: #endif On 2015/05/26 23:31:53, piman (Very slow to review) wrote: > nit: remove. Done. https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:66: MessageFilter(GpuJpegDecodeAccelerator* owner) : owner_(owner) {} On 2015/05/26 23:31:53, piman (Very slow to review) wrote: > nit: explicit. Done. https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:115: base::Unretained(owner_), *route_id)); On 2015/05/26 23:31:53, piman (Very slow to review) wrote: > The GpuJpegDecodeAccelerator could be destroyed before the filter, so > dereferencing owner_ can be racy (and using Unretained unsafe). If that happens, > sending notification messages/removing clients is unneeded though. > > So what you can do is: > 1- grab the task runners in the constructor, and keep a direct reference to > them. Use them to post the task / check the threads. > 2- keep a WeakPtr to the GpuJpegDecodeAccelerator. You can't dereference the > weak ptr on the IO thread, but it's valid to post a task with it. The task will > be cancelled on the target thread if the weak ptr is NULL (which is safe). > > Actually, I think you also need to keep ownership of > GpuJpegDecodeAccelerator::Client on this thread (otherwise there is a race there > too). The advantage is that you don't necessarily need to keep a map in the > GpuJpegDecodeAccelerator (you can just keep a client count to remove the > filter). You probably do need to post a task to the main thread to destroy the > accelerator though. Done.Thanks for your detail suggestion. However after done, I am thinking again the lifetime of the filter. The ref of filter is only owned by GpuJpegDecodeAccelerator, so the filter is usually deleted immediately when GpuJpegDecodeAccelerator got deleted. But the filter may still run on IO thread at that time. Sounds like that ~GpuJpegDecodeAccelerator should wait for filter on IO thread to finish? But then, the original issue (GpuJpegDecodeAccelerator has been destroyed before the filter) is gone. I will think how to handle it properly tomorrow. https://codereview.chromium.org/1124423008/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:214: DCHECK(child_task_runner_->BelongsToCurrentThread()); The ownership of |Client| is maintained by the filter. However, I am not sure the filter is always deleted on child thread. IIUC, if ~GpuJpegDecodeAccelerator doesn't wait for the filter, the filter may be still live due to line 286 and ~MessageFilter is invoked on IO thread. To solve this, I guess I have to PostTask a pure function to STLDeleteValues on child thread.
https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:115: base::Unretained(owner_), *route_id)); On 2015/05/27 14:13:23, kcwu wrote: > On 2015/05/26 23:31:53, piman (Very slow to review) wrote: > > The GpuJpegDecodeAccelerator could be destroyed before the filter, so > > dereferencing owner_ can be racy (and using Unretained unsafe). If that > happens, > > sending notification messages/removing clients is unneeded though. > > > > So what you can do is: > > 1- grab the task runners in the constructor, and keep a direct reference to > > them. Use them to post the task / check the threads. > > 2- keep a WeakPtr to the GpuJpegDecodeAccelerator. You can't dereference the > > weak ptr on the IO thread, but it's valid to post a task with it. The task > will > > be cancelled on the target thread if the weak ptr is NULL (which is safe). > > > > Actually, I think you also need to keep ownership of > > GpuJpegDecodeAccelerator::Client on this thread (otherwise there is a race > there > > too). The advantage is that you don't necessarily need to keep a map in the > > GpuJpegDecodeAccelerator (you can just keep a client count to remove the > > filter). You probably do need to post a task to the main thread to destroy the > > accelerator though. > > Done.Thanks for your detail suggestion. > > However after done, I am thinking again the lifetime of the filter. > The ref of filter is only owned by GpuJpegDecodeAccelerator, so the filter is > usually deleted immediately when GpuJpegDecodeAccelerator got deleted. But the > filter may still run on IO thread at that time. > Sounds like that ~GpuJpegDecodeAccelerator should wait for filter on IO thread > to finish? I would prefer if you didn't wait there. I don't think you need the GpuJpegDecodeAccelerator to destroy the Client? > > But then, the original issue (GpuJpegDecodeAccelerator has been destroyed before > the filter) is gone. I will think how to handle it properly tomorrow. https://codereview.chromium.org/1124423008/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:214: DCHECK(child_task_runner_->BelongsToCurrentThread()); On 2015/05/27 14:13:23, kcwu wrote: > The ownership of |Client| is maintained by the filter. However, I am not sure > the filter is always deleted on child thread. Correct, it could be deleted on either the child thread or the IO thread. > IIUC, if ~GpuJpegDecodeAccelerator doesn't wait for the filter, the filter may > be still live due to line 286 and ~MessageFilter is invoked on IO thread. Correct. > To solve this, I guess I have to PostTask a pure function to STLDeleteValues on > child thread. Yes, that would be a reasonable option.
https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:115: base::Unretained(owner_), *route_id)); On 2015/05/27 21:08:40, piman (Very slow to review) wrote: > On 2015/05/27 14:13:23, kcwu wrote: > > On 2015/05/26 23:31:53, piman (Very slow to review) wrote: > > > The GpuJpegDecodeAccelerator could be destroyed before the filter, so > > > dereferencing owner_ can be racy (and using Unretained unsafe). If that > > happens, > > > sending notification messages/removing clients is unneeded though. > > > > > > So what you can do is: > > > 1- grab the task runners in the constructor, and keep a direct reference to > > > them. Use them to post the task / check the threads. > > > 2- keep a WeakPtr to the GpuJpegDecodeAccelerator. You can't dereference the > > > weak ptr on the IO thread, but it's valid to post a task with it. The task > > will > > > be cancelled on the target thread if the weak ptr is NULL (which is safe). > > > > > > Actually, I think you also need to keep ownership of > > > GpuJpegDecodeAccelerator::Client on this thread (otherwise there is a race > > there > > > too). The advantage is that you don't necessarily need to keep a map in the > > > GpuJpegDecodeAccelerator (you can just keep a client count to remove the > > > filter). You probably do need to post a task to the main thread to destroy > the > > > accelerator though. > > > > Done.Thanks for your detail suggestion. > > > > However after done, I am thinking again the lifetime of the filter. > > The ref of filter is only owned by GpuJpegDecodeAccelerator, so the filter is > > usually deleted immediately when GpuJpegDecodeAccelerator got deleted. But the > > filter may still run on IO thread at that time. > > Sounds like that ~GpuJpegDecodeAccelerator should wait for filter on IO thread > > to finish? > > I would prefer if you didn't wait there. I don't think you need the > GpuJpegDecodeAccelerator to destroy the Client? GpuJpegDecodeAccelerator owns the filter. When GJDA is destructed, the filter will be destructed. We need to make sure the filter is not accessed on IO thread when it is being destructed. GpuChannel::RemoveFilter is asynchronous. So GJDA destructor needs to wait until the filter is removed. This is similar to GpuVideoDecodeAccelerator::OnWillDestroyStub, which also waits until the filter is removed. https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > But then, the original issue (GpuJpegDecodeAccelerator has been destroyed > before > > the filter) is gone. I will think how to handle it properly tomorrow.
https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/B... File content/common/BUILD.gn (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/B... content/common/BUILD.gn:387: "gpu/media/vaapi_jpeg_decode_accelerator.cc", Nit: Please fix order (above vaapi_picture). https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:112: DVLOG(3) << __func__; Nit: I'd remove this. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:119: } If it's possible for channel to be null at 115, it'd be nice to have a messsage for that as well. Perhaps if (!channel_ || !channel_->Send()) ? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:131: } else { What if the error is not critical, e.g. UNSUPPORTED_JPEG or PARSE_JPEG_FAILED? Could we continue instead of dying here? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/gpu_messages.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/gpu_messages.h:796: media::JpegDecodeAccelerator::Error) Error /* error */ https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:84: void OnChannelError() override { sender_ = NULL; } s/NULL/nullptr/ here and in other places. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:128: DCHECK(it != client_map_.end()); DCHECK_NE https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:206: DCHECK(client_map_.count(*route_id) > 0); DCHECK_GT https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:233: std::map<int32, Client*> client_map_; Could we use a scoper? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:16: struct AcceleratedJpegDecoderMsg_Decode_Params; Is this needed here? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:69: int client_number_; Please document. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:16: static void ReportVaapiError() { Please put this inside an anonymous namespace in namespace content {} https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:24: scoped_refptr<media::VideoFrame> video_frame) const scoped_refptr<>& should be enough https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:39: void VaapiJpegDecodeAccelerator::NotifyErrorFromDecoderThread( Could we have one NotifyError() and do a void VJDA::NotifyError() { if (!decoder_task_runner_->BelongsToCurrentThread()) { PostTask... return; } (...) client-_>NotifyError() (...) } ? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:49: DCHECK_EQ(task_runner_, base::ThreadTaskRunnerHandle::Get()); Would task_runner_->BelongsToCurrentThread() work here? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:129: memcpy(frame_mem, mem, frame_buffer_size); We shouldn't assume the same stride and size. We should use libyuv::I420Copy and use strides from the va_image and frame. We should also probably use image->image_data + image->image.offsets[plane] to get pointer for each plane, as well as corresponding methods from VideoFrame. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:156: << " size: " << (int)request->bitstream_buffer.size(); Please no C-style casts. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:183: vaapi_wrapper_->DestroySurfaces(); We should va_surface_ = VA_INVALID_SURFACE here in case CreateSurfaces() fails below. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:186: LOG(ERROR) << "Create VA surface failed"; Perhaps we could have a LOG(ERROR) in NotifyError printing out error, instead of LOG(ERROR) after errors in the code? If we had a macro like this: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m..., it would also report the line of code from which it happened, so we would know where exactly we reported error from. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:227: linked_ptr<DecodeRequest> input_buffer( We could just post this forward to DecodeTask and push on requests queue from there, we wouldn't have to lock then? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:51: // Processes one decode request in the queue. s/queue/decode_requests_ queue/ https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:54: // Helper for destroy, doing all the actual work except for deleting self. Do we need this helper? It's only called once from destructor and we don't delete self anymore. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:58: // surface and passes the buffer id of the resulting picture to client for s/buffer id/|input_buffer_id|/ ? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:64: // Protects |decode_requests_| and |initialized_|. Could we instead post a task to decoder thread to add to decode_requests_ on it without locking? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:66: bool initialized_; Since we Stop() the decoder thread before setting initialized_ to false and set it to true when starting the thread, could just having the decoder thread running mean initialized_ == true, and the decoder thread would not have to check this, since it'd have been stopped if initialized_ == false anyway? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:80: typedef std::queue<linked_ptr<DecodeRequest>> DecodeRequests; You may want to use the new c++11 style (if you like): using DecodeRequests = std::queue<linked_ptr<DecodeRequest>>; https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:90: Client* client_; Could we use a WeakPtr instead? https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:112: VASurfaceID va_surface_; s/va_surface_/va_surface_id_/ https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/content_... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/content_... content/content_common.gypi:291: 'common/gpu/client/gpu_jpeg_decode_accelerator_host.cc', Should be above .../gpu_memory... https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/content_... content/content_common.gypi:893: 'common/gpu/media/vaapi_jpeg_decode_accelerator.cc', Should be above vaapi_picture and vaapi_jpeg_decoder. https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/media.gyp#... media/media.gyp:599: 'video/jpeg_decode_accelerator.cc', Should be above picture.cc https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/video/jpeg... File media/video/jpeg_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/video/jpeg... media/video/jpeg_decode_accelerator.h:20: // keeps the ownership of them. Don't all methods must still be called on the same thread? https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/video/jpeg... media/video/jpeg_decode_accelerator.h:32: NO_ERROR, Can we NotifyError with NO_ERROR? NotifyError is always fatal...
Addressed most of comments. I'm still working on the life cycle issue between GpuChannelHost and GpuJpegDecodeAcceleratorHost. https://chromiumcodereview.appspot.com/1124423008/diff/40001/content/common/g... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/40001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:115: base::Unretained(owner_), *route_id)); On 2015/05/28 03:41:39, wuchengli wrote: > On 2015/05/27 21:08:40, piman (Very slow to review) wrote: > > On 2015/05/27 14:13:23, kcwu wrote: > > > On 2015/05/26 23:31:53, piman (Very slow to review) wrote: > > > > The GpuJpegDecodeAccelerator could be destroyed before the filter, so > > > > dereferencing owner_ can be racy (and using Unretained unsafe). If that > > > happens, > > > > sending notification messages/removing clients is unneeded though. > > > > > > > > So what you can do is: > > > > 1- grab the task runners in the constructor, and keep a direct reference > to > > > > them. Use them to post the task / check the threads. > > > > 2- keep a WeakPtr to the GpuJpegDecodeAccelerator. You can't dereference > the > > > > weak ptr on the IO thread, but it's valid to post a task with it. The task > > > will > > > > be cancelled on the target thread if the weak ptr is NULL (which is safe). > > > > > > > > Actually, I think you also need to keep ownership of > > > > GpuJpegDecodeAccelerator::Client on this thread (otherwise there is a race > > > there > > > > too). The advantage is that you don't necessarily need to keep a map in > the > > > > GpuJpegDecodeAccelerator (you can just keep a client count to remove the > > > > filter). You probably do need to post a task to the main thread to destroy > > the > > > > accelerator though. > > > > > > Done.Thanks for your detail suggestion. > > > > > > However after done, I am thinking again the lifetime of the filter. > > > The ref of filter is only owned by GpuJpegDecodeAccelerator, so the filter > is > > > usually deleted immediately when GpuJpegDecodeAccelerator got deleted. But > the > > > filter may still run on IO thread at that time. > > > Sounds like that ~GpuJpegDecodeAccelerator should wait for filter on IO > thread > > > to finish? > > > > I would prefer if you didn't wait there. I don't think you need the > > GpuJpegDecodeAccelerator to destroy the Client? > GpuJpegDecodeAccelerator owns the filter. When GJDA is destructed, the > filter will be destructed. We need to make sure the filter is not accessed > on IO thread when it is being destructed. GpuChannel::RemoveFilter is > asynchronous. So GJDA destructor needs to wait until the filter is removed. > This is similar to GpuVideoDecodeAccelerator::OnWillDestroyStub, which > also waits until the filter is removed. > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > > But then, the original issue (GpuJpegDecodeAccelerator has been destroyed > > before > > > the filter) is gone. I will think how to handle it properly tomorrow. > Done. I made ~GpuJpegDecodeAccelerator waiting for filter removed. https://chromiumcodereview.appspot.com/1124423008/diff/40001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/40001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:64: base::Lock lock_; On 2015/05/26 23:31:53, piman (Very slow to review) wrote: > I think you can remove this lock altogether, if you pass the DecodeRequest > explicitly in the PostTask, instead of keeping it in a queue. initialized_ is > set before Decode is ever called, so it's safe to read without lock. Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/B... File content/common/BUILD.gn (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/B... content/common/BUILD.gn:387: "gpu/media/vaapi_jpeg_decode_accelerator.cc", On 2015/05/28 09:13:17, Pawel Osciak wrote: > Nit: Please fix order (above vaapi_picture). Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:112: DVLOG(3) << __func__; On 2015/05/28 09:13:17, Pawel Osciak wrote: > Nit: I'd remove this. Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:119: } On 2015/05/28 09:13:17, Pawel Osciak wrote: > If it's possible for channel to be null at 115, it'd be nice to have a messsage > for that as well. Perhaps if (!channel_ || !channel_->Send()) ? I'm revisiting the life cycle relation between |channel_| and |this| and not sure channel could be null or not. I will address this later. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:131: } else { On 2015/05/28 09:13:17, Pawel Osciak wrote: > What if the error is not critical, e.g. UNSUPPORTED_JPEG or PARSE_JPEG_FAILED? > Could we continue instead of dying here? On earlier review, wuchengli suggest to error out for any errors in order to find issues (in early stage, say canary). Otherwise we may not notice something broken. And for camera case, if there is UNSUPPORTED_JPEG or PARSE_JPEG_FAILED error, it probably has the same error for next frame. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/gpu_messages.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/gpu_messages.h:796: media::JpegDecodeAccelerator::Error) On 2015/05/28 09:13:17, Pawel Osciak wrote: > Error /* error */ Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:84: void OnChannelError() override { sender_ = NULL; } On 2015/05/28 09:13:17, Pawel Osciak wrote: > s/NULL/nullptr/ here and in other places. Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:128: DCHECK(it != client_map_.end()); On 2015/05/28 09:13:17, Pawel Osciak wrote: > DCHECK_NE DCHECK_NE is not for iterator. The arguments need to be printable. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:206: DCHECK(client_map_.count(*route_id) > 0); On 2015/05/28 09:13:17, Pawel Osciak wrote: > DCHECK_GT Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:233: std::map<int32, Client*> client_map_; On 2015/05/28 09:13:17, Pawel Osciak wrote: > Could we use a scoper? Scoper implys it will automatically delete Client once the map is being deleted. But the filter doesn't guarantee it is deleted on the right thread. So I prefer to handle map's life cycle manually. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:16: struct AcceleratedJpegDecoderMsg_Decode_Params; On 2015/05/28 09:13:17, Pawel Osciak wrote: > Is this needed here? Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:69: int client_number_; On 2015/05/28 09:13:17, Pawel Osciak wrote: > Please document. Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:16: static void ReportVaapiError() { On 2015/05/28 09:13:18, Pawel Osciak wrote: > Please put this inside an anonymous namespace in namespace content {} Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:24: scoped_refptr<media::VideoFrame> video_frame) On 2015/05/28 09:13:18, Pawel Osciak wrote: > const scoped_refptr<>& should be enough Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:39: void VaapiJpegDecodeAccelerator::NotifyErrorFromDecoderThread( On 2015/05/28 09:13:17, Pawel Osciak wrote: > Could we have one NotifyError() and do a > > void VJDA::NotifyError() { > if (!decoder_task_runner_->BelongsToCurrentThread()) { > PostTask... > return; > } > > (...) > client-_>NotifyError() > (...) > } > > ? piman suggest not to do so. https://chromiumcodereview.appspot.com/1124423008/diff/1/content/common/gpu/m... https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:49: DCHECK_EQ(task_runner_, base::ThreadTaskRunnerHandle::Get()); On 2015/05/28 09:13:18, Pawel Osciak wrote: > Would task_runner_->BelongsToCurrentThread() work here? Why not? This method is posted from OutputPicture. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:156: << " size: " << (int)request->bitstream_buffer.size(); On 2015/05/28 09:13:18, Pawel Osciak wrote: > Please no C-style casts. Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:183: vaapi_wrapper_->DestroySurfaces(); On 2015/05/28 09:13:18, Pawel Osciak wrote: > We should va_surface_ = VA_INVALID_SURFACE here in case CreateSurfaces() fails > below. Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:186: LOG(ERROR) << "Create VA surface failed"; On 2015/05/28 09:13:17, Pawel Osciak wrote: > Perhaps we could have a LOG(ERROR) in NotifyError printing out error, instead of > LOG(ERROR) after errors in the code? If we had a macro like this: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m..., > it would also report the line of code from which it happened, so we would know > where exactly we reported error from. Passing error string as function argument doesn't save much. LOG(ERROR) is one line, string argument is still one line (the string is not short enough). I would prefer not to use macro since the benefit is not big enough. LOG() already can report line number. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:227: linked_ptr<DecodeRequest> input_buffer( On 2015/05/28 09:13:18, Pawel Osciak wrote: > We could just post this forward to DecodeTask and push on requests queue from > there, we wouldn't have to lock then? Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:51: // Processes one decode request in the queue. On 2015/05/28 09:13:18, Pawel Osciak wrote: > s/queue/decode_requests_ queue/ Acknowledged. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:54: // Helper for destroy, doing all the actual work except for deleting self. On 2015/05/28 09:13:18, Pawel Osciak wrote: > Do we need this helper? It's only called once from destructor and we don't > delete self anymore. Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:58: // surface and passes the buffer id of the resulting picture to client for On 2015/05/28 09:13:18, Pawel Osciak wrote: > s/buffer id/|input_buffer_id|/ ? Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:64: // Protects |decode_requests_| and |initialized_|. On 2015/05/28 09:13:18, Pawel Osciak wrote: > Could we instead post a task to decoder thread to add to decode_requests_ on it > without locking? Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:66: bool initialized_; On 2015/05/28 09:13:18, Pawel Osciak wrote: > Since we Stop() the decoder thread before setting initialized_ to false and set > it to true when starting the thread, could just having the decoder thread > running mean initialized_ == true, and the decoder thread would not have to > check this, since it'd have been stopped if initialized_ == false anyway? Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:80: typedef std::queue<linked_ptr<DecodeRequest>> DecodeRequests; On 2015/05/28 09:13:18, Pawel Osciak wrote: > You may want to use the new c++11 style (if you like): > using DecodeRequests = std::queue<linked_ptr<DecodeRequest>>; Acknowledged. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:90: Client* client_; On 2015/05/28 09:13:18, Pawel Osciak wrote: > Could we use a WeakPtr instead? It was. And we found it could be not a WeakPtr from the review. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/common/g... content/common/gpu/media/vaapi_jpeg_decode_accelerator.h:112: VASurfaceID va_surface_; On 2015/05/28 09:13:18, Pawel Osciak wrote: > s/va_surface_/va_surface_id_/ Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/content_... File content/content_common.gypi (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/content_... content/content_common.gypi:291: 'common/gpu/client/gpu_jpeg_decode_accelerator_host.cc', On 2015/05/28 09:13:18, Pawel Osciak wrote: > Should be above .../gpu_memory... Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/content/content_... content/content_common.gypi:893: 'common/gpu/media/vaapi_jpeg_decode_accelerator.cc', On 2015/05/28 09:13:18, Pawel Osciak wrote: > Should be above vaapi_picture and vaapi_jpeg_decoder. Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/media.gyp#... media/media.gyp:599: 'video/jpeg_decode_accelerator.cc', On 2015/05/28 09:13:18, Pawel Osciak wrote: > Should be above picture.cc Done. https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/video/jpeg... File media/video/jpeg_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/video/jpeg... media/video/jpeg_decode_accelerator.h:20: // keeps the ownership of them. On 2015/05/28 09:13:18, Pawel Osciak wrote: > Don't all methods must still be called on the same thread? Not always. For GpuJpegDecodeAccelerator, it call Initialize on child thred and call Decode on IO thread directly. https://chromiumcodereview.appspot.com/1124423008/diff/60001/media/video/jpeg... media/video/jpeg_decode_accelerator.h:32: NO_ERROR, On 2015/05/28 09:13:18, Pawel Osciak wrote: > Can we NotifyError with NO_ERROR? NotifyError is always fatal... Do you have any suggestions? Is saying NotifyError won't take NO_ERROR enough? Or should NotifyError and VideoFrameReady be merged like gpu message?
On Wed, May 27, 2015 at 8:41 PM, <wuchengli@chromium.org> wrote: > > > https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... > File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): > > > https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... > content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:115: > base::Unretained(owner_), *route_id)); > On 2015/05/27 21:08:40, piman (Very slow to review) wrote: > >> On 2015/05/27 14:13:23, kcwu wrote: >> > On 2015/05/26 23:31:53, piman (Very slow to review) wrote: >> > > The GpuJpegDecodeAccelerator could be destroyed before the filter, >> > so > >> > > dereferencing owner_ can be racy (and using Unretained unsafe). If >> > that > >> > happens, >> > > sending notification messages/removing clients is unneeded though. >> > > >> > > So what you can do is: >> > > 1- grab the task runners in the constructor, and keep a direct >> > reference to > >> > > them. Use them to post the task / check the threads. >> > > 2- keep a WeakPtr to the GpuJpegDecodeAccelerator. You can't >> > dereference the > >> > > weak ptr on the IO thread, but it's valid to post a task with it. >> > The task > >> > will >> > > be cancelled on the target thread if the weak ptr is NULL (which >> > is safe). > >> > > >> > > Actually, I think you also need to keep ownership of >> > > GpuJpegDecodeAccelerator::Client on this thread (otherwise there >> > is a race > >> > there >> > > too). The advantage is that you don't necessarily need to keep a >> > map in the > >> > > GpuJpegDecodeAccelerator (you can just keep a client count to >> > remove the > >> > > filter). You probably do need to post a task to the main thread to >> > destroy > >> the >> > > accelerator though. >> > >> > Done.Thanks for your detail suggestion. >> > >> > However after done, I am thinking again the lifetime of the filter. >> > The ref of filter is only owned by GpuJpegDecodeAccelerator, so the >> > filter is > >> > usually deleted immediately when GpuJpegDecodeAccelerator got >> > deleted. But the > >> > filter may still run on IO thread at that time. >> > Sounds like that ~GpuJpegDecodeAccelerator should wait for filter on >> > IO thread > >> > to finish? >> > > I would prefer if you didn't wait there. I don't think you need the >> GpuJpegDecodeAccelerator to destroy the Client? >> > GpuJpegDecodeAccelerator owns the filter. When GJDA is destructed, the > filter will be destructed. We need to make sure the filter is not > accessed > on IO thread when it is being destructed. GpuChannel::RemoveFilter is > asynchronous. So GJDA destructor needs to wait until the filter is > removed. > See the discussion above. It's perfectly possible to implement: 1- GpuJpegDecodeAccelerator gets destroyed on the main thread 2- Filter gets destroyed on IO thread, posts a task to main thread 3- GpuJpegDecodeAccelerator::Client gets destroyed on main thread. > This is similar to GpuVideoDecodeAccelerator::OnWillDestroyStub, which > also waits until the filter is removed. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > There are other constraints on GpuVideoDecodeAccelerator because it needs to interact with the GpuCommandBufferStub which lives on the main thread and has a separate lifetime. This is not the case for GpuJpegDecodeAccelerator. (Actually, that being said, I believe it's still possible in theory to avoid that wait, but it's a separate story). > > But then, the original issue (GpuJpegDecodeAccelerator has been >> > destroyed > >> before >> > the filter) is gone. I will think how to handle it properly >> > tomorrow. > > https://codereview.chromium.org/1124423008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/28 18:16:57, piman (Very slow to review) wrote: > On Wed, May 27, 2015 at 8:41 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=wuchengli@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... > > File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): > > > > > > > https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... > > content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:115: > > base::Unretained(owner_), *route_id)); > > On 2015/05/27 21:08:40, piman (Very slow to review) wrote: > > > >> On 2015/05/27 14:13:23, kcwu wrote: > >> > On 2015/05/26 23:31:53, piman (Very slow to review) wrote: > >> > > The GpuJpegDecodeAccelerator could be destroyed before the filter, > >> > > so > > > >> > > dereferencing owner_ can be racy (and using Unretained unsafe). If > >> > > that > > > >> > happens, > >> > > sending notification messages/removing clients is unneeded though. > >> > > > >> > > So what you can do is: > >> > > 1- grab the task runners in the constructor, and keep a direct > >> > > reference to > > > >> > > them. Use them to post the task / check the threads. > >> > > 2- keep a WeakPtr to the GpuJpegDecodeAccelerator. You can't > >> > > dereference the > > > >> > > weak ptr on the IO thread, but it's valid to post a task with it. > >> > > The task > > > >> > will > >> > > be cancelled on the target thread if the weak ptr is NULL (which > >> > > is safe). > > > >> > > > >> > > Actually, I think you also need to keep ownership of > >> > > GpuJpegDecodeAccelerator::Client on this thread (otherwise there > >> > > is a race > > > >> > there > >> > > too). The advantage is that you don't necessarily need to keep a > >> > > map in the > > > >> > > GpuJpegDecodeAccelerator (you can just keep a client count to > >> > > remove the > > > >> > > filter). You probably do need to post a task to the main thread to > >> > > destroy > > > >> the > >> > > accelerator though. > >> > > >> > Done.Thanks for your detail suggestion. > >> > > >> > However after done, I am thinking again the lifetime of the filter. > >> > The ref of filter is only owned by GpuJpegDecodeAccelerator, so the > >> > > filter is > > > >> > usually deleted immediately when GpuJpegDecodeAccelerator got > >> > > deleted. But the > > > >> > filter may still run on IO thread at that time. > >> > Sounds like that ~GpuJpegDecodeAccelerator should wait for filter on > >> > > IO thread > > > >> > to finish? > >> > > > > I would prefer if you didn't wait there. I don't think you need the > >> GpuJpegDecodeAccelerator to destroy the Client? > >> > > GpuJpegDecodeAccelerator owns the filter. When GJDA is destructed, the > > filter will be destructed. We need to make sure the filter is not > > accessed > > on IO thread when it is being destructed. GpuChannel::RemoveFilter is > > asynchronous. So GJDA destructor needs to wait until the filter is > > removed. > > > > See the discussion above. It's perfectly possible to implement: > 1- GpuJpegDecodeAccelerator gets destroyed on the main thread > 2- Filter gets destroyed on IO thread, posts a task to main thread > 3- GpuJpegDecodeAccelerator::Client gets destroyed on main thread. Ah, I got how it works now. Although AddFilter only takes MessageFilter*, it internally use scoped_refptr and thus still respect the refcount. So we can safely destroy GpuJpegDecodeAccelerator no matter the filter is still running on IO thread or not.
On Thu, May 28, 2015 at 12:05 PM, <kcwu@chromium.org> wrote: > On 2015/05/28 18:16:57, piman (Very slow to review) wrote: > >> On Wed, May 27, 2015 at 8:41 PM, >> > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=wuchengli@chromium.org > > > > wrote: > > > >> > >> > >> > > > https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... > >> > File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): >> > >> > >> > >> > > > https://codereview.chromium.org/1124423008/diff/40001/content/common/gpu/medi... > >> > content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:115: >> > base::Unretained(owner_), *route_id)); >> > On 2015/05/27 21:08:40, piman (Very slow to review) wrote: >> > >> >> On 2015/05/27 14:13:23, kcwu wrote: >> >> > On 2015/05/26 23:31:53, piman (Very slow to review) wrote: >> >> > > The GpuJpegDecodeAccelerator could be destroyed before the filter, >> >> >> > so >> > >> >> > > dereferencing owner_ can be racy (and using Unretained unsafe). If >> >> >> > that >> > >> >> > happens, >> >> > > sending notification messages/removing clients is unneeded though. >> >> > > >> >> > > So what you can do is: >> >> > > 1- grab the task runners in the constructor, and keep a direct >> >> >> > reference to >> > >> >> > > them. Use them to post the task / check the threads. >> >> > > 2- keep a WeakPtr to the GpuJpegDecodeAccelerator. You can't >> >> >> > dereference the >> > >> >> > > weak ptr on the IO thread, but it's valid to post a task with it. >> >> >> > The task >> > >> >> > will >> >> > > be cancelled on the target thread if the weak ptr is NULL (which >> >> >> > is safe). >> > >> >> > > >> >> > > Actually, I think you also need to keep ownership of >> >> > > GpuJpegDecodeAccelerator::Client on this thread (otherwise there >> >> >> > is a race >> > >> >> > there >> >> > > too). The advantage is that you don't necessarily need to keep a >> >> >> > map in the >> > >> >> > > GpuJpegDecodeAccelerator (you can just keep a client count to >> >> >> > remove the >> > >> >> > > filter). You probably do need to post a task to the main thread to >> >> >> > destroy >> > >> >> the >> >> > > accelerator though. >> >> > >> >> > Done.Thanks for your detail suggestion. >> >> > >> >> > However after done, I am thinking again the lifetime of the filter. >> >> > The ref of filter is only owned by GpuJpegDecodeAccelerator, so the >> >> >> > filter is >> > >> >> > usually deleted immediately when GpuJpegDecodeAccelerator got >> >> >> > deleted. But the >> > >> >> > filter may still run on IO thread at that time. >> >> > Sounds like that ~GpuJpegDecodeAccelerator should wait for filter on >> >> >> > IO thread >> > >> >> > to finish? >> >> >> > >> > I would prefer if you didn't wait there. I don't think you need the >> >> GpuJpegDecodeAccelerator to destroy the Client? >> >> >> > GpuJpegDecodeAccelerator owns the filter. When GJDA is destructed, the >> > filter will be destructed. We need to make sure the filter is not >> > accessed >> > on IO thread when it is being destructed. GpuChannel::RemoveFilter is >> > asynchronous. So GJDA destructor needs to wait until the filter is >> > removed. >> > >> > > See the discussion above. It's perfectly possible to implement: >> 1- GpuJpegDecodeAccelerator gets destroyed on the main thread >> 2- Filter gets destroyed on IO thread, posts a task to main thread >> 3- GpuJpegDecodeAccelerator::Client gets destroyed on main thread. >> > > Ah, I got how it works now. Although AddFilter only takes MessageFilter*, > it > internally use scoped_refptr and thus still respect the refcount. So we can > safely destroy GpuJpegDecodeAccelerator no matter the filter is still > running on > IO thread or not. > Right, MessageFilter is refcounted by the channel on the IO thread. No need for synchronizing with the IO thread to unref. > > > https://codereview.chromium.org/1124423008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:93: base::ScopedFD(input_handle.fd); This won't work on non-posix. You can use SharedMemory::CloseHandle which is more portable, but unfortunately neither one will do the right thing on Windows, because the returned handle is for another process. +jschuh do you have a best practice idea here? Can ShareToGpuProcess (i.e. BrokerDuplicateHandle) fail, outside of either invalid handles or the target process being closed? https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:114: if (!channel_) actually, you need to delete message here. Send guarantees it will delete the message. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:42: : public media::JpegDecodeAccelerator::Client { nit: can you make this NonThreadSafe, and check the thread before dereferencing owner_? https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:99: if (client_map_.count(route_id) == 0) nit: if (client_map_.find(route_id) == client_map_.end()) It's more verbose, but in theory can be faster. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:176: params.input_buffer_handle, input_buffer_handle is only released by Decode, but isn't released in any of the early exit cases. Should the SharedMemory be created here, instead of in the VaapiJpegDecodeAccelerator? This would be consistent with the JpegDecodeAccelerator documentation. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:180: new base::SharedMemory(params.output_video_frame_handle, false)); You probably want to do this before the first early return to make sure output_video_frame_handle isn't leaked. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:190: uint8* shm_memory = reinterpret_cast<uint8*>(output_shm->memory()); nit: uint8_t instead of uint8, also you can use static_cast instead of reinterpret_cast. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:231: std::swap(client_map_, *client_map); nit: client_map->swap(client_map_); https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:260: std::map<int32, Client*> client_map_; nit: hash_map. You can also make a typedef to avoid repeating the type in a few places. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:280: filter_->WaitForRemoved(); As discussed, you don't need this. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:321: route_id, client.release(), reply_msg)); can you use Passed(client) instead? https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:24: : public IPC::Listener, nit: you don't need this any more. Nothing calls OnMessageReceived since GpuJpegDecodeAccelerator is not added to the route map in the GpuChannel.
piman@chromium.org changed reviewers: + jschuh@chromium.org
+jschuh, forgot to add. Please see comment in https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/cli...
https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:42: : public media::JpegDecodeAccelerator::Client { On 2015/05/28 22:01:40, piman (Very slow to review) wrote: > nit: can you make this NonThreadSafe, and check the thread before dereferencing > owner_? Done. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:99: if (client_map_.count(route_id) == 0) On 2015/05/28 22:01:39, piman (Very slow to review) wrote: > nit: if (client_map_.find(route_id) == client_map_.end()) > > It's more verbose, but in theory can be faster. Done. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:176: params.input_buffer_handle, On 2015/05/28 22:01:40, piman (Very slow to review) wrote: > input_buffer_handle is only released by Decode, but isn't released in any of the > early exit cases. Should the SharedMemory be created here, instead of in the > VaapiJpegDecodeAccelerator? This would be consistent with the > JpegDecodeAccelerator documentation. Because SharedMemory takes ownership of SharedMemoryHandle and JpegDecodeAccelerator::Decode doesn't get SharedMemory as argument, it cannot be created here. Otherwise Decode will create SharedMemory again and results in the handle is closed twice. Now I added SharedMemory::CloseHandle before all early exits. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:180: new base::SharedMemory(params.output_video_frame_handle, false)); On 2015/05/28 22:01:40, piman (Very slow to review) wrote: > You probably want to do this before the first early return to make sure > output_video_frame_handle isn't leaked. Done. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:190: uint8* shm_memory = reinterpret_cast<uint8*>(output_shm->memory()); On 2015/05/28 22:01:40, piman (Very slow to review) wrote: > nit: uint8_t instead of uint8, also you can use static_cast instead of > reinterpret_cast. This aligns to VideoFrame::WrapExternalPackedMemory which takes uint8. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:231: std::swap(client_map_, *client_map); On 2015/05/28 22:01:39, piman (Very slow to review) wrote: > nit: client_map->swap(client_map_); Done. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:260: std::map<int32, Client*> client_map_; On 2015/05/28 22:01:39, piman (Very slow to review) wrote: > nit: hash_map. > You can also make a typedef to avoid repeating the type in a few places. Done. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:280: filter_->WaitForRemoved(); On 2015/05/28 22:01:39, piman (Very slow to review) wrote: > As discussed, you don't need this. Done. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:321: route_id, client.release(), reply_msg)); On 2015/05/28 22:01:39, piman (Very slow to review) wrote: > can you use Passed(client) instead? Does io_task_runner guarantee the task of RefCountedThreadSafe(filter) run? If not, Passed(scoper) implies it could be deleted safely (on IO thread?) if the task doesn't run, which is not true. Should I change Client to be RefCountedThreasSafe ? https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:24: : public IPC::Listener, On 2015/05/28 22:01:40, piman (Very slow to review) wrote: > nit: you don't need this any more. Nothing calls OnMessageReceived since > GpuJpegDecodeAccelerator is not added to the route map in the GpuChannel. Done.
https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:176: params.input_buffer_handle, On 2015/05/29 11:11:25, kcwu wrote: > On 2015/05/28 22:01:40, piman (Very slow to review) wrote: > > input_buffer_handle is only released by Decode, but isn't released in any of > the > > early exit cases. Should the SharedMemory be created here, instead of in the > > VaapiJpegDecodeAccelerator? This would be consistent with the > > JpegDecodeAccelerator documentation. > > Because SharedMemory takes ownership of SharedMemoryHandle and > JpegDecodeAccelerator::Decode doesn't get SharedMemory as argument, it cannot be > created here. Otherwise Decode will create SharedMemory again and results in the > handle is closed twice. Now I added SharedMemory::CloseHandle before all early > exits. The documentation in JpegDecodeAccelerator::Decode explicitly states "Ownership of the |bitstream_buffer| and |video_frame| remains with the client." Does that mean the documentation is wrong and needs to be fixed? Though I'm not sure how you would document inconsistent behavior. Obviously the other way is to explicitly pass base::SharedMemory to JpegDecodeAccelerator::Decode https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:190: uint8* shm_memory = reinterpret_cast<uint8*>(output_shm->memory()); On 2015/05/29 11:11:24, kcwu wrote: > On 2015/05/28 22:01:40, piman (Very slow to review) wrote: > > nit: uint8_t instead of uint8, also you can use static_cast instead of > > reinterpret_cast. > > This aligns to VideoFrame::WrapExternalPackedMemory which takes uint8. Please use uint8_t in new code. uint8 is deprecated. See base/basictypes.h https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:321: route_id, client.release(), reply_msg)); On 2015/05/29 11:11:25, kcwu wrote: > On 2015/05/28 22:01:39, piman (Very slow to review) wrote: > > can you use Passed(client) instead? > > Does io_task_runner guarantee the task of RefCountedThreadSafe(filter) run? > > If not, Passed(scoper) implies it could be deleted safely (on IO thread?) if the > task doesn't run, which is not true. Should I change Client to be > RefCountedThreasSafe ? Fair enough, the message loop would be destroyed on the IO thread, possibly deleting tasks there. Can you add a comment instead, explaining the lifetime of |client|?
Addressed all comments. The only remain issue is how to properly SharedMemory::CloseHandle https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h (right): https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/client/g... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.h:25: // |channel_|.) On 2015/05/18 22:46:18, piman (Very slow to review) wrote: > I'm confused by this comment. Comments for channel_ mention it can be NULL, > which would suggest |this| could outlive |channel_|. > > More generally, what guarantees that GpuJpegDecodeAcceleratorHost will be > Destroy'ed before the channel is? In particular after a lost channel (e.g. GPU > process dying), the GpuChannelHost could get destroyed as a side effect of > context recreation - see RenderThreadImpl::EstablishGpuChannelSync / > BrowserGpuChannelHostFactory::EstablishGpuChannel, and this might happen before > this class had a chance to get the OnChannelError notification. Done. The life cycle issue is fixed (together with the other CL, https://codereview.chromium.org/1132683004/ ) https://codereview.chromium.org/1124423008/diff/60001/content/common/gpu/medi... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/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/05/28 09:13:18, Pawel Osciak wrote: > We shouldn't assume the same stride and size. We should use libyuv::I420Copy and > use strides from the va_image and frame. We should also probably use > image->image_data + image->image.offsets[plane] to get pointer for each plane, > as well as corresponding methods from VideoFrame. Done. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:114: if (!channel_) On 2015/05/28 22:01:39, piman (Very slow to review) wrote: > actually, you need to delete message here. Send guarantees it will delete the > message. Done. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:176: params.input_buffer_handle, On 2015/06/01 22:33:36, piman (Very slow to review) wrote: > On 2015/05/29 11:11:25, kcwu wrote: > > On 2015/05/28 22:01:40, piman (Very slow to review) wrote: > > > input_buffer_handle is only released by Decode, but isn't released in any of > > the > > > early exit cases. Should the SharedMemory be created here, instead of in the > > > VaapiJpegDecodeAccelerator? This would be consistent with the > > > JpegDecodeAccelerator documentation. > > > > Because SharedMemory takes ownership of SharedMemoryHandle and > > JpegDecodeAccelerator::Decode doesn't get SharedMemory as argument, it cannot > be > > created here. Otherwise Decode will create SharedMemory again and results in > the > > handle is closed twice. Now I added SharedMemory::CloseHandle before all early > > exits. > > The documentation in JpegDecodeAccelerator::Decode explicitly states "Ownership > of the |bitstream_buffer| and |video_frame| remains with the client." > > Does that mean the documentation is wrong and needs to be fixed? Though I'm not > sure how you would document inconsistent behavior. > > Obviously the other way is to explicitly pass base::SharedMemory to > JpegDecodeAccelerator::Decode The "ownership" are referring to the underlying memory storage, which is still owned by the client (on the browser process) in this case. Not to about the handle. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:190: uint8* shm_memory = reinterpret_cast<uint8*>(output_shm->memory()); On 2015/06/01 22:33:35, piman (Very slow to review) wrote: > On 2015/05/29 11:11:24, kcwu wrote: > > On 2015/05/28 22:01:40, piman (Very slow to review) wrote: > > > nit: uint8_t instead of uint8, also you can use static_cast instead of > > > reinterpret_cast. > > > > This aligns to VideoFrame::WrapExternalPackedMemory which takes uint8. > > Please use uint8_t in new code. uint8 is deprecated. See base/basictypes.h Done. https://codereview.chromium.org/1124423008/diff/120001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:321: route_id, client.release(), reply_msg)); On 2015/06/01 22:33:36, piman (Very slow to review) wrote: > On 2015/05/29 11:11:25, kcwu wrote: > > On 2015/05/28 22:01:39, piman (Very slow to review) wrote: > > > can you use Passed(client) instead? > > > > Does io_task_runner guarantee the task of RefCountedThreadSafe(filter) run? > > > > If not, Passed(scoper) implies it could be deleted safely (on IO thread?) if > the > > task doesn't run, which is not true. Should I change Client to be > > RefCountedThreasSafe ? > > Fair enough, the message loop would be destroyed on the IO thread, possibly > deleting tasks there. > Can you add a comment instead, explaining the lifetime of |client|? Done.
The gpu process side looks good modulo nits, but I think the host side needs more work - maybe we can separate that part out into on another CL so that the GPU side can land? https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/cli... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:279: decoder->AsWeakPtr(), io_task_runner)); Mmh, it's not valid to dereference the weak pointer on the IO thread if the class is deleted on another thread. https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:41: event.Wait(); I don't think this is enough, you could still have a race, because the code on the IO thread posts a task to call OnMessageReceived/OnChannelError. So on the IO thread you could end up with the task queue with the RemoveRoute task and the WaitableEvent::Signal task, but if an IPC message is received before the RemoveRoute task is handled, then the filter will handle the message and post another task to do the OnMessageReceived, that will be queued *after* the WaitableEvent::Signal task. I think to solve this and the problem mentioned in GpuChannelHost, you may want to separate a "receiver" object which purely lives on the IO thread and has its own weakptr. https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:244: using ClientMap = std::unordered_map<int32, Client*>; nit: we can't use unordered_map yet because it's a c++11 library feature. You can use base::hash_map from base/containers/hash_tables.h https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:316: // to pretect it because |client| can only be deleted on child thread. We nit: pretect->protect https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:318: // complicated. nit: maybe you can mention that the IO thread is destroyed at termination, at which point it's ok to leak since we're going to tear down the process anyway.
jschuh and jln: owner review for gpu_messages.h. Thanks.
The GPU host part is split into a separate CL https://codereview.chromium.org/1165943008 https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:244: using ClientMap = std::unordered_map<int32, Client*>; On 2015/06/05 21:49:40, piman (Very slow to review) wrote: > nit: we can't use unordered_map yet because it's a c++11 library feature. You > can use base::hash_map from base/containers/hash_tables.h Done. https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:316: // to pretect it because |client| can only be deleted on child thread. We On 2015/06/05 21:49:40, piman (Very slow to review) wrote: > nit: pretect->protect Done. https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:318: // complicated. On 2015/06/05 21:49:40, piman (Very slow to review) wrote: > nit: maybe you can mention that the IO thread is destroyed at termination, at > which point it's ok to leak since we're going to tear down the process anyway. Done.
In summary, there are total 4 CLs for MJPEG acceleration. They are - https://codereview.chromium.org/1124423008/ GPU and IPC part <-- this one - https://codereview.chromium.org/1165943008/ GPU host part - https://codereview.chromium.org/1132683004/ browser client to use - https://codereview.chromium.org/1153163003/ flag to enable
(host side fixed in https://codereview.chromium.org/1165943008 ) https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/cli... File content/common/gpu/client/gpu_channel_host.cc (right): https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/cli... content/common/gpu/client/gpu_channel_host.cc:279: decoder->AsWeakPtr(), io_task_runner)); On 2015/06/05 21:49:39, piman (Very slow to review) wrote: > Mmh, it's not valid to dereference the weak pointer on the IO thread if the > class is deleted on another thread. Done. https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/cli... File content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/1124423008/diff/180001/content/common/gpu/cli... content/common/gpu/client/gpu_jpeg_decode_accelerator_host.cc:41: event.Wait(); On 2015/06/05 21:49:40, piman (Very slow to review) wrote: > I don't think this is enough, you could still have a race, because the code on > the IO thread posts a task to call OnMessageReceived/OnChannelError. > > So on the IO thread you could end up with the task queue with the RemoveRoute > task and the WaitableEvent::Signal task, but if an IPC message is received > before the RemoveRoute task is handled, then the filter will handle the message > and post another task to do the OnMessageReceived, that will be queued *after* > the WaitableEvent::Signal task. > > > I think to solve this and the problem mentioned in GpuChannelHost, you may want > to separate a "receiver" object which purely lives on the IO thread and has its > own weakptr. Done.
@piman, do you continue to review this CL or did you lgtm the wrong one? I asked so because you said this one (GPU side) is good but you lgtm the other (host side).
On 2015/06/09 08:33:52, kcwu wrote: > @piman, do you continue to review this CL or did you lgtm the wrong one? > I asked so because you said this one (GPU side) is good but you lgtm the other > (host side). Sorry, LGTM here too. Other CL LGTM as well.
jschuh and jln: security review for gpu_messages.h, please. All other files already got lgtm. piman: thanks a lot for your review comments in this and other CLs.
https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... File content/common/gpu/gpu_channel.h (right): https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... content/common/gpu/gpu_channel.h:21: #include "content/common/gpu/media/gpu_jpeg_decode_accelerator.h" This is not required here I believe, since we are declaring it below. https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:21: // TODO(kcwu) report error to UMA This should be simple enough to do already, if we did something like this: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu.... Or did you have something more complicated in mind? https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:122: void* mem = nullptr; const possible? https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:135: uint8* src_y = static_cast<uint8*>(mem) + image.offsets[0]; s/uint8*/char*/ (or uint8_t*). But could we just have mem as char/uint8_t and cast when calling GetVaImage, instead of here three times? https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:148: libyuv::I420Copy(src_y, src_y_stride, // Y Please check return value. https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:183: gfx::Size coded_size(parse_result.frame_header.coded_width, s/coded_size/new_coded_size/ perhaps? https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:189: std::vector<VASurfaceID> va_surfaces; va_surfaces(1) https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:190: if (!vaapi_wrapper_->CreateSurfaces(coded_size, 1, &va_surfaces)) { Ouch, &va_surfaces[0]. https://chromiumcodereview.appspot.com/1124423008/diff/200001/media/video/jpe... File media/video/jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/200001/media/video/jpe... media/video/jpeg_decode_accelerator.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Do we still need this file?
wuchengli@chromium.org changed reviewers: + nasko@chromium.org
+nasko nasko@ OWNER review for gpu_messages.h. Thanks.
https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/200001/content/common/... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:21: // TODO(kcwu) report error to UMA On 2015/06/10 08:02:45, Pawel Osciak wrote: > This should be simple enough to do already, if we did something like this: > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu.... > > Or did you have something more complicated in mind? Let's make it a separate CL because it will need another owner review. Kuang-che. Please prepare a CL for it.
https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/gpu... File content/common/gpu/gpu_channel.h (right): https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/gpu... content/common/gpu/gpu_channel.h:21: #include "content/common/gpu/media/gpu_jpeg_decode_accelerator.h" On 2015/06/10 08:02:44, Pawel Osciak wrote: > This is not required here I believe, since we are declaring it below. Done. https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:122: void* mem = nullptr; On 2015/06/10 08:02:44, Pawel Osciak wrote: > const possible? GetVaImage (and vaMapBuffer) want void**. If mem is const uint8_t*, reinterpret_cast<void**>(const_cast<uint8_t**>(&mem)) looks ugly. (any suggestions?) So I let mem be non-const. https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:135: uint8* src_y = static_cast<uint8*>(mem) + image.offsets[0]; On 2015/06/10 08:02:45, Pawel Osciak wrote: > s/uint8*/char*/ (or uint8_t*). But could we just have mem as char/uint8_t and > cast when calling GetVaImage, instead of here three times? Done. https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:148: libyuv::I420Copy(src_y, src_y_stride, // Y On 2015/06/10 08:02:44, Pawel Osciak wrote: > Please check return value. Done. https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:183: gfx::Size coded_size(parse_result.frame_header.coded_width, On 2015/06/10 08:02:45, Pawel Osciak wrote: > s/coded_size/new_coded_size/ perhaps? Done. https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:189: std::vector<VASurfaceID> va_surfaces; On 2015/06/10 08:02:45, Pawel Osciak wrote: > va_surfaces(1) no, VaapiWrapper::CreateSurfaces expect an empty vector. https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:190: if (!vaapi_wrapper_->CreateSurfaces(coded_size, 1, &va_surfaces)) { On 2015/06/10 08:02:45, Pawel Osciak wrote: > Ouch, &va_surfaces[0]. No, VaapiWrapper::CreateSurfaces takes pointer of vector, not array. https://codereview.chromium.org/1124423008/diff/200001/media/video/jpeg_decod... File media/video/jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/200001/media/video/jpeg_decod... media/video/jpeg_decode_accelerator.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/06/10 08:02:45, Pawel Osciak wrote: > Do we still need this file? Done. Move the destructor to header file. It's okay to inline destructor for this case, right?
lgtm https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:190: if (!vaapi_wrapper_->CreateSurfaces(coded_size, 1, &va_surfaces)) { On 2015/06/11 07:29:15, kcwu wrote: > On 2015/06/10 08:02:45, Pawel Osciak wrote: > > Ouch, &va_surfaces[0]. > > No, VaapiWrapper::CreateSurfaces takes pointer of vector, not array. Ah sorry, got it mixed up with vaCreateSurfaces. Explains how it worked before.
nasko@chromium.org changed reviewers: + palmer@chromium.org
Adding palmer@ to do the IPC review, as I won't have the chance in the next couple of days.
Not done reviewing yet, but here are some initial comments/questions. Note also typo in commit message: "percitile" (should be "percentile"). Also, is this still a net performance win, given that it exercises the IPC channel and I/O thread more than before? https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/gpu... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:63: IPC_ENUM_TRAITS_MAX_VALUE(media::JpegDecodeAccelerator::Error, It might be a good idea to use IPC_ENUM_TRAITS_MIN_MAX_VALUE, and set the minimum to INVALID_ARGUMENT. https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:50: // GpuChannels destroy all the GpuJpegDecodeAccelerator that they own when This comment feels a bit strange to me; as declared, the code seems to be saying that GpuJpegDecodeAccelerator has-a GpuChannel* (to me, has-a means "owns a"), but the comment says the opposite. Can you please clarify?
https://chromiumcodereview.appspot.com/1124423008/diff/240001/content/common/... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/240001/content/common/... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:160: const AcceleratedJpegDecoderMsg_Decode_Params& params) { Could you validate all the params members early on here? Notably, could you check that the "size" members fall into some reasonable range?
https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:21: // TODO(kcwu) report error to UMA It's probably best to file a bug and mention the bug # here.
On 2015/06/11 23:28:28, palmer wrote: > Not done reviewing yet, but here are some initial comments/questions. > > Note also typo in commit message: "percitile" (should be "percentile"). > > Also, is this still a net performance win, given that it exercises the IPC > channel and I/O thread more than before? Yes. This is a net performance win. The numbers in the change description already include all the overhead of HW decode. > > https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/gpu... > File content/common/gpu/gpu_messages.h (right): > > https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/gpu... > content/common/gpu/gpu_messages.h:63: > IPC_ENUM_TRAITS_MAX_VALUE(media::JpegDecodeAccelerator::Error, > It might be a good idea to use IPC_ENUM_TRAITS_MIN_MAX_VALUE, and set the > minimum to INVALID_ARGUMENT. > > https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... > File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): > > https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... > content/common/gpu/media/gpu_jpeg_decode_accelerator.h:50: // GpuChannels > destroy all the GpuJpegDecodeAccelerator that they own when > This comment feels a bit strange to me; as declared, the code seems to be saying > that GpuJpegDecodeAccelerator has-a GpuChannel* (to me, has-a means "owns a"), > but the comment says the opposite. Can you please clarify?
https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:50: // GpuChannels destroy all the GpuJpegDecodeAccelerator that they own when On 2015/06/11 23:28:28, palmer wrote: > This comment feels a bit strange to me; as declared, the code seems to be saying > that GpuJpegDecodeAccelerator has-a GpuChannel* (to me, has-a means "owns a"), > but the comment says the opposite. Can you please clarify? The comment is right. GpuChannel owns GpuJpegDecodeAccelerator. In my understanding, a pointer does not indicate the ownership, scoped_ptr does. Shared ownership is discouraged in the style guide. That leaves us using a plain pointer. https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... File content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... content/common/gpu/media/vaapi_jpeg_decode_accelerator.cc:21: // TODO(kcwu) report error to UMA On 2015/06/12 00:29:16, palmer wrote: > It's probably best to file a bug and mention the bug # here. There is a separate CL to add UMA. https://codereview.chromium.org/1179803002 It has got LGTM and will be merged as soon as this CL goes in.
https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/gpu... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/gpu... content/common/gpu/gpu_messages.h:63: IPC_ENUM_TRAITS_MAX_VALUE(media::JpegDecodeAccelerator::Error, On 2015/06/11 23:28:28, palmer wrote: > It might be a good idea to use IPC_ENUM_TRAITS_MIN_MAX_VALUE, and set the > minimum to INVALID_ARGUMENT. For IPC, the minimum is actually NO_ERROR(0). AcceleratedJpegDecoderHostMsg_DecodeAck is used for both successful and failure cases. https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:160: const AcceleratedJpegDecoderMsg_Decode_Params& params) { On 2015/06/11 23:57:11, jln wrote: > Could you validate all the params members early on here? Notably, could you > check that the "size" members fall into some reasonable range? Done. input_buffer_size is not validated as I have no idea about its reasonable range. https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... File content/common/gpu/media/gpu_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1124423008/diff/240001/content/common/gpu/med... content/common/gpu/media/gpu_jpeg_decode_accelerator.h:50: // GpuChannels destroy all the GpuJpegDecodeAccelerator that they own when On 2015/06/12 02:29:41, wuchengli wrote: > On 2015/06/11 23:28:28, palmer wrote: > > This comment feels a bit strange to me; as declared, the code seems to be > saying > > that GpuJpegDecodeAccelerator has-a GpuChannel* (to me, has-a means "owns a"), > > but the comment says the opposite. Can you please clarify? > The comment is right. GpuChannel owns GpuJpegDecodeAccelerator. In my > understanding, a pointer does not indicate the ownership, scoped_ptr does. > Shared ownership is discouraged in the style guide. That leaves us using > a plain pointer. Yes, as wuchengli said, GpuChannel owns GpuJpegDecodeAccelerator.
@palmer, could you take another look? thanks.
I get that this is a net win for MJPEG decoding, but I'm wondering about the overall, Chrome-wide effect. Many different areas of code, operating on different time-scales, depend on the I/O thread: IPC, disl I/O, network I/O. Anyway, from an IPC security perspective, LGTM. But, we should keep an eye on I/O thread load, especially when we use IPC to drive compute optimizations. (My intuition is that we usually have more raw compute available than IOPS and context switches.)
On Mon, Jun 15, 2015 at 11:18 AM, <palmer@chromium.org> wrote: > I get that this is a net win for MJPEG decoding, but I'm wondering about > the > overall, Chrome-wide effect. Many different areas of code, operating on > different time-scales, depend on the I/O thread: IPC, disl I/O, network > I/O. > > Anyway, from an IPC security perspective, LGTM. > > But, we should keep an eye on I/O thread load, especially when we use IPC > to > drive compute optimizations. (My intuition is that we usually have more raw > compute available than IOPS and context switches.) > I'm not sure I understand the concerns - this CL doesn't run computations on the IO thread - it picks up messages on the IO thread and then posts tasks to the decoder thread. > https://codereview.chromium.org/1124423008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> I'm not sure I understand the concerns - this CL doesn't run computations > on the IO thread - it picks up messages on the IO thread and then posts > tasks to the decoder thread. I didn't say that this CL runs computations on the I/O thread. I was saying that it incurs more IPC as part of a strategy to optimize computation. But, it seems to me that computers usually have lots of compute power available, and that doing more on a thread that is serving lots of different clients might be a net-loss, application-wide.
On Mon, Jun 15, 2015 at 11:59 AM, <palmer@chromium.org> wrote: > I'm not sure I understand the concerns - this CL doesn't run computations >> on the IO thread - it picks up messages on the IO thread and then posts >> tasks to the decoder thread. >> > > I didn't say that this CL runs computations on the I/O thread. I was > saying that > it incurs more IPC as part of a strategy to optimize computation. But, it > seems > to me that computers usually have lots of compute power available, and that > doing more on a thread that is serving lots of different clients might be a > net-loss, application-wide. > This is about being able to use dedicated hardware to do the decoding, which is faster and uses less power. It certainly is not true that "computers usually have lots of compute power available", especially on low-end devices such as Chromebooks. Besides, even if the CPU does have enough power to run the computations at the target frame rate, it's not a reason to stop there because of the power considerations. Being able to use a minimal amount of CPU, reduce operating frequency/voltage or even turn off cores is critical. Lastly, even if the CPU can support running the computations at full frame rate, keeping threads busy doing computation mean that CPU cores aren't available to service threads (such as, for example, the IO thread) in a timely fashion (e.g. needs to wait for the next scheduling time slice), meaning extra latency (or reduced throughput) for other operations. > https://codereview.chromium.org/1124423008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by kcwu@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, piman@chromium.org, wuchengli@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1124423008/#ps260001 (title: "validate params")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124423008/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kcwu@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, wuchengli@chromium.org, palmer@chromium.org, xhwang@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1124423008/#ps280001 (title: "fix compile")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124423008/280001
On 2015/06/15 18:59:05, palmer wrote: > > I'm not sure I understand the concerns - this CL doesn't run computations > > on the IO thread - it picks up messages on the IO thread and then posts > > tasks to the decoder thread. > > I didn't say that this CL runs computations on the I/O thread. I was saying that > it incurs more IPC as part of a strategy to optimize computation. But, it seems > to me that computers usually have lots of compute power available, and that > doing more on a thread that is serving lots of different clients might be a > net-loss, application-wide. Running a 1:1 Hangout takes 70-80% CPU. Enabling effects can reach 95% CPU. There's not a lot of compute power left. Increasing battery life for Hangout is also important for laptops. If Hangouts uses 1080p camera in the future, the improvement will be even more significant.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kcwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, wuchengli@chromium.org, palmer@chromium.org, xhwang@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1124423008/#ps300001 (title: "fix compile for windows & gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124423008/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Ah, just found palmer@ is not listed as gpu_messages.h OWNERS. and I don't understand why it link failed on windows http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...
> Ah, just found palmer@ is not listed as gpu_messages.h OWNERS. Oops, sorry! Fixing that, and I've also asked other security crew members to take a look.
On 2015/06/16 19:13:52, palmer wrote: > > Ah, just found palmer@ is not listed as gpu_messages.h OWNERS. > > Oops, sorry! Fixing that, and I've also asked other security crew members to > take a look. jln@ Can you take a look again? We need owner review for gpu_messages.h. palmer already LGTM this. Let us know if you have the time. If not, please suggest another owner. Thanks.
On 2015/06/18 00:04:30, wuchengli wrote: > On 2015/06/16 19:13:52, palmer wrote: > > > Ah, just found palmer@ is not listed as gpu_messages.h OWNERS. > > > > Oops, sorry! Fixing that, and I've also asked other security crew members to > > take a look. > jln@ Can you take a look again? We need owner review for gpu_messages.h. palmer > already LGTM this. Let us know if you have the time. If not, please suggest > another owner. Thanks. Kuang-che. palmer@ now is an OWNER in content/common/gpu/. You should be able to commit this.
https://chromiumcodereview.appspot.com/1124423008/diff/200001/media/video/jpe... File media/video/jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1124423008/diff/200001/media/video/jpe... media/video/jpeg_decode_accelerator.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/06/11 07:29:15, kcwu wrote: > On 2015/06/10 08:02:45, Pawel Osciak wrote: > > Do we still need this file? > > Done. > Move the destructor to header file. It's okay to inline destructor for this > case, right? Due to compile fail on bot win_chromium_compile_dbg_ng, I added back jpeg_decode_accelerator.cc http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...
The CQ bit was checked by kcwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org, palmer@chromium.org, xhwang@chromium.org, posciak@chromium.org, wuchengli@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1124423008/#ps340001 (title: "add back jpeg_decode_accelerator.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124423008/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by kcwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1124423008/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/2bdb8a4d9f21dd1effcff644f182dff8eba179a3 Cr-Commit-Position: refs/heads/master@{#335008} |
