[chrome.displaySource] Implement software based H.264 video encoder.

[chrome.displaySource] Implement software based H.264 video encoder.

This adds a software based implementation of Wi-Fi Display H.264 video
encoder. It uses OpenH264 library for actual encoding.

This part of a Wi-Fi Display video encoder patch series:
 * https://codereview.chromium.org/1905673002/
   Add common functionality to base video encoder.
 * https://codereview.chromium.org/1905683002/
   Implement hardware accelerated H.264 video encoder.
 * https://codereview.chromium.org/1900233004/  <-- This CL
   Implement software based H.264 video encoder.

BUG=242107
Committed: https://crrev.com/51607a47ff3614a68e479806472a8fc73cbacab8
Cr-Commit-Position: refs/heads/master@{#390650}

[e_hakkinen, 2016-04-20 18:41:42]

Description was changed from

==========
[chrome.displaySource] Implement software based H.264 video encoder.

This adds a software based implementation of Wi-Fi Display H.264 video
encoder. It uses OpenH264 library for actual encoding.

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

to

==========
[chrome.displaySource] Implement software based H.264 video encoder.

This adds a software based implementation of Wi-Fi Display H.264 video
encoder. It uses OpenH264 library for actual encoding.

This part of a Wi-Fi Display video encoder patch series:
 * https://codereview.chromium.org/1905673002/
   Enhance video encoder base class.
 * https://codereview.chromium.org/1905683002/
   Implement hardware accelerated H.264 video encoder.
 * https://codereview.chromium.org/1900233004/  <-- This CL
   Implement software based H.264 video encoder.

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

[e_hakkinen, 2016-04-22 10:04:30]

Description was changed from

==========
[chrome.displaySource] Implement software based H.264 video encoder.

This adds a software based implementation of Wi-Fi Display H.264 video
encoder. It uses OpenH264 library for actual encoding.

This part of a Wi-Fi Display video encoder patch series:
 * https://codereview.chromium.org/1905673002/
   Enhance video encoder base class.
 * https://codereview.chromium.org/1905683002/
   Implement hardware accelerated H.264 video encoder.
 * https://codereview.chromium.org/1900233004/  <-- This CL
   Implement software based H.264 video encoder.

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

to

==========
[chrome.displaySource] Implement software based H.264 video encoder.

This adds a software based implementation of Wi-Fi Display H.264 video
encoder. It uses OpenH264 library for actual encoding.

This part of a Wi-Fi Display video encoder patch series:
 * https://codereview.chromium.org/1905673002/
   Add common functionality to base video encoder.
 * https://codereview.chromium.org/1905683002/
   Implement hardware accelerated H.264 video encoder.
 * https://codereview.chromium.org/1900233004/  <-- This CL
   Implement software based H.264 video encoder.

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

[e_hakkinen, 2016-04-22 16:29:59]

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

[e_hakkinen, 2016-04-22 16:30:00]

PTAL.

[e_hakkinen, 2016-04-22 16:30:55]

Patchset #1 (id:1) has been deleted

[Mikhail, 2016-04-27 08:51:53]

lgtm

[asargent_no_longer_on_chrome, 2016-04-27 22:06:24]

lgtm

you might also need to get review from someone in third_party/openh264/OWNERS -
checkdeps will probably tell you if you do

[e_hakkinen, 2016-04-28 05:25:16]

eero.hakkinen@intel.com changed reviewers:
+ hbos@chromium.org

[e_hakkinen, 2016-04-28 05:26:25]

@hbos: pls review extensions/renderer/api/display_source/wifi_display/DEPS

[hbos_chromium, 2016-04-28 09:02:33]

Encoder code is looking good.

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.g...
extensions/extensions.gyp:201: # GN version: //extensions/renderer
Make a similar change to the GN version

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.g...
extensions/extensions.gyp:230: ['use_openh264==1', {
I added the use_openh264 flag so that I could control if openh264 is build in
chromium. While writing the build files for third_party/openh264 I changed the
default value of use_openh264 to true to make sure it built on the bots (and
then changed back to false before landing).

I don't know about enable_wifi_display, but I don't think any bots override
use_openh264, which means your code is not getting tested by our bots. This flag
is so far not used for anything except adding a dependency to the All targets
(which is probably a dependency I should remove now that the build files are
healthy, only targets using the library should have a direct dependency on it,
such as this one).

I was going to use the flag, but for the third_party/webrtc encoder using
openh264 that I wrote (h264_encoder_impl.cc) I had to use a flag defined in the
webrtc repo instead (rtc_use_h264). We made this flag default to
proprietary_codecs, which is used in Chrome Official build and other bots.

You probably want to use a flag that defaults to proprietaty_codecs too? You
could modify use_openh264 to default to proprietary_codecs or create a new flag
that defaults to it. That way your code is used without extra flags.

I'm debating removing use_openh264 since even if its false openh264 could be
used (if rtc_use_h264=1); confusing.

[hbos_chromium, 2016-04-28 09:22:15]

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.g...
extensions/extensions.gyp:230: ['use_openh264==1', {
On 2016/04/28 09:02:33, hbos_chromium wrote:
> I added the use_openh264 flag so that I could control if openh264 is build in
> chromium. While writing the build files for third_party/openh264 I changed the
> default value of use_openh264 to true to make sure it built on the bots (and
> then changed back to false before landing).
> 
> I don't know about enable_wifi_display, but I don't think any bots override
> use_openh264, which means your code is not getting tested by our bots. This
flag
> is so far not used for anything except adding a dependency to the All targets
> (which is probably a dependency I should remove now that the build files are
> healthy, only targets using the library should have a direct dependency on it,
> such as this one).
> 
> I was going to use the flag, but for the third_party/webrtc encoder using
> openh264 that I wrote (h264_encoder_impl.cc) I had to use a flag defined in
the
> webrtc repo instead (rtc_use_h264). We made this flag default to
> proprietary_codecs, which is used in Chrome Official build and other bots.
> 
> You probably want to use a flag that defaults to proprietaty_codecs too? You
> could modify use_openh264 to default to proprietary_codecs or create a new
flag
> that defaults to it. That way your code is used without extra flags.
> 
> I'm debating removing use_openh264 since even if its false openh264 could be
> used (if rtc_use_h264=1); confusing.

If this does not apply because this is an extension and you don't mind the extra
flag you can leave this as-is. I don't know the background.

[e_hakkinen, 2016-04-28 13:47:40]

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.g...
extensions/extensions.gyp:201: # GN version: //extensions/renderer
On 2016/04/28 09:02:33, hbos_chromium wrote:
> Make a similar change to the GN version

Done.

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.g...
extensions/extensions.gyp:230: ['use_openh264==1', {
On 2016/04/28 09:02:33, hbos_chromium wrote:
> You probably want to use a flag that defaults to proprietaty_codecs too? You
> could modify use_openh264 to default to proprietary_codecs or create a new
flag
> that defaults to it. That way your code is used without extra flags.

The code is not used without an extra flags anyway as enable_wifi_display is
false by default.

As the software fallback is quite important part of WiFi Display stack, I just
removed the condition altogether.

[hbos_chromium, 2016-04-28 14:27:29]

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/1900233004/diff/60001/extensions/extensions.g...
extensions/extensions.gyp:230: ['use_openh264==1', {
On 2016/04/28 13:47:39, e_hakkinen wrote:
> On 2016/04/28 09:02:33, hbos_chromium wrote:
> > You probably want to use a flag that defaults to proprietaty_codecs too? You
> > could modify use_openh264 to default to proprietary_codecs or create a new
> flag
> > that defaults to it. That way your code is used without extra flags.
> 
> The code is not used without an extra flags anyway as enable_wifi_display is
> false by default.
> 
> As the software fallback is quite important part of WiFi Display stack, I just
> removed the condition altogether.

There are reasons for why third_party/openh264 is not built by default. We need
to be careful about adding dependency on it without special flags. It is already
behind one flag, but I think you should add a new flag and default it to
proprietary_codecs, making openh264 an optional part of this extension.

[e_hakkinen, 2016-04-28 16:32:56]

On 2016/04/28 14:27:29, hbos_chromium wrote:
> There are reasons for why third_party/openh264 is not built by default. We
need
> to be careful about adding dependency on it without special flags. It is
already
> behind one flag, but I think you should add a new flag and default it to
> proprietary_codecs, making openh264 an optional part of this extension.

The thing is that the whole Wi-Fi Display and not just this software encode is
all about proprietary codecs (Wi-Fi Display is mainly about sending H.264
encoded video frames in MPEG Transport Stream). So the flag which could default
to proprietary_codecs is enable_wifi_display. Therefore, I do not see a need for
a new flag.

I added explicit checks for proprietary_codecs just like is done in //media (for
decoders, WiFi Display is about encoders). Is that OK?

[hbos_chromium, 2016-04-29 08:17:05]

On 2016/04/28 16:32:56, e_hakkinen wrote:
> On 2016/04/28 14:27:29, hbos_chromium wrote:
> > There are reasons for why third_party/openh264 is not built by default. We
> need
> > to be careful about adding dependency on it without special flags. It is
> already
> > behind one flag, but I think you should add a new flag and default it to
> > proprietary_codecs, making openh264 an optional part of this extension.
> 
> The thing is that the whole Wi-Fi Display and not just this software encode is
> all about proprietary codecs (Wi-Fi Display is mainly about sending H.264
> encoded video frames in MPEG Transport Stream). So the flag which could
default
> to proprietary_codecs is enable_wifi_display. Therefore, I do not see a need
for
> a new flag.
> 
> I added explicit checks for proprietary_codecs just like is done in //media
(for
> decoders, WiFi Display is about encoders). Is that OK?

That sounds like a good approach, but then I have to ask: Was it previously
possible to build enable_wifi_display without also building proprietary_codecs?
With the requirement that proprietary_codecs be true, more codecs than just
third_party/openh264 would have to be pulled in for other stuff. If that's OK,
then this should be OK, otherwise it might be a problem.

Having a separate flag (that defaults to proprietary_codecs) has the benefit
that you can flip the switch only for that codec in case you want to build with
proprietary_codecs=false. But if proprietary_codecs would have to be true anyway
a separate flag is pointless, and explicit checks a good thing.

(Sorry for delaying the otherwise good CL, being careful.)

[Mikhail, 2016-04-29 09:34:54]

On 2016/04/29 08:17:05, hbos_chromium wrote:
> That sounds like a good approach, but then I have to ask: Was it previously
> possible to build enable_wifi_display without also building
proprietary_codecs?
> With the requirement that proprietary_codecs be true, more codecs than just
> third_party/openh264 would have to be pulled in for other stuff. If that's OK,
> then this should be OK, otherwise it might be a problem.
> 
> Having a separate flag (that defaults to proprietary_codecs) has the benefit
> that you can flip the switch only for that codec in case you want to build
with
> proprietary_codecs=false. But if proprietary_codecs would have to be true
anyway
> a separate flag is pointless, and explicit checks a good thing.
> 
> (Sorry for delaying the otherwise good CL, being careful.)

Thanks for you comments and concerns. At the moment the WiFi Display
functionality is under development so the dedicated build flag
('enable_wifi_display') is just set to 'false' by default, however eventually
that should be in sync with 'proprietary_codecs' flag as the WiFi Display
implementation both uses the encoders which already exist in chromium and
implements some new ones. IMO there is no need in introduction of a new build
flag.

[hbos_chromium, 2016-04-29 13:56:58]

On 2016/04/29 09:34:54, Mikhail wrote:
> On 2016/04/29 08:17:05, hbos_chromium wrote:
> > That sounds like a good approach, but then I have to ask: Was it previously
> > possible to build enable_wifi_display without also building
> proprietary_codecs?
> > With the requirement that proprietary_codecs be true, more codecs than just
> > third_party/openh264 would have to be pulled in for other stuff. If that's
OK,
> > then this should be OK, otherwise it might be a problem.
> > 
> > Having a separate flag (that defaults to proprietary_codecs) has the benefit
> > that you can flip the switch only for that codec in case you want to build
> with
> > proprietary_codecs=false. But if proprietary_codecs would have to be true
> anyway
> > a separate flag is pointless, and explicit checks a good thing.
> > 
> > (Sorry for delaying the otherwise good CL, being careful.)
> 
> Thanks for you comments and concerns. At the moment the WiFi Display
> functionality is under development so the dedicated build flag
> ('enable_wifi_display') is just set to 'false' by default, however eventually
> that should be in sync with 'proprietary_codecs' flag as the WiFi Display
> implementation both uses the encoders which already exist in chromium and
> implements some new ones. IMO there is no need in introduction of a new build
> flag.

OK. LGTM!

[Mikhail, 2016-04-29 14:11:40]

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

[Mikhail, 2016-04-29 14:11:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, mikhail.pozdnyakov@intel.com
Link to the patchset: https://codereview.chromium.org/1900233004/#ps100001
(title: "Check proprietary_codecs")

[commit-bot: I haz the power, 2016-04-29 14:11:57]

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

[commit-bot: I haz the power, 2016-04-29 15:08:29]

Description was changed from

==========
[chrome.displaySource] Implement software based H.264 video encoder.

This adds a software based implementation of Wi-Fi Display H.264 video
encoder. It uses OpenH264 library for actual encoding.

This part of a Wi-Fi Display video encoder patch series:
 * https://codereview.chromium.org/1905673002/
   Add common functionality to base video encoder.
 * https://codereview.chromium.org/1905683002/
   Implement hardware accelerated H.264 video encoder.
 * https://codereview.chromium.org/1900233004/  <-- This CL
   Implement software based H.264 video encoder.

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

to

==========
[chrome.displaySource] Implement software based H.264 video encoder.

This adds a software based implementation of Wi-Fi Display H.264 video
encoder. It uses OpenH264 library for actual encoding.

This part of a Wi-Fi Display video encoder patch series:
 * https://codereview.chromium.org/1905673002/
   Add common functionality to base video encoder.
 * https://codereview.chromium.org/1905683002/
   Implement hardware accelerated H.264 video encoder.
 * https://codereview.chromium.org/1900233004/  <-- This CL
   Implement software based H.264 video encoder.

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

[commit-bot: I haz the power, 2016-04-29 15:08:30]

Committed patchset #5 (id:100001)

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

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

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

Description was changed from

==========
[chrome.displaySource] Implement software based H.264 video encoder.

This adds a software based implementation of Wi-Fi Display H.264 video
encoder. It uses OpenH264 library for actual encoding.

This part of a Wi-Fi Display video encoder patch series:
 * https://codereview.chromium.org/1905673002/
   Add common functionality to base video encoder.
 * https://codereview.chromium.org/1905683002/
   Implement hardware accelerated H.264 video encoder.
 * https://codereview.chromium.org/1900233004/  <-- This CL
   Implement software based H.264 video encoder.

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

to

==========
[chrome.displaySource] Implement software based H.264 video encoder.

This adds a software based implementation of Wi-Fi Display H.264 video
encoder. It uses OpenH264 library for actual encoding.

This part of a Wi-Fi Display video encoder patch series:
 * https://codereview.chromium.org/1905673002/
   Add common functionality to base video encoder.
 * https://codereview.chromium.org/1905683002/
   Implement hardware accelerated H.264 video encoder.
 * https://codereview.chromium.org/1900233004/  <-- This CL
   Implement software based H.264 video encoder.

BUG=242107

Committed: https://crrev.com/51607a47ff3614a68e479806472a8fc73cbacab8
Cr-Commit-Position: refs/heads/master@{#390650}
==========