Adding error signalling from device to VideocaptureManager to relay up to MediaStream and WebKit.

content/common/media/media_stream_options.h

+// Copyright (c) 20101 The Chromium Authors. All rights reserved.

[John Knottenbelt, 2011/06/16 15:20:43]

nit: 20101 => 2011

[mflodman1, 2011/06/20 19:48:03]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_COMMON_MEDIA_MEDIA_STREAM_OPTIONS_H_

[scherkus (not reviewing), 2011/06/17 03:03:52]

sanity check... does this have to be inside of content/common/ ?

I don't think I see any code inside of content/renderer yet but perhaps there
will be soon :)

in any case, could you add an OWNERS file w/ myself listed for the time being?

[mflodman1, 2011/06/20 19:48:03]

The structs in this file will be used to requesting capture device usage and to
signal the result of the requests.

OWNERS added.

+#define CONTENT_COMMON_MEDIA_MEDIA_STREAM_OPTIONS_H_
+
+#include <string>
+#include <vector>
+
+namespace media_stream {

[scherkus (not reviewing), 2011/06/17 03:03:52]

nit: content/ doesn't typically use namespaces, but up to you folks

you don't have to make this change now

[mflodman1, 2011/06/20 19:48:03]

Good to know, noted and we'll look at this later.

+
+// GenerateStreamOptions is a Chromium representation of WebKits
+// WebGenerateStreamOptionFlags. It describes the components in a request for a
+// new media stream.
+struct GenerateStreamOptions {

[scherkus (not reviewing), 2011/06/17 03:03:52]

nit: the name of this struct is rather confusing as it reads as a verb and not a
noun

is that intentional?

[mflodman1, 2011/06/20 19:48:03]

Changed to only 'StreamOptions'.

+  enum VideoOptionFlags {
+    kVideoNoCamera = 0,

[scherkus (not reviewing), 2011/06/17 03:03:52]

nit: "Video" prefix is redundant considering this is defined inside of a struct

[mflodman1, 2011/06/20 19:48:03]

Done.

+    kVideoFacingUser = 1,
+    kVideoFacingEnvironment = 1 << 1,

[scherkus (not reviewing), 2011/06/17 03:03:52]

considering the amount of flags is relatively small, what about enforcing type
safety by having all 4 options:
kNone
kFacingUser
kFacingEnvironment
kFacingBoth

...or something like that

then you do w/o the unsigned typedef and things are self describing, etc..

also in the kFacingBoth case... are you requesting that you want both cameras or
that you want either camera (i.e,. whatever we can satisfy).

[mflodman1, 2011/06/20 19:48:03]

I tried changing but ran into IPC issues when building with the code that will
be uploaded after this is done. We, i.e. Per and I, looked at the IPC messages
and defined what we thought is correct, but obviously something was missing.
I'll have another look at this tomorrow.

+  };
+
+  typedef unsigned VideoOptions;
+
+  GenerateStreamOptions() : audio(false), video_options(kVideoNoCamera) {}

[scherkus (not reviewing), 2011/06/17 03:03:52]

does having a default ctor make sense or will all callees be calling the other
ctor?

[mflodman1, 2011/06/20 19:48:03]

We don't explicitly call this ctor and I tried to remove it,  resulting in
errors for no matching function to the remove ctor (this is from the IPC part of
media stream, which will be uploaded after this patch):
In file included from ./ipc/ipc_message_utils.h:19,
                 from ./ipc/ipc_message_macros.h:179,
                 from ./content/common/child_process_messages.h:9,
                 from ./content/common/content_message_generator.h:7,
                 from content/common/content_message_generator.cc:7:
./base/tuple.h: In constructor ‘Tuple4<A, B, C, D>::Tuple4() [with A = int, B =
int, C = media_stream::StreamOptions, D = std::basic_string<char,
std::char_traits<char>, std::allocator<char> >]’:
./content/common/media_stream_messages.h:60:   instantiated from here
./base/tuple.h:136: error: no matching function for call to
‘media_stream::StreamOptions::StreamOptions()’

Any ideas? I'm far from an IPC expert.

Thanks!

[scherkus (not reviewing), 2011/06/21 00:27:46]

Ahh I wasn't aware this ended up getting used in IPC messages... in which I
don't think we can avoid it.

The alternative would be to use a define a different struct in the IPC layer,
but I don't think it's strictly necessary.

+  GenerateStreamOptions(bool audio, VideoOptions options)
+      : audio(audio), video_options(options) {}
+
+  // True if the stream shall contain an audio input stream.
+  bool audio;
+
+  // Describes if a / which type of video capture device is requested.
+  VideoOptions video_options;
+};
+
+// Type of media stream.
+enum MediaStreamType {
+  kNoService = 0,
+  kAudioCapture,
+  kVideoCapture
+};
+
+// StreamDeviceInfo describes information about a device.
+struct StreamDeviceInfo {
+  StreamDeviceInfo();

[scherkus (not reviewing), 2011/06/17 03:03:52]

nit: in general if you all callees properly initialize the struct then you can
do without this ctor

otherwise you get stuck with an ill-defined ctor (i.e., what does a default
StreamDeviceInfo represent?)

[mflodman1, 2011/06/20 19:48:03]

Same as for removing StreamOptions ctor above, I'll have another look at this
tomorrow.

+  StreamDeviceInfo(MediaStreamType service_param,
+                   const std::string name_param,

[scherkus (not reviewing), 2011/06/17 03:03:52]

make const-ref

[mflodman1, 2011/06/20 19:48:03]

Done.

+                   const std::string device_param,

[scherkus (not reviewing), 2011/06/17 03:03:52]

ditto

[mflodman1, 2011/06/20 19:48:03]

Done.

+                   bool opened);
+
+  enum { kNoId = -1};

[scherkus (not reviewing), 2011/06/17 03:03:52]

nit: space before }

[mflodman1, 2011/06/20 19:48:03]

Done.

+
+  // Describes the capture type.
+  MediaStreamType stream_type;
+  // Friendly name of the device.
+  std::string name;
+  // Unique name of a device. Even if there are multiple devices with the same
+  // friendly name connected to the computer, this will be unique.
+  std::string device_id;

[scherkus (not reviewing), 2011/06/17 03:03:52]

these two parameters seem similar to what's defined under
media/audio/audio_device_name.h

is that intentional?

[mflodman1, 2011/06/20 19:48:03]

Yes. This struct will be used to signal name/id for both audio capture devices
and video capture devices to the WebKit glue to keep track of open devices.

+  // Set to true if the device has been opened, false otherwise.
+  bool in_use;
+  // Id for this capture session. Unique for all sessions of the same type.
+  int session_id;
+};
+
+typedef std::vector<StreamDeviceInfo> StreamDeviceInfoArray;
+
+}  // namespace media_stream
+
+#endif  // CONTENT_COMMON_MEDIA_MEDIA_STREAM_OPTIONS_H_
+

[scherkus (not reviewing), 2011/06/17 03:03:52]

nit: get rid of blank line

[mflodman1, 2011/06/20 19:48:03]

Done.