Add video frame recording capability to Chromoting hosts.

Add video frame recording capability to Chromoting hosts.

This will be used to record sequences of video frames for real-world sessions, for performance-evaluation purposes.

If the host is run with a video-recording buffer size specified then it will advertise a videoRecorder capability to clients. Clients seeing this capability can send "video-recorder" extension messages to the host to start and stop recording, and to fetch the next recorded frame.

Frames are encoded to VideoPackets using the "verbatim" encoder, and must then be serialized and Base-64 encoded for transmission via the extension message protocol.

See crrev.com/386853002 for the client part of this.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287183

[Wez, 2014-07-11 02:28:46]

PTAL

This approach ends up being considerably less efficient than adding a new
message to the core Chromocol, but avoids adding complexity to the Chromotocol
for what is a developer capability.

[Sergey Ulanov, 2014-07-11 18:38:04]

https://codereview.chromium.org/372943002/diff/20001/remoting/host/chromoting...
File remoting/host/chromoting_host.cc (right):

https://codereview.chromium.org/372943002/diff/20001/remoting/host/chromoting...
remoting/host/chromoting_host.cc:187: if (!enable) {
nit: remove {}

https://codereview.chromium.org/372943002/diff/20001/remoting/host/chromoting...
File remoting/host/chromoting_host.h (right):

https://codereview.chromium.org/372943002/diff/20001/remoting/host/chromoting...
remoting/host/chromoting_host.h:121: // Enables recording of video frame
sequences. by clients.
remove . after 'sequences'

https://codereview.chromium.org/372943002/diff/20001/remoting/host/chromoting...
remoting/host/chromoting_host.h:122: void SetEnableVideoFrameRecording(bool
enable);
Do we really need this flag? VideoFrameRecorder doesn't use any significant
resources unless recording has been started so I think ClientSession can just
always create it.

https://codereview.chromium.org/372943002/diff/20001/remoting/host/host_config.h
File remoting/host/host_config.h (right):

https://codereview.chromium.org/372943002/diff/20001/remoting/host/host_confi...
remoting/host/host_config.h:45: // Number of Kilobytes of frame data to allow
each client to record.
nit: it's Kibibytes.

https://codereview.chromium.org/372943002/diff/20001/remoting/host/remoting_m...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/372943002/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:128: const char
kFrameRecorderBufferKbName[] = "frame-recorder-buffer-kb";
Do we really need this flag? Maybe always have recorder enabled with some
sensible buffer size. It won't do anything until recording is enabled anyway.

https://codereview.chromium.org/372943002/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:128: const char
kFrameRecorderBufferKbName[] = "frame-recorder-buffer-kb";
Add this flag in the usage message in host_main.cc

https://codereview.chromium.org/372943002/diff/20001/remoting/host/video_fram...
File remoting/host/video_frame_recorder_host_extension.cc (right):

https://codereview.chromium.org/372943002/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_recorder_host_extension.cc:53: ClientSession*
client_session, const protocol::ExtensionMessage& message) {
nit: one argument per line please

https://codereview.chromium.org/372943002/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_recorder_host_extension.cc:86: if (frame) {
How will client use this extension? Will it be just querying new frames
repeatedly? Maybe in case there is no frame the request needs to be queued until
the next frame?

https://codereview.chromium.org/372943002/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_recorder_host_extension.cc:107: if
(!base::JSONWriter::Write(&reply_message, &reply_json)) {
A single string with quotes on both ends is a valid JSON. So you could just add
quotes in "base64_data" and send the message that way. That would allow to avoid
at least one copy.

[Wez, 2014-07-31 23:57:46]

Jamie, PTAL!

https://codereview.chromium.org/372943002/diff/20001/remoting/host/remoting_m...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/372943002/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:128: const char
kFrameRecorderBufferKbName[] = "frame-recorder-buffer-kb";
On 2014/07/11 18:38:03, Sergey Ulanov wrote:
> Do we really need this flag? Maybe always have recorder enabled with some
> sensible buffer size. It won't do anything until recording is enabled anyway.

It's not clear what a "sensible buffer size" would be; I'd prefer not to leave a
loophole whereby a client can exhaust memory resources on constrained host
systems, for example.

https://codereview.chromium.org/372943002/diff/20001/remoting/host/video_fram...
File remoting/host/video_frame_recorder_host_extension.cc (right):

https://codereview.chromium.org/372943002/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_recorder_host_extension.cc:53: ClientSession*
client_session, const protocol::ExtensionMessage& message) {
On 2014/07/11 18:38:03, Sergey Ulanov wrote:
> nit: one argument per line please

Done.

https://codereview.chromium.org/372943002/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_recorder_host_extension.cc:86: if (frame) {
On 2014/07/11 18:38:03, Sergey Ulanov wrote:
> How will client use this extension? Will it be just querying new frames
> repeatedly? Maybe in case there is no frame the request needs to be queued
until
> the next frame?

No, the client needs to know when the last frame was received, so an empty frame
is essentially equivalent to an EOF.

https://codereview.chromium.org/372943002/diff/20001/remoting/host/video_fram...
remoting/host/video_frame_recorder_host_extension.cc:107: if
(!base::JSONWriter::Write(&reply_message, &reply_json)) {
On 2014/07/11 18:38:03, Sergey Ulanov wrote:
> A single string with quotes on both ends is a valid JSON. So you could just
add
> quotes in "base64_data" and send the message that way. That would allow to
avoid
> at least one copy.

We still need the kType==kNextFrameReplyType header, though, so I'm not sure
there's an easy way to arrange that. This isn't a performance critical path, so
I'm not too worried, and I'll likely follow-up with a patch to offline this work
to a background thread.

[Jamie, 2014-08-01 19:53:52]

You seem to be sending the video stream twice when the recording feature is
enabled. Is there no way to intercept the video frames client-side? Assuming
not, or if it's not worth the additional complexity, then LGTM.

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
File remoting/host/video_frame_recorder.h (right):

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
remoting/host/video_frame_recorder.h:55: // a new wrapper.
This seems fragile. Why can't Wrap call Detach automatically? Or are you relying
on the DCHECK to catch bad callers?

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
File remoting/host/video_frame_recorder_host_extension.cc (right):

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
remoting/host/video_frame_recorder_host_extension.cc:34: const char
kNextFrameReplyType[] = "next-frame-reply";
Are these constants duplicated somewhere in JS code? If so, then a comment here
pointing to that location might help maintainability.

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
remoting/host/video_frame_recorder_host_extension.cc:87: if (value &&
value->GetAsDictionary(&client_message)) {
Elsewhere in this function, you seem to prefer early returns for invalid data.
Would it make sense to rewrite this for consistency:

if (!value || !value->GetAsDictionary...) {
  return true;
}

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
File remoting/host/video_frame_recorder_host_extension.h (right):

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
remoting/host/video_frame_recorder_host_extension.h:22: void
SetMaxContentBytes(int64_t max_content_bytes);
If you can't pass max_content_bytes to the ctor, I think it would be safer to
initialize it to -1 and DCHECK in CreateExtensionSession to ensure that
SetMaxContentBytes has been called. The same might apply to the
VideoFrameRecorder class.

[Wez, 2014-08-01 22:33:56]

The aim of this extension is to capture pristeen frames, with no encoding
artefacts, from real sessions; we could achieve that by configuring the client
to run with "verbatim" video but that would impact session performance, and
thereby impact the frame capture rate. By copying frames into memory we minimize
the impact of the recording activity.

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
File remoting/host/video_frame_recorder.h (right):

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
remoting/host/video_frame_recorder.h:55: // a new wrapper.
On 2014/08/01 19:53:51, Jamie wrote:
> This seems fragile. Why can't Wrap call Detach automatically? Or are you
relying
> on the DCHECK to catch bad callers?

If you create a second wrapper then it'll replace the existing one, which will
stop having its frames recorded. I think it's preferable that the caller
indicate via Detach...() that that behaviour is acceptable.

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
File remoting/host/video_frame_recorder_host_extension.cc (right):

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
remoting/host/video_frame_recorder_host_extension.cc:34: const char
kNextFrameReplyType[] = "next-frame-reply";
On 2014/08/01 19:53:51, Jamie wrote:
> Are these constants duplicated somewhere in JS code? If so, then a comment
here
> pointing to that location might help maintainability.

I've added a note to the capability definition; I'll try to remember to add ones
for the others when the JS lands.

Would you like a reference at the capability definition site in the JS back to
this location? We don't currently have such references for existing
capabilities.

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
remoting/host/video_frame_recorder_host_extension.cc:87: if (value &&
value->GetAsDictionary(&client_message)) {
On 2014/08/01 19:53:52, Jamie wrote:
> Elsewhere in this function, you seem to prefer early returns for invalid data.
> Would it make sense to rewrite this for consistency:
> 
> if (!value || !value->GetAsDictionary...) {
>   return true;
> }

Done.

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
File remoting/host/video_frame_recorder_host_extension.h (right):

https://codereview.chromium.org/372943002/diff/140001/remoting/host/video_fra...
remoting/host/video_frame_recorder_host_extension.h:22: void
SetMaxContentBytes(int64_t max_content_bytes);
On 2014/08/01 19:53:52, Jamie wrote:
> If you can't pass max_content_bytes to the ctor, I think it would be safer to
> initialize it to -1 and DCHECK in CreateExtensionSession to ensure that
> SetMaxContentBytes has been called. The same might apply to the
> VideoFrameRecorder class.

VideoFrameRecorder can actually be configured with a new buffer size on-the-fly,
including coping with the buffer size being dropped to zero.

The extension doesn't make use of that currently, nor do we re-configure it
after process startup, so I'll move the parameter to the ctor.

[Wez, 2014-08-01 22:34:04]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2014-08-01 22:36:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/372943002/140001

[commit-bot: I haz the power, 2014-08-02 07:55:54]

Change committed as 287183