WebRtcMediaStreamTrackMap added

content/renderer/media/webrtc/webrtc_media_stream_track_collection.h

Index: content/renderer/media/webrtc/webrtc_media_stream_track_collection.h
diff --git a/content/renderer/media/webrtc/webrtc_media_stream_track_collection.h b/content/renderer/media/webrtc/webrtc_media_stream_track_collection.h
new file mode 100644
index 0000000000000000000000000000000000000000..33cc0d3901b9df72c1d172d9b25c2a019b0d55b2
--- /dev/null
+++ b/content/renderer/media/webrtc/webrtc_media_stream_track_collection.h
@@ -0,0 +1,69 @@
+// Copyright (c) 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_RENDERER_MEDIA_WEBRTC_WEBRTC_MEDIA_STREAM_TRACK_COLLECTION_H_
+#define CONTENT_RENDERER_MEDIA_WEBRTC_WEBRTC_MEDIA_STREAM_TRACK_COLLECTION_H_
+
+#include <map>
+#include <string>
+
+#include "base/memory/ref_counted.h"
+#include "base/single_thread_task_runner.h"
+#include "content/common/content_export.h"
+#include "content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h"
+#include "third_party/WebKit/public/platform/WebMediaStreamTrack.h"
+#include "third_party/webrtc/api/mediastreaminterface.h"
+
+namespace content {
+
+class PeerConnectionDependencyFactory;
+
+class CONTENT_EXPORT WebRtcMediaStreamTrackCollection

[Guido Urdaneta, 2017/05/22 16:14:47]

Would WebRtcMediaStreamTrackMap be a more descriptive name?
Also, add a comment explaining what the class does.

[hbos_chromium, 2017/05/29 13:28:51]

Done. Changed to WebRtcMediaStreamTrackAdapterMap.

+    : public base::RefCountedThreadSafe<WebRtcMediaStreamTrackCollection> {
+ public:
+  WebRtcMediaStreamTrackCollection(
+      PeerConnectionDependencyFactory* const factory,
+      const scoped_refptr<base::SingleThreadTaskRunner>& main_thread);
+
+  scoped_refptr<WebRtcMediaStreamTrackAdapter> GetLocalTrack(

[Guido Urdaneta, 2017/05/22 16:14:45]

Add comment. What happens if the id is not found?

[hbos_chromium, 2017/05/29 13:28:51]

Done.

+      const std::string& id) const;
+  // Invoke on the main thread. Newly created tracks are initialized when
+  // returned.
+  scoped_refptr<WebRtcMediaStreamTrackAdapter> GetOrCreateLocalTrack(
+      const blink::WebMediaStreamTrack& web_track);
+  bool RemoveAndUninitializeLocalTrack(const std::string& id);

[Guido Urdaneta, 2017/05/22 16:14:44]

Add a comment explaining the meaning of the result.

[hbos_chromium, 2017/05/29 13:28:50]

Done.

+  size_t GetLocalTrackCount() const;
+
+  scoped_refptr<WebRtcMediaStreamTrackAdapter> GetRemoteTrack(
+      const std::string& id) const;
+  // Invoke on the webrtc signaling thread. Newly created tracks are initialized

[Guido Urdaneta, 2017/05/22 16:14:47]

Since this function has no callback, the detail about initialization being in a
post to the main thread seems unnecessary.

[hbos_chromium, 2017/05/29 13:28:50]

It is relevant because it would be an error to try to use the adapter (its
web_track or webrtc_track) before it is initialized and this function does not
guarantee that it has completed initialization when returned, only that there is
an adapter for the track.

A callback could be used but considering the initialization is completed in a
post and this is documented, any subsequent post to the main thread would occur
after it has been initialized. That is, if you want a "callback" you can post
one yourself.

Updated comment.

[Guido Urdaneta, 2017/05/30 09:02:41]

Acknowledged.

+  // in a post to the main thread.
+  scoped_refptr<WebRtcMediaStreamTrackAdapter> GetOrCreateRemoteTrack(
+      webrtc::MediaStreamTrackInterface* webrtc_track);
+  bool RemoveAndUninitializeRemoteTrack(const std::string& id);
+  size_t GetRemoteTrackCount() const;
+
+  void ClearAndUninitializeTracks();

[Guido Urdaneta, 2017/05/22 16:14:44]

Since there are no versions of these methods without the "AndUninitialize" part,
can you just remove that part from the method names?

[hbos_chromium, 2017/05/29 13:28:50]

Done. Renamed to Dispose[Local/Remote]TrackAdapter.

+
+ protected:
+  friend class base::RefCountedThreadSafe<WebRtcMediaStreamTrackCollection>;
+
+  virtual ~WebRtcMediaStreamTrackCollection();
+
+ private:
+  // Pointer to a |PeerConnectionDependencyFactory| owned by the |RenderThread|.
+  // It's valid for the lifetime of |RenderThread|.
+  PeerConnectionDependencyFactory* const factory_;
+  scoped_refptr<base::SingleThreadTaskRunner> main_thread_;
+
+  mutable base::Lock lock_;

[Guido Urdaneta, 2017/05/22 16:14:46]

Would having a separate lock for each map have any practical effect on allowing
more concurrency?

[hbos_chromium, 2017/05/29 13:28:51]

Contention is unlikely. I prefer to have a single lock here for simplicity
because I don't think there are any practical benefits to having multiple locks,
though I don't have a strong preference.

[Guido Urdaneta, 2017/05/30 09:02:41]

Acknowledged.

+  std::map<std::string, scoped_refptr<WebRtcMediaStreamTrackAdapter>>
+      local_tracks_;
+  std::map<std::string, scoped_refptr<WebRtcMediaStreamTrackAdapter>>
+      remote_tracks_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_RENDERER_MEDIA_WEBRTC_WEBRTC_MEDIA_STREAM_TRACK_COLLECTION_H_