Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1132683004:
MJPEG acceleration for video capture, browser part (Closed)
|
Created:
5 years, 7 months ago by kcwu Modified:
5 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org, henryhsu Base URL:
https://chromium.googlesource.com/chromium/src.git@mjpeg-2-gpu Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMJPEG acceleration for video capture
This is the browser part to utilize the acceleration. The acceleration is not enabled yet in this CL.
BUG=335778
TEST=apprtc.appspot.com on intel platform
Committed: https://crrev.com/f2c6c3c19f9729e35822ecd9cdf1bb35390fd0db
Cr-Commit-Position: refs/heads/master@{#336550}
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 22
Patch Set 3 : rebase due to VideoFrame refactoring #Patch Set 4 : #Patch Set 5 : addressed all comments #Patch Set 6 : add flag --enable-accelerated-mjpeg-decode #
Total comments: 8
Patch Set 7 : revert patchset 6; disable by default #Patch Set 8 : rebase (ignore this) #Patch Set 9 : fix rebase #Patch Set 10 : #Patch Set 11 : rebase #Patch Set 12 : fix gpu channel establish #
Total comments: 4
Patch Set 13 : #
Total comments: 50
Patch Set 14 : #Patch Set 15 : gpu_jpeg_decoder renamed to video_capture_gpu_jpeg_decoder; rebase #Patch Set 16 : fix compile for clang/windows/mac #
Total comments: 6
Patch Set 17 : addressed pawel's comments #Patch Set 18 : rebase #Dependent Patchsets: Messages
Total messages: 33 (3 generated)
kcwu@chromium.org changed reviewers: + mcasas@chromium.org, piman@chromium.org, posciak@chromium.org, wuchengli@chromium.org
Please take a look, thanks. @piman for content/ @mcasas for media/video/capture/video_capture_device.h This is split from https://codereview.chromium.org/1016773002/ . This patch depends on https://codereview.chromium.org/1124423008/ and https://codereview.chromium.org/1147693002/
https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:32: : jpeg_thread_("GpuJpegDecoderThread"), Why do we need this thread? It seems all you're doing on the thread is mapping buffers, copying memory and sending IPCs, but even mapping buffers / copying memory you do synchronously. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:80: BrowserGpuChannelHostFactory::instance()->GetGpuChannel(); This is problematic for 2 reasons: - BrowserGpuChannelHostFactory::instance() should not be called from a thread - GetGpuChannel doesn't create a GPU channel (only returns the existing one if any), and may return a lost channel. You need to use EstablishGpuChannel or EstablishGpuChannelSync - it has to be called from the UI thread. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:81: decoder_ = host->CreateJpegDecoder(); You're creating decoder_ on the jpeg_thread_ but destroy it on the embedder thread (where GpuJpegDecoder::~GpuJpegDecoder is called), but GpuJpegDecodeAcceleratorHost is marked as not thread safe. decoder_ is actually destroyed before jpeg_thread_ is destroyed (and joined), so you definitely have a race, where e.g. the bottom half of DecodeCapturedDataOnJpegThread could run concurrently with the destruction. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:173: in_buffer_size > in_shared_memory_->mapped_size()) { If you reuse the shared memory across decodes, it would be useful to do that on the GPU process side to. Mapping the buffer has a cost, which sounds like it could be avoided. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_device_client.cc:232: AsWeakPtr()), If the callback is called from the jpeg decoder thread, passing a WeakPtr is invalid, because it will be dereferenced on the incorrect thread. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_device_client.cc:234: base::Bind(&VideoCaptureDeviceClient::OnError, AsWeakPtr()))); Same here.
https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:32: : jpeg_thread_("GpuJpegDecoderThread"), On 2015/05/18 23:18:30, piman (Very slow to review) wrote: > Why do we need this thread? It seems all you're doing on the thread is mapping > buffers, copying memory and sending IPCs, but even mapping buffers / copying > memory you do synchronously. The main point to have another thread is the capture thread often blocks waiting for camera (in ioctl) and IO thread should only do fast things. My earlier CL was using only capture thread and IO thread, and became complicated.
On Mon, May 25, 2015 at 11:57 AM, <kcwu@chromium.org> wrote: > > > https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): > > > https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/gpu_jpeg_decoder.cc:32: : > jpeg_thread_("GpuJpegDecoderThread"), > On 2015/05/18 23:18:30, piman (Very slow to review) wrote: > >> Why do we need this thread? It seems all you're doing on the thread is >> > mapping > >> buffers, copying memory and sending IPCs, but even mapping buffers / >> > copying > >> memory you do synchronously. >> > > The main point to have another thread is the capture thread often blocks > waiting for camera (in ioctl) and IO thread should only do fast things. > My earlier CL was using only capture thread and IO thread, and became > complicated. > But the capture thread is the one feeding the decoder anyway, so if it's blocked, it can't feed the decoder, even with the thread? See the feedback above why adding this thread makes things compilcated too. > > https://codereview.chromium.org/1132683004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/26 22:44:31, piman (Very slow to review) wrote: > On Mon, May 25, 2015 at 11:57 AM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=kcwu@chromium.org> wrote: > > The main point to have another thread is the capture thread often blocks > > waiting for camera (in ioctl) and IO thread should only do fast things. > > My earlier CL was using only capture thread and IO thread, and became > > complicated. > > > > But the capture thread is the one feeding the decoder anyway, so if it's > blocked, it can't feed the decoder, even with the thread? The capture thread is blocked and thus it cannot be used to receive decode reply (VideoFrameReady). Otherwise VideoFrameRead will become 15fps if captures at 30fps. > See the feedback above why adding this thread makes things compilcated too. The most problems you mentioned still exist if I do GpuJpegDecoder on capture thread or IO thread. Where else is better.... could I do it on UI thread?
On Tue, May 26, 2015 at 7:03 PM, <kcwu@chromium.org> wrote: > On 2015/05/26 22:44:31, piman (Very slow to review) wrote: > >> On Mon, May 25, 2015 at 11:57 AM, >> > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=kcwu@chromium.org> > wrote: > >> > The main point to have another thread is the capture thread often blocks >> > waiting for camera (in ioctl) and IO thread should only do fast things. >> > My earlier CL was using only capture thread and IO thread, and became >> > complicated. >> > >> > > But the capture thread is the one feeding the decoder anyway, so if it's >> blocked, it can't feed the decoder, even with the thread? >> > The capture thread is blocked and thus it cannot be used to receive decode > reply > (VideoFrameReady). Otherwise VideoFrameRead will become 15fps if captures > at > 30fps. > > > > See the feedback above why adding this thread makes things compilcated >> too. >> > The most problems you mentioned still exist if I do GpuJpegDecoder on > capture > thread or IO thread. Where else is better.... could I do it on UI thread? > If what matters is getting the reply, maybe we can add a filter on the IO thread to catch that and forward to the right thread? > > https://codereview.chromium.org/1132683004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Getting closer. Most of the video_* classes look good. Some more comments though. The Signal()-Wait makes me feel uneasy :-S https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:33: jpeg_event_(false, false), jpeg_event( false /* manual reset */, false /* initially_signaled */), ? https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:131: base::Unretained(this), Isn't this a bad idea? https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:139: jpeg_event_.Wait(); I have a few objections/remarks to the wait-signal semaphore construction, beyond the fact that Chromium is supposed to be asynchronous in general. One is that, if VCDeviceClient gets destroyed and has a GpuJpegDecoder Wait()ing, how's that supposed to go about? Maybe we could use TimedWait() ISO Wait() and add some large timeout value, and signal OnError() if appropriate. WDYT? https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:142: Remove empty line. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:158: jpeg_event_.Signal(); The other objection, DecodeCapturedDataOnJpegThread() has three return paths and you need to explicitly Signal() on each. You can make a local class AutoReleaseWaitableEVent { public: AutoReleaseWaitableEvent(base::WaitableEvent& completion) : completion_(completion) {} ~AutoReleaseWaitableEvent() { completion_.Signal() } private: const base::WaitableEvent& completion_; } And locally wrap |jpeg_event_| ? https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:165: // Reserve 2x space to avoid frequently reallocate for initial frames. s/frequently reallocate/frequent reallocations/ https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:166: size_t reserved_size = 2 * in_buffer_size; const https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.h:89: DecodeDoneCB decode_done_cb_; const https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.h:92: ErrorCB error_cb_; const https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.cc:65: base::SharedMemoryHandle handle_; const https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:352: !flip && external_jpeg_decoder_) { I would put |external_jpeg_decoder_| first since is the condition most likely to be false, looking at all Cr platforms; that way we won't need to evaluate the rest, which are more likely to be true. Also, if we have created the |external_jpeg_decoder_|, the pixel format is likely to be true, i.e. video sources usually do not change the created format.
@mcasas, Sorry I forgot to reply earlier. As piman's suggestion, I will attempt to remove jpeg thread again. Maybe the code will be cleaner this time since the code evolved from review comments and we don't have to deal the fallback now.
Addressed some comments. Jpeg thread is removed. The major remain issues are ~GpuJpegDecoder called on the wrong thread. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:80: BrowserGpuChannelHostFactory::instance()->GetGpuChannel(); On 2015/05/18 23:18:30, piman (Very slow to review) wrote: > This is problematic for 2 reasons: > - BrowserGpuChannelHostFactory::instance() should not be called from a thread > - GetGpuChannel doesn't create a GPU channel (only returns the existing one if > any), and may return a lost channel. You need to use EstablishGpuChannel or > EstablishGpuChannelSync - it has to be called from the UI thread. Done. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:173: in_buffer_size > in_shared_memory_->mapped_size()) { On 2015/05/18 23:18:30, piman (Very slow to review) wrote: > If you reuse the shared memory across decodes, it would be useful to do that on > the GPU process side to. Mapping the buffer has a cost, which sounds like it > could be avoided. I feel mapping is acceptable since VDA is doing so. This needs to change the interface a lot, so I prefer to revisit this issue in another CL. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:33: jpeg_event_(false, false), On 2015/05/27 18:08:43, mcasas wrote: > jpeg_event( false /* manual reset */, false /* initially_signaled */), ? Done. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:142: On 2015/05/27 18:08:43, mcasas wrote: > Remove empty line. Done. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:158: jpeg_event_.Signal(); On 2015/05/27 18:08:43, mcasas wrote: > The other objection, DecodeCapturedDataOnJpegThread() has > three return paths and you need to explicitly Signal() on each. > You can make a local > > class AutoReleaseWaitableEVent { > public: > AutoReleaseWaitableEvent(base::WaitableEvent& completion) > : completion_(completion) {} > ~AutoReleaseWaitableEvent() { > completion_.Signal() > } > > private: > const base::WaitableEvent& completion_; > } > > And locally wrap |jpeg_event_| ? Acknowledged. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:165: // Reserve 2x space to avoid frequently reallocate for initial frames. On 2015/05/27 18:08:43, mcasas wrote: > s/frequently reallocate/frequent reallocations/ Done. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:166: size_t reserved_size = 2 * in_buffer_size; On 2015/05/27 18:08:43, mcasas wrote: > const Done. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.h:89: DecodeDoneCB decode_done_cb_; On 2015/05/27 18:08:43, mcasas wrote: > const Done. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.h:92: ErrorCB error_cb_; On 2015/05/27 18:08:43, mcasas wrote: > const Done. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right): https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.cc:65: base::SharedMemoryHandle handle_; On 2015/05/27 18:08:43, mcasas wrote: > const Done. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:352: !flip && external_jpeg_decoder_) { On 2015/05/27 18:08:43, mcasas wrote: > I would put |external_jpeg_decoder_| first since is the > condition most likely to be false, looking at all Cr > platforms; that way we won't need to evaluate the rest, > which are more likely to be true. > > Also, if we have created the |external_jpeg_decoder_|, > the pixel format is likely to be true, i.e. video sources > usually do not change the created format. Done.
All comments are addressed. PTAL. I will update new performance numbers tomorrow. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:81: decoder_ = host->CreateJpegDecoder(); On 2015/05/18 23:18:30, piman (Very slow to review) wrote: > You're creating decoder_ on the jpeg_thread_ but destroy it on the embedder > thread (where GpuJpegDecoder::~GpuJpegDecoder is called), but > GpuJpegDecodeAcceleratorHost is marked as not thread safe. > > decoder_ is actually destroyed before jpeg_thread_ is destroyed (and joined), so > you definitely have a race, where e.g. the bottom half of > DecodeCapturedDataOnJpegThread could run concurrently with the destruction. Done. I found ~VideoCaptureDeviceClient is always called on device thread. So there is no problem. I make it explicit now. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_device_client.cc:232: AsWeakPtr()), On 2015/05/18 23:18:30, piman (Very slow to review) wrote: > If the callback is called from the jpeg decoder thread, passing a WeakPtr is > invalid, because it will be dereferenced on the incorrect thread. Done. https://codereview.chromium.org/1132683004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/video_capture_device_client.cc:234: base::Bind(&VideoCaptureDeviceClient::OnError, AsWeakPtr()))); On 2015/05/18 23:18:30, piman (Very slow to review) wrote: > Same here. Changed to base::Unretained(this) since VideoCaptureDeviceClient outlives GpuJpegDecoder. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:131: base::Unretained(this), On 2015/05/27 18:08:43, mcasas wrote: > Isn't this a bad idea? jpeg thread is removed. https://codereview.chromium.org/1132683004/diff/20001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:139: jpeg_event_.Wait(); On 2015/05/27 18:08:43, mcasas wrote: > I have a few objections/remarks to the wait-signal semaphore > construction, beyond the fact that Chromium is supposed to > be asynchronous in general. > > One is that, if VCDeviceClient gets destroyed and has a > GpuJpegDecoder Wait()ing, how's that supposed to go about? > Maybe we could use TimedWait() ISO Wait() and add some > large timeout value, and signal OnError() if appropriate. > WDYT? jpeg thread is removed.
also added flag --enable-accelerated-mjpeg-decode
Some comments, but my review in https://codereview.chromium.org/1124423008/ indicated there are still some races there (in particular the tasks posted on the IO thread may outlive the classes involved, making some Unretained uses unsafe), so I will delay the final review until we got consensus on that one. https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:74: base::Unretained(BrowserGpuChannelHostFactory::instance()), BrowserGpuChannelHostFactory is a UI thread class. You should consider this to be invalid to call from another thread (and Unretained could be unsafe). Instead you should post a task to the UI thread to a free function that can then do BrowserGpuChannelHostFactory::instance()->EstablishGpuChannel(...) https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:78: gpu_channel_event_.Wait(); Which thread is this called on? It seems like this could cause a deadlock on termination when the UI thread joins this thread. I would like to say that a way to make this work would be to establish the channel on the UI thread before even creating this class (it's done anyway for the UI), and inject it in the constructor (or in Initialize), but I don't know how all this flows. If I understand correctly, this gets called when you start receiving encoded capture packets. So maybe a way is to post the task at that point, and delay decoding of the packets (keeping them in a queue or discarding) until the reply/establish is done. https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:235: base::Bind(&VideoCaptureDeviceClient::OnIncomingCapturedVideoFrame, Maybe you can bind to DoIncomingCapturedVideoFrameOnIOThread directly to avoid the extra PostTask? That way you don't need Unretained(this) - you can pass the controller_ WeakPtr directly. https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:238: base::Bind(&VideoCaptureDeviceClient::OnError, Same here, bind to DoErrorOnIOThread?
Kuang-che. Please don't add new big changes in the middle of the review. It's harder for the reviewer to compare the delta. Also, the flag --enable-accelerated-mjpeg-decode will need three more OWNERS approval. Please revert it and make it a separate CL so you can land this earlier.
In summary, there are total 4 CLs for MJPEG acceleration. They are - https://codereview.chromium.org/1124423008/ GPU and IPC part - https://codereview.chromium.org/1165943008/ GPU host part - https://codereview.chromium.org/1132683004/ browser client to use <-- this one - https://codereview.chromium.org/1153163003/ flag to enable
Sorry late to revise. PTAL. https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:74: base::Unretained(BrowserGpuChannelHostFactory::instance()), On 2015/06/05 23:07:51, piman (Very slow to review) wrote: > BrowserGpuChannelHostFactory is a UI thread class. You should consider this to > be invalid to call from another thread (and Unretained could be unsafe). > > Instead you should post a task to the UI thread to a free function that can then > do BrowserGpuChannelHostFactory::instance()->EstablishGpuChannel(...) Done. https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:78: gpu_channel_event_.Wait(); On 2015/06/05 23:07:51, piman (Very slow to review) wrote: > Which thread is this called on? It seems like this could cause a deadlock on > termination when the UI thread joins this thread. > > I would like to say that a way to make this work would be to establish the > channel on the UI thread before even creating this class (it's done anyway for > the UI), and inject it in the constructor (or in Initialize), but I don't know > how all this flows. If I understand correctly, this gets called when you start > receiving encoded capture packets. So maybe a way is to post the task at that > point, and delay decoding of the packets (keeping them in a queue or discarding) > until the reply/establish is done. Done. I added a function ReadyToDecode, which indicates the establish is done. Before that, it still uses software decode, so no frames are drop. https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:235: base::Bind(&VideoCaptureDeviceClient::OnIncomingCapturedVideoFrame, On 2015/06/05 23:07:51, piman (Very slow to review) wrote: > Maybe you can bind to DoIncomingCapturedVideoFrameOnIOThread directly to avoid > the extra PostTask? > > That way you don't need Unretained(this) - you can pass the controller_ WeakPtr > directly. Done. https://codereview.chromium.org/1132683004/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:238: base::Bind(&VideoCaptureDeviceClient::OnError, On 2015/06/05 23:07:51, piman (Very slow to review) wrote: > Same here, bind to DoErrorOnIOThread? Not exactly. VideoCaptureDeviceClient::OnError = OnLog + DoErrorOnIOThread. Since error callback is not on the critical path, I'd prefer less code with the cost of extra PostTask. What do you think?
lgtm https://codereview.chromium.org/1132683004/diff/220001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/220001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:241: base::Unretained(this)))); nit: can you add a comment about why Unretained is safe?
https://codereview.chromium.org/1132683004/diff/220001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/220001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:241: base::Unretained(this)))); On 2015/06/20 00:33:57, piman (Very slow to review) wrote: > nit: can you add a comment about why Unretained is safe? line 233 does. Do you want me to explain it further?
https://codereview.chromium.org/1132683004/diff/220001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/220001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:241: base::Unretained(this)))); On 2015/06/20 00:47:08, kcwu wrote: > On 2015/06/20 00:33:57, piman (Very slow to review) wrote: > > nit: can you add a comment about why Unretained is safe? > > line 233 does. Do you want me to explain it further? Oh, missed that. I guess that's fine... the main thing is that callbacks happen on the IO thread, so it's not necessarily obvious why they don't happen even after the external_jpeg_decoder_ is destroyed (there's an explicit wait in the GpuJpegDecodeAcceleratorHost). Maybe worth mentioning in the GpuJpegDecoder class? The callbacks are called on the IO thread but never after the GpuJpegDecoder is destroyed?
https://codereview.chromium.org/1132683004/diff/220001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/220001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:241: base::Unretained(this)))); On 2015/06/20 00:59:26, piman (Very slow to review) wrote: > On 2015/06/20 00:47:08, kcwu wrote: > > On 2015/06/20 00:33:57, piman (Very slow to review) wrote: > > > nit: can you add a comment about why Unretained is safe? > > > > line 233 does. Do you want me to explain it further? > > Oh, missed that. I guess that's fine... the main thing is that callbacks happen > on the IO thread, so it's not necessarily obvious why they don't happen even > after the external_jpeg_decoder_ is destroyed (there's an explicit wait in the > GpuJpegDecodeAcceleratorHost). Maybe worth mentioning in the GpuJpegDecoder > class? The callbacks are called on the IO thread but never after the > GpuJpegDecoder is destroyed? Done.
Please also update the CL title/description (this is no longer related to VAAPI). https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:25: // TODO(kcwu): move this information to GpuInfo. Please add a crbug for this. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:26: return false; Are you planning to use a flag here for now? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:47: // must be released after |decoder_| is been destroyed. s/is/has/ https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:117: LOG(ERROR) << "Unexpected bitstream_buffer_id " << bitstream_buffer_id Should we just store the id, instead of the whole in_buffer_, if we are not using it? Or even just use next_bitstream_buffer_id_-1 if we don't allow more than one decode? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:120: } Should we clear in_buffer_ here? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:133: LOG(ERROR) << "Decode error, bitstream_buffer_id=" << bitstream_buffer_id; Should we print error here? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:139: base::StringPrintf("Decode error, bitstream_buffer_id=%d, error=%d", s/Decode/JPEG Decode/ https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:157: // (say, if decoding time is longer than 16ms for 60fps 4k image) I'd remove this, since 16ms would probably be too slow anyway, as decode is just a part of the pipeline? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:166: // Enlarge input buffer if necessary. Why do we need a larger input buffer than the input data? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:191: media::VideoFrame::I420, // format VideoCaptureFormat has a pixel_format member. Could we use that instead (and error out on something not supported)? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:195: static_cast<uint8*>(out_buffer->data()), // data s/uint8/uin8_t/ https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:199: base::TimeDelta()); // timestamp Should we use timestamp from this method's arguments? Is the timestamp propagated properly right now to the client? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:200: DCHECK(out_frame.get()); I think we should handle this, instead of just DCHECKing. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:17: #include "media/video/capture/video_capture_device.h" Would it be enough to declare media::VideoCaptureDevice::Client::Buffer? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:32: // on the same thread. Should we mention explicitly that Client methods should be called on Browser IO thread? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:33: class CONTENT_EXPORT GpuJpegDecoder Since this is a Capture-specific class, I'd prefer renaming it to indicate that. Otherwise this looks like a general GpuJpegDecoder that could be reused for other uses, which it is not. Perhaps GpuJpegDecoderForVideoCapture? Or maybe even have a subclass adapter that handles the Capture-specific parts? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:59: bool ReadyToDecode() { return decoder_; } Should this DCHECK(CalledOnValidThread()) ? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:63: const uint8* data, s/uint8/uint8_t/ ? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:76: // Initialization helper, to establish GPU channel Nit: dot at the end of sentences please. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:99: base::Lock lock_; Should we just trampoline NotifyError() and VideoFrameReady() to the main thread this class runs on (same as Decode()) and eliminate the lock entirely? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:114: media::BitstreamBuffer in_buffer_; Should we say this is the in_buffer_ currently being decoded? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:230: !external_jpeg_decoder_initialized_) { Perhaps instead: if (format ==... && GpuJpegDecoder::Supported()) { if (!external_jpeg_decoder_) { // initialize... } } https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:240: // TODO(kcwu): fallback to software decode if error. Please add a crbug.
(quick reply first) https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:26: return false; On 2015/06/22 08:51:22, Pawel Osciak wrote: > Are you planning to use a flag here for now? Yes, there is another CL https://codereview.chromium.org/1153163003/ https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:157: // (say, if decoding time is longer than 16ms for 60fps 4k image) On 2015/06/22 08:51:23, Pawel Osciak wrote: > I'd remove this, since 16ms would probably be too slow anyway, as decode is just > a part of the pipeline? Do you mean remove only line 157 or the whole TODO? Maybe the throughput is enough, just high latency, so it is probably okay. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:166: // Enlarge input buffer if necessary. On 2015/06/22 08:51:22, Pawel Osciak wrote: > Why do we need a larger input buffer than the input data? We are reusing the shared memory and want to avoid reallocations (as comment in line 169). Does this answer your question? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:191: media::VideoFrame::I420, // format On 2015/06/22 08:51:23, Pawel Osciak wrote: > VideoCaptureFormat has a pixel_format member. Could we use that instead (and > error out on something not supported)? Do you mean the argument |frame_format| ? If so, no, |frame_format.pixel_format| is media::PIXEL_FORMAT_MJPEG https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:199: base::TimeDelta()); // timestamp On 2015/06/22 08:51:23, Pawel Osciak wrote: > Should we use timestamp from this method's arguments? Is the timestamp > propagated properly right now to the client? The timestamp is propagated separatedly. VideoFrame.timestamp is not used. And their type mismatch (one is TimeDelta, the other is TimeTicks) https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:33: class CONTENT_EXPORT GpuJpegDecoder On 2015/06/22 08:51:23, Pawel Osciak wrote: > Since this is a Capture-specific class, I'd prefer renaming it to indicate that. > Otherwise this looks like a general GpuJpegDecoder that could be reused for > other uses, which it is not. Perhaps GpuJpegDecoderForVideoCapture? Or maybe > even have a subclass adapter that handles the Capture-specific parts? I was thinking it's an adapter in the other direction and named it GpuJpegDecodeAcceleratorAdapter and wucheng suggested current one. @wuchengli, what do you think? https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:99: base::Lock lock_; On 2015/06/22 08:51:23, Pawel Osciak wrote: > Should we just trampoline NotifyError() and VideoFrameReady() to the main thread > this class runs on (same as Decode()) and eliminate the lock entirely? They cannot be on the device thread. The device thread often blocks on waiting ioctl for next frame. If they are trampolined to the device thread, the capture fps becomes half (15fps) because the ready event blocked by next incoming frame.
@mcasas, please advise how to obtain SharedMemoryHandle from media::VideoCaptureDevice::Client::Buffer. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:188: base::SharedMemoryHandle out_handle(out_buffer->AsPlatformFile(), true); I found AsPlatformFile() become only available on OS_POSIX recently crrev.com/335139. @mcasas, what do you suggest to get SharedMemoryHandle on non-posix platform?
https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:25: // TODO(kcwu): move this information to GpuInfo. On 2015/06/22 08:51:23, Pawel Osciak wrote: > Please add a crbug for this. Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:47: // must be released after |decoder_| is been destroyed. On 2015/06/22 08:51:22, Pawel Osciak wrote: > s/is/has/ Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:117: LOG(ERROR) << "Unexpected bitstream_buffer_id " << bitstream_buffer_id On 2015/06/22 08:51:23, Pawel Osciak wrote: > Should we just store the id, instead of the whole in_buffer_, if we are not > using it? Or even just use next_bitstream_buffer_id_-1 if we don't allow more > than one decode? Replace in_buffer_ by in_buffer_id_. I don't use next_bitstream_buffer_id_-1 to avoid handling the overflow case. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:120: } On 2015/06/22 08:51:22, Pawel Osciak wrote: > Should we clear in_buffer_ here? Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:133: LOG(ERROR) << "Decode error, bitstream_buffer_id=" << bitstream_buffer_id; On 2015/06/22 08:51:23, Pawel Osciak wrote: > Should we print error here? Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:139: base::StringPrintf("Decode error, bitstream_buffer_id=%d, error=%d", On 2015/06/22 08:51:23, Pawel Osciak wrote: > s/Decode/JPEG Decode/ Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:195: static_cast<uint8*>(out_buffer->data()), // data On 2015/06/22 08:51:23, Pawel Osciak wrote: > s/uint8/uin8_t/ Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:200: DCHECK(out_frame.get()); On 2015/06/22 08:51:23, Pawel Osciak wrote: > I think we should handle this, instead of just DCHECKing. Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.h (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:17: #include "media/video/capture/video_capture_device.h" On 2015/06/22 08:51:23, Pawel Osciak wrote: > Would it be enough to declare media::VideoCaptureDevice::Client::Buffer? AFAIK, there is no way to forward declare an inner class. http://stackoverflow.com/questions/1021793/ https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:32: // on the same thread. On 2015/06/22 08:51:23, Pawel Osciak wrote: > Should we mention explicitly that Client methods should be called on Browser IO > thread? Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:33: class CONTENT_EXPORT GpuJpegDecoder On 2015/06/22 09:18:48, kcwu wrote: > On 2015/06/22 08:51:23, Pawel Osciak wrote: > > Since this is a Capture-specific class, I'd prefer renaming it to indicate > that. > > Otherwise this looks like a general GpuJpegDecoder that could be reused for > > other uses, which it is not. Perhaps GpuJpegDecoderForVideoCapture? Or maybe > > even have a subclass adapter that handles the Capture-specific parts? > > I was thinking it's an adapter in the other direction and named it > GpuJpegDecodeAcceleratorAdapter and wucheng suggested current one. > @wuchengli, what do you think? wucheng has flaky network and we chatted off-line. He said he prefer video_capture_gpu_jpeg_decoder among video_capture_jpeg_decoder, video_capture_gpu_jpeg_decoder, and gpu_jpeg_decoder. He also mentioned this class should be under renderer_host/media. If there is no objection, I will rename this class to VideoCaptureGpuJpegDecoder and move the file to renderer_host/media tomorrow. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:59: bool ReadyToDecode() { return decoder_; } On 2015/06/22 08:51:23, Pawel Osciak wrote: > Should this DCHECK(CalledOnValidThread()) ? Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:63: const uint8* data, On 2015/06/22 08:51:23, Pawel Osciak wrote: > s/uint8/uint8_t/ ? Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:76: // Initialization helper, to establish GPU channel On 2015/06/22 08:51:23, Pawel Osciak wrote: > Nit: dot at the end of sentences please. Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.h:114: media::BitstreamBuffer in_buffer_; On 2015/06/22 08:51:23, Pawel Osciak wrote: > Should we say this is the in_buffer_ currently being decoded? Done. (applied to in_buffer_id_) https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:230: !external_jpeg_decoder_initialized_) { On 2015/06/22 08:51:23, Pawel Osciak wrote: > Perhaps instead: > > if (format ==... && GpuJpegDecoder::Supported()) { > if (!external_jpeg_decoder_) { > // initialize... > } > } Done. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:240: // TODO(kcwu): fallback to software decode if error. On 2015/06/22 08:51:23, Pawel Osciak wrote: > Please add a crbug. Done.
https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:188: base::SharedMemoryHandle out_handle(out_buffer->AsPlatformFile(), true); On 2015/06/22 12:59:35, kcwu wrote: > I found AsPlatformFile() become only available on OS_POSIX recently > crrev.com/335139. > @mcasas, what do you suggest to get SharedMemoryHandle on non-posix platform? mcasas had no time to reply this week. Since other platforms do not support HW JPEG decode anyway, let's add a #if defined(OS_POSIX) and merge the CL.
Pawel, PTAL. thanks. https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1132683004/diff/240001/content/browser/render... content/browser/renderer_host/gpu_jpeg_decoder.cc:188: base::SharedMemoryHandle out_handle(out_buffer->AsPlatformFile(), true); On 2015/06/25 07:42:27, wuchengli wrote: > On 2015/06/22 12:59:35, kcwu wrote: > > I found AsPlatformFile() become only available on OS_POSIX recently > > crrev.com/335139. > > @mcasas, what do you suggest to get SharedMemoryHandle on non-posix platform? > mcasas had no time to reply this week. Since other platforms do not support > HW JPEG decode anyway, let's add a #if defined(OS_POSIX) and merge the CL. Done.
lgtm % nits https://codereview.chromium.org/1132683004/diff/300001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:214: external_jpeg_decoder_.reset(); Not needed? https://codereview.chromium.org/1132683004/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:361: frame_format.pixel_format == media::PIXEL_FORMAT_MJPEG && rotation == 0 && Could we decode in HW and rotate afterwards using libyuv? https://codereview.chromium.org/1132683004/diff/300001/content/common/gpu/gpu... File content/common/gpu/gpu_process_launch_causes.h (right): https://codereview.chromium.org/1132683004/diff/300001/content/common/gpu/gpu... content/common/gpu/gpu_process_launch_causes.h:11: // TODO(kcwu) where is new location of this file?? Perhaps we should file a bug instead of adding this comment?
https://codereview.chromium.org/1132683004/diff/300001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/1132683004/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:214: external_jpeg_decoder_.reset(); On 2015/06/29 06:18:39, Pawel Osciak wrote: > Not needed? Replaced by a comment to explain the thread constraint. https://codereview.chromium.org/1132683004/diff/300001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:361: frame_format.pixel_format == media::PIXEL_FORMAT_MJPEG && rotation == 0 && On 2015/06/29 06:18:39, Pawel Osciak wrote: > Could we decode in HW and rotate afterwards using libyuv? Yes, however I'd prefer do it in another CL. https://codereview.chromium.org/1132683004/diff/300001/content/common/gpu/gpu... File content/common/gpu/gpu_process_launch_causes.h (right): https://codereview.chromium.org/1132683004/diff/300001/content/common/gpu/gpu... content/common/gpu/gpu_process_launch_causes.h:11: // TODO(kcwu) where is new location of this file?? On 2015/06/29 06:18:39, Pawel Osciak wrote: > Perhaps we should file a bug instead of adding this comment? I found and updated the location in the comment.
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, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1132683004/#ps340001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1132683004/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/f2c6c3c19f9729e35822ecd9cdf1bb35390fd0db Cr-Commit-Position: refs/heads/master@{#336550} |
