MediaRecorder: use VideoTrackRecorder::GetPreferredCodecId() when available

MediaRecorder: use VideoTrackRecorder::GetPreferredCodecId() when available

This CL adds a static method to VTR to query which
one, if any, is the preferred video codec. 

A new singleton Lazy Leaky class CodecEnumerator is
added to encapsulate poking the VEA to see which
codecs are supported, and to further query 
a) the preferred codec id
b) if a given codec is supported
and wraps the previous CodecIdToVEAProfile()
functionality.

BUG=679946
Review-Url: https://codereview.chromium.org/2624053002
Cr-Commit-Position: refs/heads/master@{#443165}
Committed: https://chromium.googlesource.com/chromium/src/+/904b7a6f6dffa9cee5ac930c37dd591c330d14c6

[mcasas, 2017-01-11 01:29:52]

Patchset #1 (id:1) has been deleted

[mcasas, 2017-01-11 05:28:13]

Patchset #1 (id:20001) has been deleted

[mcasas, 2017-01-11 15:28:47]

Description was changed from

==========
MediaRecorder: use VideoTrackRecorder::GetPreferredCodec() when available

Added static method to get the preferred codec

BUG=679946
==========

to

==========
MediaRecorder: use VideoTrackRecorder::GetPreferredCodec() when available

This CL adds a static method to VTR to query which
one, if any, is the preferred one.  The algorithm is simple
(trivial), just walk the list of CodecIds, which is inherently
ordered by priority, and return the first one that is possibly
hardware accelerated.

BUG=679946
==========

[mcasas, 2017-01-11 15:30:08]

mcasas@chromium.org changed reviewers:
+ emircan@chromium.org

[mcasas, 2017-01-11 15:30:08]

emircan@ PTAL

Any suggestions on testing this? 
VEAs are not present in content_browsertests,
it seems (neither render main loop).

[chfremer, 2017-01-11 17:34:51]

chfremer@chromium.org changed reviewers:
+ chfremer@chromium.org

[chfremer, 2017-01-11 17:34:52]

chfremer@chromium.org changed reviewers:
+ chfremer@chromium.org

[chfremer, 2017-01-11 17:34:52]

https://codereview.chromium.org/2624053002/diff/40001/content/renderer/media/...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2624053002/diff/40001/content/renderer/media/...
content/renderer/media/video_track_recorder.cc:1085: switch (codec_id) {
Why not use a for-loop to check a fixed list of candidate codecs instead of a
surprising while-loop with a switch-case for iteration updates and an artifical
CodecID::LAST for termination?

std::vector<CodecId> candidate_codecs({
  CodecId::VP8,
  CodecId::VP9,
  CodecId::H264
});
for (auto codec_id : candidate_codec_ids) {
  if (CodecIdToVEAProfile(codec_id) != media::VIDEO_CODEC_PROFILE_UNKNOWN)
    return codec_id;
}

Could maybe even use std::find_if() instead of for-loop.

[chfremer, 2017-01-11 17:34:52]

https://codereview.chromium.org/2624053002/diff/40001/content/renderer/media/...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2624053002/diff/40001/content/renderer/media/...
content/renderer/media/video_track_recorder.cc:1085: switch (codec_id) {
Why not use a for-loop to check a fixed list of candidate codecs instead of a
surprising while-loop with a switch-case for iteration updates and an artifical
CodecID::LAST for termination?

std::vector<CodecId> candidate_codecs({
  CodecId::VP8,
  CodecId::VP9,
  CodecId::H264
});
for (auto codec_id : candidate_codec_ids) {
  if (CodecIdToVEAProfile(codec_id) != media::VIDEO_CODEC_PROFILE_UNKNOWN)
    return codec_id;
}

Could maybe even use std::find_if() instead of for-loop.

[chfremer, 2017-01-11 17:44:29]

On 2017/01/11 15:30:08, mcasas wrote:
> emircan@ PTAL
> 
> Any suggestions on testing this? 
> VEAs are not present in content_browsertests,
> it seems (neither render main loop).

Regarding testing, would it work to mock out 
the method CodecIdToVEAProfile() for the tests?

[emircan, 2017-01-11 18:01:58]

https://codereview.chromium.org/2624053002/diff/40001/content/renderer/media/...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2624053002/diff/40001/content/renderer/media/...
content/renderer/media/video_track_recorder.cc:1082: if
(CodecIdToVEAProfile(codec_id) != media::VIDEO_CODEC_PROFILE_UNKNOWN)
We end up calling CodecIdToVEAProfile() at least 3 times here and I think it is
redundant. Why don't we refactor that CodecIdToVEAProfile() into
GetSupportedCodecsAndProfiles(), where you can just remove the if clause on
l.110 and it would lazy evaluate supported
<content::VideoTrackRecorder::CodecId, media::VideoCodecProfile> pairs. CodecId
is needed by MediaRecorder and VideoCodecProfile by VEA. You can use that
function here, and just keep the if clause on l.110 in CodecIdToVEAProfile().

P.S. We currently always go for the min VideoCodecProfile that matches, i.e.
when H264PROFILE_BASELINE and H264PROFILE_MAIN is supported, we would only go
for baseline. You can also order them if it makes a difference for the
container.

[mcasas, 2017-01-11 21:26:24]

Patchset #2 (id:60001) has been deleted

[mcasas, 2017-01-11 21:26:31]

Patchset #2 (id:80001) has been deleted

[mcasas, 2017-01-11 21:28:22]

Description was changed from

==========
MediaRecorder: use VideoTrackRecorder::GetPreferredCodec() when available

This CL adds a static method to VTR to query which
one, if any, is the preferred one.  The algorithm is simple
(trivial), just walk the list of CodecIds, which is inherently
ordered by priority, and return the first one that is possibly
hardware accelerated.

BUG=679946
==========

to

==========
MediaRecorder: use VideoTrackRecorder::GetPreferredCodec() when available

This CL adds a static method to VTR to query which
one, if any, is the preferred one. 

A new singleton Lazy Leaky class CodecEnumerator is
added to encapsulate poking the VEA to see which
codecs are supported, and to further query 
a) the preferred codec id
b) if a given codec is supported

BUG=679946
==========

[mcasas, 2017-01-11 21:29:04]

Description was changed from

==========
MediaRecorder: use VideoTrackRecorder::GetPreferredCodec() when available

This CL adds a static method to VTR to query which
one, if any, is the preferred one. 

A new singleton Lazy Leaky class CodecEnumerator is
added to encapsulate poking the VEA to see which
codecs are supported, and to further query 
a) the preferred codec id
b) if a given codec is supported

BUG=679946
==========

to

==========
MediaRecorder: use VideoTrackRecorder::GetPreferredCodecId() when available

This CL adds a static method to VTR to query which
one, if any, is the preferred video codec. 

A new singleton Lazy Leaky class CodecEnumerator is
added to encapsulate poking the VEA to see which
codecs are supported, and to further query 
a) the preferred codec id
b) if a given codec is supported
and wraps the previous CodecIdToVEAProfile()
functionality.

BUG=679946
==========

[mcasas, 2017-01-11 21:37:36]

Patchset #2 (id:100001) has been deleted

[mcasas, 2017-01-11 21:40:48]

ptal.

https://codereview.chromium.org/2624053002/diff/40001/content/renderer/media/...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2624053002/diff/40001/content/renderer/media/...
content/renderer/media/video_track_recorder.cc:1082: if
(CodecIdToVEAProfile(codec_id) != media::VIDEO_CODEC_PROFILE_UNKNOWN)
On 2017/01/11 18:01:58, emircan wrote:
> We end up calling CodecIdToVEAProfile() at least 3 times here and I think it
is
> redundant. Why don't we refactor that CodecIdToVEAProfile() into
> GetSupportedCodecsAndProfiles(), where you can just remove the if clause on
> l.110 and it would lazy evaluate supported
> <content::VideoTrackRecorder::CodecId, media::VideoCodecProfile> pairs.
CodecId
> is needed by MediaRecorder and VideoCodecProfile by VEA. You can use that
> function here, and just keep the if clause on l.110 in CodecIdToVEAProfile().
> 

Ok, you convinced me to cut deeper and made a singleton
lazy class encapsulating all these interactions,

> P.S. We currently always go for the min VideoCodecProfile that matches, i.e.
> when H264PROFILE_BASELINE and H264PROFILE_MAIN is supported, we would only go
> for baseline. You can also order them if it makes a difference for the
> container.

Hmm uncertain, really.  This is a fallback path for
when the user has no preference either, so using
the baseline profile sounds good enough.

https://codereview.chromium.org/2624053002/diff/40001/content/renderer/media/...
content/renderer/media/video_track_recorder.cc:1085: switch (codec_id) {
On 2017/01/11 17:34:52, chfremer wrote:
> Why not use a for-loop to check a fixed list of candidate codecs instead of a
> surprising while-loop with a switch-case for iteration updates and an
artifical
> CodecID::LAST for termination?
> 
> std::vector<CodecId> candidate_codecs({
>   CodecId::VP8,
>   CodecId::VP9,
>   CodecId::H264
> });
> for (auto codec_id : candidate_codec_ids) {
>   if (CodecIdToVEAProfile(codec_id) != media::VIDEO_CODEC_PROFILE_UNKNOWN)
>     return codec_id;
> }
> 
> Could maybe even use std::find_if() instead of for-loop.

I wanted to have a compiler-enforced way to guarantee
that all codecs are iterated for, hence the switch with
no default:

Following the comments of ermican@ above allows to
enforce it via static_assert.

[emircan, 2017-01-11 22:11:05]

https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:138:
std::make_pair(codec_id_and_profile.codec_id, vea_profile.profile));
Note that this would take the min profile if there is multiple. For instance, if
H264_BASELINE and H264_MAIN is supported, it would see that key exists in the
second pass and emplace() would return false.

https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:138:
std::make_pair(codec_id_and_profile.codec_id, vea_profile.profile));
codec_id_to_profile_.emplace(codec_id_and_profile.codec_id,
vea_profile.profile);

[emircan, 2017-01-11 22:14:36]

On 2017/01/11 22:11:05, emircan wrote:
>
https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
> File content/renderer/media/video_track_recorder.cc (right):
> 
>
https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
> content/renderer/media/video_track_recorder.cc:138:
> std::make_pair(codec_id_and_profile.codec_id, vea_profile.profile));
> Note that this would take the min profile if there is multiple. For instance,
if
> H264_BASELINE and H264_MAIN is supported, it would see that key exists in the
> second pass and emplace() would return false.
> 
>
https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
> content/renderer/media/video_track_recorder.cc:138:
> std::make_pair(codec_id_and_profile.codec_id, vea_profile.profile));
> codec_id_to_profile_.emplace(codec_id_and_profile.codec_id,
> vea_profile.profile);

lgtm % nits above and bots.

[mcasas, 2017-01-11 22:30:16]

https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:138:
std::make_pair(codec_id_and_profile.codec_id, vea_profile.profile));
On 2017/01/11 22:11:05, emircan wrote:
> codec_id_to_profile_.emplace(codec_id_and_profile.codec_id,
> vea_profile.profile);

Done.

https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:138:
std::make_pair(codec_id_and_profile.codec_id, vea_profile.profile));
On 2017/01/11 22:11:05, emircan wrote:
> Note that this would take the min profile if there is multiple. For instance,
if
> H264_BASELINE and H264_MAIN is supported, it would see that key exists in the
> second pass and emplace() would return false.

Gotcha. That's ok for the time being.
We'll be keeping an eye out, specially as 
we enable more platforms.

[chfremer, 2017-01-11 23:24:56]

PS#3 lgtm with some nits (and whining)

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/media_recorder_handler.cc:141: codec_id_ =
VideoTrackRecorder::GetPreferredCodecId();
Use of a static method is probably considered okay here, even though I don't
like it because it takes us one step further away from being able to test this
class in isolation. The alternative would be to pass an interface into the
constructor that provides the GetPreferredCodecId() method, which is probably
considered overkill. Your call.

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:65: // Class to encapsulate the
enumeration of CodecIds/VideoCodecProfiles supported
Nit: "Encapsulates the ..."

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:73: // Get the first CodecId that
has an associated VEA VideoCodecProfile, or VP8.
Nit: "Returns the ..."

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:130: const
media::VideoEncodeAccelerator::SupportedProfiles& vea_profiles =
Nit: During my read-through, I would have found it more useful if the variable
was called |supported_vea_profiles|, because in l.132 it didn't help me much
that the type name |SupportedProfiles| contained the hint that these are the
supported profiles. (I guess I didn't read the code strictly from top to bottom,
... or I have an extremely short memory)

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:138: vea_profile.profile);
Can it happen that more than one entry is emplaced for the same codec_id?

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:147: return
codec_id_to_profile_.begin()->first;
Hmm, so if multiple codecs have VEA profiles, the order in
|kCodecIdAndVEAProfiles| determines which one is preferred. 
If that is true and intended, could we express that more explicitly?
Best would be in the variable name. Second best may be in a comment at the
declaration of |kCodecIdAndVEAProfiles|.

[mcasas, 2017-01-12 00:15:15]

Patchset #4 (id:160001) has been deleted

[mcasas, 2017-01-12 00:20:19]

Patchset #4 (id:180001) has been deleted

[mcasas, 2017-01-12 00:20:55]

https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2624053002/diff/120001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:138:
std::make_pair(codec_id_and_profile.codec_id, vea_profile.profile));
On 2017/01/11 22:30:16, mcasas wrote:
> On 2017/01/11 22:11:05, emircan wrote:
> > codec_id_to_profile_.emplace(codec_id_and_profile.codec_id,
> > vea_profile.profile);
> 
> Done.

Some bots were unhappy, see e.g. linux_chromium_compile_rel_ng:

video_track_recorder.cc:137:30: error: no member named 'emplace' in
'std::map<content::VideoTrackRecorder::CodecId, media::VideoCodecProfile,
std::less<content::VideoTrackRecorder::CodecId>, std::allocator<std::pair<const
content::VideoTrackRecorder::CodecId, media::VideoCodecProfile> > >'
        codec_id_to_profile_.emplace(
        ~~~~~~~~~~~~~~~~~~~~ ^

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/media_recorder_handler.cc:141: codec_id_ =
VideoTrackRecorder::GetPreferredCodecId();
On 2017/01/11 23:24:55, chfremer wrote:
> Use of a static method is probably considered okay here, even though I don't
> like it because it takes us one step further away from being able to test this
> class in isolation. The alternative would be to pass an interface into the
> constructor that provides the GetPreferredCodecId() method, which is probably
> considered overkill. Your call.

Hmm given that the whole testing of this code is
anyway done via integration tests (the VEA test
mocking is a mock), I'd say let's leave it as is...

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:65: // Class to encapsulate the
enumeration of CodecIds/VideoCodecProfiles supported
On 2017/01/11 23:24:55, chfremer wrote:
> Nit: "Encapsulates the ..." 

"to encapsulate" is correct, right?

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:73: // Get the first CodecId that
has an associated VEA VideoCodecProfile, or VP8.
On 2017/01/11 23:24:55, chfremer wrote:
> Nit: "Returns the ..."

Done.

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:130: const
media::VideoEncodeAccelerator::SupportedProfiles& vea_profiles =
On 2017/01/11 23:24:55, chfremer wrote:
> Nit: During my read-through, I would have found it more useful if the variable
> was called |supported_vea_profiles|, because in l.132 it didn't help me much
> that the type name |SupportedProfiles| contained the hint that these are the
> supported profiles. (I guess I didn't read the code strictly from top to
bottom,
> ... or I have an extremely short memory)

Done.

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:138: vea_profile.profile);
On 2017/01/11 23:24:55, chfremer wrote:
> Can it happen that more than one entry is emplaced for the same codec_id?

Yeah, as emircan@ commented, subsequent ones will be
ignored, so if we have e.g. 
  H264PROFILE_MAIN = 1,
  H264PROFILE_EXTENDED = 2,
  H264PROFILE_HIGH = 3,
 
only the first one would be in |codec_id_to_profile_|.
At this point that's fine, and is the current behaviour
for CodecIdToVEAProfile().

https://codereview.chromium.org/2624053002/diff/140001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:147: return
codec_id_to_profile_.begin()->first;
On 2017/01/11 23:24:55, chfremer wrote:
> Hmm, so if multiple codecs have VEA profiles, the order in
> |kCodecIdAndVEAProfiles| determines which one is preferred. 
> If that is true and intended, could we express that more explicitly?
> Best would be in the variable name. Second best may be in a comment at the
> declaration of |kCodecIdAndVEAProfiles|.

It's good that you take a fresh look at this!
s/kCodecIdAndVEAProfiles/kPreferredCodecAndProfiles/.

[mcasas, 2017-01-12 00:21:03]

Patchset #4 (id:80002) has been deleted

[mcasas, 2017-01-12 02:41:54]

Patchset #5 (id:230001) has been deleted

[mcasas, 2017-01-12 02:53:33]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 02:54:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 03:04:18]

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

[commit-bot: I haz the power, 2017-01-12 03:04:19]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[mcasas, 2017-01-12 05:59:37]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2017-01-12 05:59:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
emircan@chromium.org, chfremer@chromium.org
Link to the patchset: https://codereview.chromium.org/2624053002/#ps250001
(title: "Windows issue: doesn't like static constexpr members w/
initialization")

[commit-bot: I haz the power, 2017-01-12 05:59:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 06:04:45]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1484200777287610,
"parent_rev": "14f223988bfd4444a1238450919cef246d0ed51a", "commit_rev":
"904b7a6f6dffa9cee5ac930c37dd591c330d14c6"}

[commit-bot: I haz the power, 2017-01-12 06:06:03]

Description was changed from

==========
MediaRecorder: use VideoTrackRecorder::GetPreferredCodecId() when available

This CL adds a static method to VTR to query which
one, if any, is the preferred video codec. 

A new singleton Lazy Leaky class CodecEnumerator is
added to encapsulate poking the VEA to see which
codecs are supported, and to further query 
a) the preferred codec id
b) if a given codec is supported
and wraps the previous CodecIdToVEAProfile()
functionality.

BUG=679946
==========

to

==========
MediaRecorder: use VideoTrackRecorder::GetPreferredCodecId() when available

This CL adds a static method to VTR to query which
one, if any, is the preferred video codec. 

A new singleton Lazy Leaky class CodecEnumerator is
added to encapsulate poking the VEA to see which
codecs are supported, and to further query 
a) the preferred codec id
b) if a given codec is supported
and wraps the previous CodecIdToVEAProfile()
functionality.

BUG=679946

Review-Url: https://codereview.chromium.org/2624053002
Cr-Commit-Position: refs/heads/master@{#443165}
Committed:
https://chromium.googlesource.com/chromium/src/+/904b7a6f6dffa9cee5ac930c37dd...
==========

[commit-bot: I haz the power, 2017-01-12 06:06:05]

Committed patchset #5 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/904b7a6f6dffa9cee5ac930c37dd...

[xlai (Olivia), 2017-01-12 16:10:15]

A revert of this CL (patchset #5 id:250001) has been created in
https://codereview.chromium.org/2622273006/ by xlai@chromium.org.

The reason for reverting is: Suspected CL-to-blame for webkit tests failing on
Mac Retina:
fast/mediacapturefromelement/HTMLMediaElementCapture-capture.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.11%20%28....