Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 2 months ago by perkj_chrome Modified:
6 years, 2 months ago Reviewers:
bbudge, tommi (sloooow) - chröme, magjed_chromium CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMove VideoDestinationHandler processing to the IO-thread.
This moves reading video frames from shared memory memory received from the pepper effects plugin to the IO thread.
Also RGBA color conversion is now done on the IO thread.
The purpose of this is to free up processing time on the main render thread. Note that, before this change, the frames from the plugin was posted to the IO thread after the color conversion was done.
BUG=421554
Committed: https://crrev.com/49069e124dc1d57b19ffe87c8d324c16f0a2c861
Cr-Commit-Position: refs/heads/master@{#298885}
Patch Set 1 : Fix lint & added comments. #Patch Set 2 : Rebased #
Total comments: 4
Patch Set 3 : Addressed comments #
Total comments: 7
Patch Set 4 : Addressed comments. #
Total comments: 4
Patch Set 5 : Addressed nit #Patch Set 6 : Rebased #
Created: 6 years, 2 months ago
Messages
Total messages: 27 (9 generated)
Patchset #1 (id:1) has been deleted
perkj@chromium.org changed reviewers: + magjed@chromium.org, tommi@chromium.org
Hej Can you please review?
https://codereview.chromium.org/631903003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.h (right): https://codereview.chromium.org/631903003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.h:47: // video frames to MediaStreamVideoTracks. It implements a method |Bind| to unterminated comment https://codereview.chromium.org/631903003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.h:57: VideoDestinationHandler::FrameWriterCallback Bind(); Maybe 'Bind' is a strange name for this function?
PTAL https://codereview.chromium.org/631903003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.h (right): https://codereview.chromium.org/631903003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.h:47: // video frames to MediaStreamVideoTracks. It implements a method |Bind| to On 2014/10/07 14:07:01, magjed wrote: > unterminated comment Done. https://codereview.chromium.org/631903003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.h:57: VideoDestinationHandler::FrameWriterCallback Bind(); On 2014/10/07 14:07:01, magjed wrote: > Maybe 'Bind' is a strange name for this function? Done.
https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:102: new_frame_callback_, Are you sending a pointer to the callback to the IO thread? It looks like the callback is otherwise only accessed on this thread (thread_checker_) and there's no synchronization, so we would need to send a copy here I think unless there's some magic happening here that I'm missing. Alternatively, could the callback be touched only on the IO thread? https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:114: image_data->Unmap(); should there be a thread check in this function? https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:156: delegate_ = new FrameWriterDelegate(io_message_loop()); initializer list?
Patchset #4 (id:80001) has been deleted
PTAL https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:102: new_frame_callback_, On 2014/10/08 09:11:24, tommi wrote: > Are you sending a pointer to the callback to the IO thread? > It looks like the callback is otherwise only accessed on this thread > (thread_checker_) and there's no synchronization, so we would need to send a > copy here I think unless there's some magic happening here that I'm missing. > > Alternatively, could the callback be touched only on the IO thread? Changed so that StartDeliver post the cb to the io thread. https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:114: image_data->Unmap(); On 2014/10/08 09:11:24, tommi wrote: > should there be a thread check in this function? Done. https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:156: delegate_ = new FrameWriterDelegate(io_message_loop()); On 2014/10/08 09:11:24, tommi wrote: > initializer list? I guess I can but I have a vague memory that you didnt like calling a base class method in an initializer list? io_message_loop()
perkj@chromium.org changed reviewers: + bbudge@chromium.org
Adding bbudge as well Tommi and bbudge, please take a look.
https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/631903003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/video_destination_handler.cc:156: delegate_ = new FrameWriterDelegate(io_message_loop()); On 2014/10/08 12:20:18, perkj wrote: > On 2014/10/08 09:11:24, tommi wrote: > > initializer list? > > I guess I can but I have a vague memory that you didnt like calling a base class > method in an initializer list? io_message_loop() oh, delegate_ is a base class member. If so, leave as is. https://codereview.chromium.org/631903003/diff/100001/content/renderer/media/... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/631903003/diff/100001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:123: const VideoCaptureDeliverFrameCB& frame_callback) { should this be by-value or is it guaranteed to be a new copy of the callback? (I think this should be safe, but my brain isn't working properly right now, so I'm checking to be sure!)
https://codereview.chromium.org/631903003/diff/100001/content/renderer/media/... File content/renderer/media/webrtc/video_destination_handler.cc (right): https://codereview.chromium.org/631903003/diff/100001/content/renderer/media/... content/renderer/media/webrtc/video_destination_handler.cc:123: const VideoCaptureDeliverFrameCB& frame_callback) { On 2014/10/08 14:50:36, tommi wrote: > should this be by-value or is it guaranteed to be a new copy of the callback? (I > think this should be safe, but my brain isn't working properly right now, so I'm > checking to be sure!) Here is an example of code that do the same thing (not written by me :->) https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... callback.h state that callbacks should be passed by reference. https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=41 But it does not explicitly say how they should be used together with bind.
Great. lgtm. I like that we moved the cb completely over to the IO thread.
pepper lgtm w/ 1 nit https://codereview.chromium.org/631903003/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_video_destination_host.cc (right): https://codereview.chromium.org/631903003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_video_destination_host.cc:7: #include <string> nit: this is included in the header now.
https://codereview.chromium.org/631903003/diff/100001/content/renderer/pepper... File content/renderer/pepper/pepper_video_destination_host.cc (right): https://codereview.chromium.org/631903003/diff/100001/content/renderer/pepper... content/renderer/pepper/pepper_video_destination_host.cc:7: #include <string> On 2014/10/08 17:23:23, bbudge wrote: > nit: this is included in the header now. Done.
The CQ bit was checked by perkj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/631903003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/66110) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/70936) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by perkj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/631903003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
The CQ bit was checked by perkj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/631903003/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as 3255c2f7bfc8b4e64dd5d45c167469baa6e4b038
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/49069e124dc1d57b19ffe87c8d324c16f0a2c861 Cr-Commit-Position: refs/heads/master@{#298885} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
