[chrome.displaySource][WiFi Display] Media pipeline infrastructure

[chrome.displaySource][WiFi Display] Media pipeline infrastructure

This patch introduces essential classes and interfaces for processing WiFi Display
session media: interface for H.264 video encoder (WiFiDisplayVideoEncoder),
an object coordinating all the media processing stages (WiFiDisplayMediaPipeline),
an auxiliary video capturer class (WiFiDisplayVideoSink).

Media is processed out of the main renderer thread (on IO thread).

WiFiDisplayMediaPacketizer initialization code is based on patch from
Eero Hakkinen <eero.hakkinen@intel.com>;.

BUG=242107
Committed: https://crrev.com/61dfa7763344c32d5477c4a97f4417fae7fd7e9b
Cr-Commit-Position: refs/heads/master@{#386423}

[Mikhail, 2016-04-05 16:59:07]

Description was changed from

==========
[chrome.displaySource][WiFi Display] Media pipeline infrastructure

This patch introduces essential classes and interfaces for processing WiFi
Display
session media: interface for H.264 video encoder (WiFiDisplayVideoEncoder),
an object coordinating all the media processing stages
(WiFiDisplayMediaPipeline),
an auxiliary video capturer class (WiFiDisplayVideoSink).

Media is processed out of the main renderer thread (on IO thread).

WiFiDisplayMediaPacketizer initialization code is based on patch from
Eero Hakkinen <eero.hakkinen@intel.com>.

BUG=242107
==========

to

==========
[chrome.displaySource][WiFi Display] Media pipeline infrastructure

This patch introduces essential classes and interfaces for processing WiFi
Display
session media: interface for H.264 video encoder (WiFiDisplayVideoEncoder),
an object coordinating all the media processing stages
(WiFiDisplayMediaPipeline),
an auxiliary video capturer class (WiFiDisplayVideoSink).

Media is processed out of the main renderer thread (on IO thread).

WiFiDisplayMediaPacketizer initialization code is based on patch from
Eero Hakkinen <eero.hakkinen@intel.com>.

BUG=242107
==========

[Mikhail, 2016-04-05 16:59:07]

mikhail.pozdnyakov@intel.com changed reviewers:
+ asargent@chromium.org

[Mikhail, 2016-04-05 17:01:17]

Please take a look.

[shalamov, 2016-04-06 07:48:48]

alexander.shalamov@intel.com changed reviewers:
+ alexander.shalamov@intel.com

[shalamov, 2016-04-06 07:48:48]

https://codereview.chromium.org/1860083002/diff/1/extensions/renderer/api/dis...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc
(right):

https://codereview.chromium.org/1860083002/diff/1/extensions/renderer/api/dis...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc:366:
return base::RandBytesAsString(8);

I think session id should be unique during the session. Otherwise, there would
be issues with RTSP message exchange. Maybe you can generate unique id on first
call, then cache it and reset on teardown.

[Mikhail, 2016-04-06 09:40:59]

https://codereview.chromium.org/1860083002/diff/1/extensions/renderer/api/dis...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc
(right):

https://codereview.chromium.org/1860083002/diff/1/extensions/renderer/api/dis...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc:366:
return base::RandBytesAsString(8);
On 2016/04/06 07:48:48, shalamov wrote:
> 
> I think session id should be unique during the session. Otherwise, there would
> be issues with RTSP message exchange. Maybe you can generate unique id on
first
> call, then cache it and reset on teardown.
Fixed, ptal

[shalamov, 2016-04-06 10:08:34]

On 2016/04/06 09:40:59, Mikhail wrote:
>
https://codereview.chromium.org/1860083002/diff/1/extensions/renderer/api/dis...
> File
>
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc
> (right):
> 
>
https://codereview.chromium.org/1860083002/diff/1/extensions/renderer/api/dis...
>
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc:366:
> return base::RandBytesAsString(8);
> On 2016/04/06 07:48:48, shalamov wrote:
> > 
> > I think session id should be unique during the session. Otherwise, there
would
> > be issues with RTSP message exchange. Maybe you can generate unique id on
> first
> > call, then cache it and reset on teardown.
> Fixed, ptal

lgtm

[asargent_no_longer_on_chrome, 2016-04-06 20:53:21]

A couple of minor comments.

Also one general question: what's the plan for tests of the newly added
functionality?

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/DEPS
File extensions/renderer/DEPS (right):

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/DEP...
extensions/renderer/DEPS:7: "+media",
can this be restricted to just media/base?

Also, you should make sure to get someone from media/OWNERS to ok the added
dependency.

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc
(right):

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc:296:
DCHECK(shm) << "Failed to allocate shared memory";
Maybe this should just be a CHECK?

Alternately, maybe you should consider gracefully handling the error? I guess it
depends on whether it's possible to fail in a more user-friendly way than just
having the renderer process die. 

I took a quick glance and saw a bunch of consumers of
HostAllocateSharedMemoryBuffer doing graceful error handling, which makes me
wonder if this might be more likely than a standard allocation (of reasonable
size) to fail. 

Does code elsewhere do any sanity checking on the value you're passing as |size|
here?

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.h
(right):

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.h:13:
#include "base/memory/scoped_ptr.h"
nit: per a very recent announcement on chromium-dev@chromium.org list, we are
supposed to start using std::unique_ptr instead of base::scoped_ptr in all new
code (a mass conversion of existing code is going to happen at some point as
well).

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_pipeline.h
(right):

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_pipeline.h:13:
#include "base/memory/scoped_ptr.h"
unique_ptr

[Mikhail, 2016-04-07 09:05:47]

mikhail.pozdnyakov@intel.com changed reviewers:
+ ddorwin@chromium.org

[Mikhail, 2016-04-07 09:07:26]

Thanks for your comments!

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/DEPS
File extensions/renderer/DEPS (right):

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/DEP...
extensions/renderer/DEPS:7: "+media",
On 2016/04/06 20:53:21, Antony Sargent wrote:
> can this be restricted to just media/base?
> 
We also need media/video (and will need media/audio soon) but let's make it
granular indeed.
> Also, you should make sure to get someone from media/OWNERS to ok the added
> dependency. 
Done.

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc
(right):

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc:296:
DCHECK(shm) << "Failed to allocate shared memory";
On 2016/04/06 20:53:21, Antony Sargent wrote:
> Maybe this should just be a CHECK?
> 
> Alternately, maybe you should consider gracefully handling the error? I guess
it
> depends on whether it's possible to fail in a more user-friendly way than just
> having the renderer process die. 
> 
> I took a quick glance and saw a bunch of consumers of
> HostAllocateSharedMemoryBuffer doing graceful error handling, which makes me
> wonder if this might be more likely than a standard allocation (of reasonable
> size) to fail. 
> 
> Does code elsewhere do any sanity checking on the value you're passing as
|size|
> here?

We can just pass nullptr to encoder that will be able to report an error in a
graceful way via error callback.

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.h
(right):

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.h:13:
#include "base/memory/scoped_ptr.h"
On 2016/04/06 20:53:21, Antony Sargent wrote:
> nit: per a very recent announcement on mailto:chromium-dev@chromium.org list,
we are
> supposed to start using std::unique_ptr instead of base::scoped_ptr in all new
> code (a mass conversion of existing code is going to happen at some point as
> well). 

Thanks for commenting on this, and it's great that more standard types are to be
used now.

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_pipeline.h
(right):

https://codereview.chromium.org/1860083002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_pipeline.h:13:
#include "base/memory/scoped_ptr.h"
On 2016/04/06 20:53:21, Antony Sargent wrote:
> unique_ptr

Done.

[Mikhail, 2016-04-07 09:11:38]

On 2016/04/06 20:53:21, Antony Sargent wrote:
> A couple of minor comments.
> 
> Also one general question: what's the plan for tests of the newly added
> functionality?

The media pipeline elements (encoders and packetizers) are to be covered with
unit tests.
The whole media pipeline is to be tested in browser tests (verify that packets
are delivered after successful RTSP message exchange).

[Mikhail, 2016-04-07 09:14:27]

@ddorwin could you pls take a look at DEPS file changes?

[ddorwin, 2016-04-08 17:41:30]

ddorwin@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[ddorwin, 2016-04-08 17:41:31]

+dalecurtis for thoughts on DEPS and BindToCurrentLoop() use.

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEPS
File extensions/renderer/DEPS (right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEP...
extensions/renderer/DEPS:7: "+media/base",
It seems that we shouldn't give all extensions code access to these files. You
can add a new DEPS file exactly where you need it.

OTOH, these two are already in content/public/renderer/DEPS.

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc
(right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc:325:
media::BindToCurrentLoop(base::Bind(&CreateVideoEncodeMemory));
Why is media::BindToCurrentLoop being used outside media/ for a non-media
callback?

There are 7 cases outside */media/* directories today [1]. If this is generally
useful, maybe it should be in src/base/ instead.

[1]
https://code.google.com/p/chromium/codesearch#search/&q=BindToCurrentLoop%20-...

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc:326:
video_encoder_parameters_.vea_create_callback =
vea_create_callback is not used anywhere.

VEA has no SW fallback (you probably wouldn't want to use this feature in that
case anyway). Do you correctly detect/handle the lack of a decoder?

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.cc
(right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.cc:23:
encoded_callback_ = media::BindToCurrentLoop(encoded_callback);
ditto

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.h
(right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.h:80:
const scoped_refptr<media::VideoFrame>& video_frame,
ICWYU: media/base/video_frame.h

[DaleCurtis, 2016-04-08 17:44:40]

https://bugs.chromium.org/p/chromium/issues/detail?id=432381

[DaleCurtis, 2016-04-08 17:53:59]

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEPS
File extensions/renderer/DEPS (right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEP...
extensions/renderer/DEPS:7: "+media/base",
On 2016/04/08 at 17:41:31, ddorwin wrote:
> It seems that we shouldn't give all extensions code access to these files. You
can add a new DEPS file exactly where you need it.
> 
> OTOH, these two are already in content/public/renderer/DEPS.

Whoops missed this reply, DEPS should always be narrowly scoped. So add this to
the same directory as the rest as these files.

[ddorwin, 2016-04-08 17:56:57]

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEPS
File extensions/renderer/DEPS (right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEP...
extensions/renderer/DEPS:7: "+media/base",
In the new location, add a TODO to remove this one when https://crbug.com/432381
is addressed. That is the only reason it's required currently.

[Mikhail, 2016-04-08 18:28:58]

Thanks for having a look!

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEPS
File extensions/renderer/DEPS (right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEP...
extensions/renderer/DEPS:7: "+media/base",
On 2016/04/08 17:41:31, ddorwin wrote:
> It seems that we shouldn't give all extensions code access to these files. You
> can add a new DEPS file exactly where you need it.
Thanks for the proposal, I'll create a new DEPS in the following patch
> 
> OTOH, these two are already in content/public/renderer/DEPS.

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc
(right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc:325:
media::BindToCurrentLoop(base::Bind(&CreateVideoEncodeMemory));
On 2016/04/08 17:41:31, ddorwin wrote:
> Why is media::BindToCurrentLoop being used outside media/ for a non-media
> callback?

IMO this is convenient and quite generic feature which I would like to see
within 'base' namespace. Is it supposed to be used for media methods only?

> 
> There are 7 cases outside */media/* directories today [1]. If this is
generally
> useful, maybe it should be in src/base/ instead.
I would vote for it :)

> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=BindToCurrentLoop%20-...

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_media_manager.cc:326:
video_encoder_parameters_.vea_create_callback =
On 2016/04/08 17:41:31, ddorwin wrote:
> vea_create_callback is not used anywhere.
> 
> VEA has no SW fallback (you probably wouldn't want to use this feature in that
> case anyway). Do you correctly detect/handle the lack of a decoder?

This functionality is in the coming patches we'll use a SW OpenH264-based
encoder if VEA is not accessible.

[Mikhail, 2016-04-08 19:28:00]

PTAL

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEPS
File extensions/renderer/DEPS (right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/DEP...
extensions/renderer/DEPS:7: "+media/base",
On 2016/04/08 17:41:31, ddorwin wrote:
> It seems that we shouldn't give all extensions code access to these files. You
> can add a new DEPS file exactly where you need it.
> 
> OTOH, these two are already in content/public/renderer/DEPS.

Done.

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.h
(right):

https://codereview.chromium.org/1860083002/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.h:80:
const scoped_refptr<media::VideoFrame>& video_frame,
On 2016/04/08 17:41:31, ddorwin wrote:
> ICWYU: media/base/video_frame.h

Done.

[ddorwin, 2016-04-08 20:36:44]

Thanks. DEPS files LGTM with comment.

https://codereview.chromium.org/1860083002/diff/60001/extensions/renderer/api...
File extensions/renderer/api/display_source/wifi_display/DEPS (right):

https://codereview.chromium.org/1860083002/diff/60001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/DEPS:2: "+media/base",
 # TODO(): Remove when https://crbug.com/432381 is fixed.

[Mikhail, 2016-04-11 09:32:51]

@Antony, could you pls take a look?

https://codereview.chromium.org/1860083002/diff/60001/extensions/renderer/api...
File extensions/renderer/api/display_source/wifi_display/DEPS (right):

https://codereview.chromium.org/1860083002/diff/60001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/DEPS:2: "+media/base",
On 2016/04/08 20:36:44, ddorwin wrote:
>  # TODO(): Remove when https://crbug.com/432381 is fixed.

Done.

[asargent_no_longer_on_chrome, 2016-04-11 18:07:38]

lgtm

[Mikhail, 2016-04-11 18:24:11]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2016-04-11 18:24:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexander.shalamov@intel.com, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1860083002/#ps80001
(title: "Added 'TODO' to the DEPS file")

[commit-bot: I haz the power, 2016-04-11 18:24:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860083002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1860083002/80001

[commit-bot: I haz the power, 2016-04-11 18:33:56]

Description was changed from

==========
[chrome.displaySource][WiFi Display] Media pipeline infrastructure

This patch introduces essential classes and interfaces for processing WiFi
Display
session media: interface for H.264 video encoder (WiFiDisplayVideoEncoder),
an object coordinating all the media processing stages
(WiFiDisplayMediaPipeline),
an auxiliary video capturer class (WiFiDisplayVideoSink).

Media is processed out of the main renderer thread (on IO thread).

WiFiDisplayMediaPacketizer initialization code is based on patch from
Eero Hakkinen <eero.hakkinen@intel.com>.

BUG=242107
==========

to

==========
[chrome.displaySource][WiFi Display] Media pipeline infrastructure

This patch introduces essential classes and interfaces for processing WiFi
Display
session media: interface for H.264 video encoder (WiFiDisplayVideoEncoder),
an object coordinating all the media processing stages
(WiFiDisplayMediaPipeline),
an auxiliary video capturer class (WiFiDisplayVideoSink).

Media is processed out of the main renderer thread (on IO thread).

WiFiDisplayMediaPacketizer initialization code is based on patch from
Eero Hakkinen <eero.hakkinen@intel.com>.

BUG=242107
==========

[commit-bot: I haz the power, 2016-04-11 18:33:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-11 18:36:26]

Description was changed from

==========
[chrome.displaySource][WiFi Display] Media pipeline infrastructure

This patch introduces essential classes and interfaces for processing WiFi
Display
session media: interface for H.264 video encoder (WiFiDisplayVideoEncoder),
an object coordinating all the media processing stages
(WiFiDisplayMediaPipeline),
an auxiliary video capturer class (WiFiDisplayVideoSink).

Media is processed out of the main renderer thread (on IO thread).

WiFiDisplayMediaPacketizer initialization code is based on patch from
Eero Hakkinen <eero.hakkinen@intel.com>.

BUG=242107
==========

to

==========
[chrome.displaySource][WiFi Display] Media pipeline infrastructure

This patch introduces essential classes and interfaces for processing WiFi
Display
session media: interface for H.264 video encoder (WiFiDisplayVideoEncoder),
an object coordinating all the media processing stages
(WiFiDisplayMediaPipeline),
an auxiliary video capturer class (WiFiDisplayVideoSink).

Media is processed out of the main renderer thread (on IO thread).

WiFiDisplayMediaPacketizer initialization code is based on patch from
Eero Hakkinen <eero.hakkinen@intel.com>.

BUG=242107

Committed: https://crrev.com/61dfa7763344c32d5477c4a97f4417fae7fd7e9b
Cr-Commit-Position: refs/heads/master@{#386423}
==========

[commit-bot: I haz the power, 2016-04-11 18:36:27]

Patchset 5 (id:??) landed as
https://crrev.com/61dfa7763344c32d5477c4a97f4417fae7fd7e9b
Cr-Commit-Position: refs/heads/master@{#386423}