Move VideoDestinationHandler processing to the IO-thread.

Move 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}

[perkj_chrome, 2014-10-07 10:03:49]

Patchset #1 (id:1) has been deleted

[perkj_chrome, 2014-10-07 10:04:51]

perkj@chromium.org changed reviewers:
+ magjed@chromium.org, tommi@chromium.org

[perkj_chrome, 2014-10-07 10:04:51]

Hej
Can you please review?

[magjed_chromium, 2014-10-07 14:07:01]

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?

[perkj_chrome, 2014-10-07 15:39:04]

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.

[tommi (sloooow) - chröme, 2014-10-08 09:11:25]

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?

[perkj_chrome, 2014-10-08 12:20:05]

Patchset #4 (id:80001) has been deleted

[perkj_chrome, 2014-10-08 12:20:19]

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_chrome, 2014-10-08 14:29:57]

perkj@chromium.org changed reviewers:
+ bbudge@chromium.org

[perkj_chrome, 2014-10-08 14:29:58]

Adding bbudge as well

Tommi  and bbudge, please take a look.

[tommi (sloooow) - chröme, 2014-10-08 14:50:36]

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!)

[perkj_chrome, 2014-10-08 15:27:55]

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.

[tommi (sloooow) - chröme, 2014-10-08 15:40:18]

Great. lgtm.  I like that we moved the cb completely over to the IO thread.

[bbudge, 2014-10-08 17:23:24]

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.

[perkj_chrome, 2014-10-08 18:37:37]

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.

[perkj_chrome, 2014-10-08 18:37:42]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-10-08 18:40:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631903003/120001

[commit-bot: I haz the power, 2014-10-08 18:47:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 18:47:49]

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_...)

[perkj_chrome, 2014-10-09 07:45:44]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-10-09 07:46:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631903003/140001

[commit-bot: I haz the power, 2014-10-09 09:10:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 09:10:17]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[perkj_chrome, 2014-10-09 14:05:06]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2014-10-09 14:06:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631903003/140001

[commit-bot: I haz the power, 2014-10-09 16:07:22]

Committed patchset #6 (id:140001) as 3255c2f7bfc8b4e64dd5d45c167469baa6e4b038

[commit-bot: I haz the power, 2014-10-09 16:09:34]

Patchset 6 (id:??) landed as
https://crrev.com/49069e124dc1d57b19ffe87c8d324c16f0a2c861
Cr-Commit-Position: refs/heads/master@{#298885}