[chrome.displaySource] Move video stream info creation to encoders.

[chrome.displaySource] Move video stream info creation to encoders.

This patch moves creation of elementary stream infos for video streams
from media pipeline to video encoders. Detailed knowledge of encoding
details does not have to be duplicated from encoders to media pipeline.

BUG=242107
Committed: https://crrev.com/856cbf9b015810a1087b300412d81a274a35685b
Cr-Commit-Position: refs/heads/master@{#390612}

[e_hakkinen, 2016-04-22 16:24:11]

eero.hakkinen@intel.com changed reviewers:
+ alexander.shalamov@intel.com, asargent@chromium.org,
mikhail.pozdnyakov@intel.com

[e_hakkinen, 2016-04-22 16:24:12]

PTAL.

For audio streams, a similar change will be done in audio encoder CLs (see
https://codereview.chromium.org/1903823002/ and
https://codereview.chromium.org/1905243003/).

[Mikhail, 2016-04-29 06:36:57]

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

https://codereview.chromium.org/1911403003/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.cc:40:
WiFiDisplayVideoEncoder::CreateElementaryStreamDescriptors() const {
why this method is needed(isn't CreateElementaryStreamInfo enough)?

[e_hakkinen, 2016-04-29 07:16:13]

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

https://codereview.chromium.org/1911403003/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.cc:40:
WiFiDisplayVideoEncoder::CreateElementaryStreamDescriptors() const {
On 2016/04/29 06:36:57, Mikhail wrote:
> why this method is needed(isn't CreateElementaryStreamInfo enough)?

The idea is that it can be overridden by WiFiDisplayVideoEncoder{SVC,VEA} so
that SVC and VEA can modify descriptors (particularly insert AVC Video
descriptors) before an info is created.

[Mikhail, 2016-04-29 07:37:41]

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

https://codereview.chromium.org/1911403003/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.cc:40:
WiFiDisplayVideoEncoder::CreateElementaryStreamDescriptors() const {
On 2016/04/29 07:16:13, e_hakkinen wrote:
> On 2016/04/29 06:36:57, Mikhail wrote:
> > why this method is needed(isn't CreateElementaryStreamInfo enough)?
> 
> The idea is that it can be overridden by WiFiDisplayVideoEncoder{SVC,VEA} so
> that SVC and VEA can modify descriptors (particularly insert AVC Video
> descriptors) before an info is created.

CreateElementaryStreamInfo's body is tiny, IMO encoder implementations could
just override it if needed.

[e_hakkinen, 2016-04-29 08:27:07]

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

https://codereview.chromium.org/1911403003/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_video_encoder.cc:40:
WiFiDisplayVideoEncoder::CreateElementaryStreamDescriptors() const {
On 2016/04/29 07:37:41, Mikhail wrote:
> CreateElementaryStreamInfo's body is tiny, IMO encoder implementations could
> just override it if needed.

With the current design, WiFiDisplayVideoEncoder defines the functions

  WiFiDisplayElementaryStreamInfo CreateElementaryStreamInfo() const final;

  virtual std::vector<WiFiDisplayElementaryStreamDescriptor>
  CreateElementaryStreamDescriptors() const;

and an implementation class would define

  std::vector<WiFiDisplayElementaryStreamDescriptor>
  CreateElementaryStreamDescriptors() const override {
   // Create descriptors common to all H.264 video encoders.
   auto descriptors =
       WiFiDisplayVideoEncoder::CreateElementaryStreamDescriptors();
   // Modify descriptors as needed. At least add an AVCVideo descriptor.
   descriptors.push_back(
       WiFiDisplayElementaryStreamDescriptor::AVCVideo::Create(/* ... */));
   // Return descriptors.
   return descriptors;
  }

With CreateElementaryStreamDescriptors replaced with
CreateCommonElementaryStreamDescriptors, WiFiDisplayVideoEncoder would define
the function

  std::vector<WiFiDisplayElementaryStreamDescriptor>
  CreateCommonElementaryStreamDescriptors() const;

and an implementation class would define

  WiFiDisplayElementaryStreamInfo CreateElementaryStreamInfo() const override {
   // Create descriptors common to all H.264 video encoders.
   auto descriptors = CreateCommonElementaryStreamDescriptors();
   // Modify descriptors as needed. At least add an AVCVideo descriptor.
   descriptors.push_back(
       WiFiDisplayElementaryStreamDescriptor::AVCVideo::Create(/* ... */));
   // Create and return info.
   return WiFiDisplayElementaryStreamInfo(
       WiFiDisplayElementaryStreamInfo::VIDEO_H264, std::move(descriptors));
  }

So in both options, in WiFiDisplayVideoEncoder there is a function for creating
elementary stream descriptors (CreateElementaryStreamDescriptors or
CreateCommonElementaryStreamDescriptors). It does not make sense to drop those
functions completely because they implement common functionality which should
not be reimplemented in each and every implementation class. Although, at the
moment, those functions only create AVC Timing and HRD descriptors they should
eventually create other descriptors such as window info descriptors, too.

Are you saying that you would prefer the latter design more?
In the beginning, you asked whether a function to create descriptors is needed.
It is. But now I am not sure what you are asking. Hopefully this is detailed
enough description for getting us to agreement.

[Mikhail, 2016-04-29 08:39:29]

On 2016/04/29 08:27:07, e_hakkinen wrote:
> 
> Are you saying that you would prefer the latter design more?
> In the beginning, you asked whether a function to create descriptors is
needed.
> It is. But now I am not sure what you are asking. Hopefully this is detailed
> enough description for getting us to agreement.
would it be simpler if 'descriptors' were a protected member of
WiFiDisplayVideoEncoder, inherited classes could add the extra items in their
ctors, an we had only one generic
WiFiDisplayVideoEncoder::CreateElementaryStreamInfo() implementation?

[e_hakkinen, 2016-04-29 09:43:12]

On 2016/04/29 08:39:29, Mikhail wrote:
> would it be simpler if 'descriptors' were a protected member of
> WiFiDisplayVideoEncoder, inherited classes could add the extra items in their
> ctors, an we had only one generic
> WiFiDisplayVideoEncoder::CreateElementaryStreamInfo() implementation?

Yes, it is simpler way, but IMHO also less clean way. Anyway, done.

[Mikhail, 2016-04-29 09:46:45]

lgtm

[e_hakkinen, 2016-04-29 09:47:33]

The CQ bit was checked by eero.hakkinen@intel.com

[commit-bot: I haz the power, 2016-04-29 09:47:39]

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

[commit-bot: I haz the power, 2016-04-29 10:34:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-30 17:24:52]

Patchset 3 (id:??) landed as
https://crrev.com/856cbf9b015810a1087b300412d81a274a35685b
Cr-Commit-Position: refs/heads/master@{#390612}

[commit-bot: I haz the power, 2016-04-30 18:00:01]

Description was changed from

==========
[chrome.displaySource] Move video stream info creation to encoders.

This patch moves creation of elementary stream infos for video streams
from media pipeline to video encoders. Detailed knowledge of encoding
details does not have to be duplicated from encoders to media pipeline.

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

to

==========
[chrome.displaySource] Move video stream info creation to encoders.

This patch moves creation of elementary stream infos for video streams
from media pipeline to video encoders. Detailed knowledge of encoding
details does not have to be duplicated from encoders to media pipeline.

BUG=242107

Committed: https://crrev.com/856cbf9b015810a1087b300412d81a274a35685b
Cr-Commit-Position: refs/heads/master@{#390612}
==========