MediaStreamVideoSource exports current video format

MediaStreamVideoSource exports current video format

This patch adds a getter to the MediaStreamVideoSource class which
returns the current video format.

The video format details are necessary during establishing of WiFi
Display session (for the capabilities negotiation with the sink).

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

[Mikhail, 2016-02-05 15:23:22]

Description was changed from

==========
MediaStreamVideoSource exports current video format

This patch adds a getter to the MediaStreamVideoSource class which
returns the current video format.

The video format details are necessary during establishing of WiFi
Display session (for the capabilities negotiation with the sink).

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

to

==========
MediaStreamVideoSource exports current video format

This patch adds a getter to the MediaStreamVideoSource class which
returns the current video format.

The video format details are necessary during establishing of WiFi
Display session (for the capabilities negotiation with the sink).

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

[Mikhail, 2016-02-05 15:23:22]

mikhail.pozdnyakov@intel.com changed reviewers:
+ miu@chromium.org

[Mikhail, 2016-02-05 15:23:37]

PTAL

[miu, 2016-02-05 22:20:47]

https://codereview.chromium.org/1672713002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/1672713002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_source.h:90: media::VideoCaptureFormat
current_format() const { return current_format_; }
The format is invalid until the source is started.  Also, there are
thread-safety concerns here.  IMO, I think the method should look something like
the following:

  // Returns the current video format of this source, or null if it has not yet
been determined.
  const media::VideoCaptureFormat* current_format() const {
    DCHECK(CalledOnValidThread());
    if (state_ == STARTING || state_ == STARTED)
      return &current_format_;
    return nullptr;
  }

https://codereview.chromium.org/1672713002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_source.h:90: media::VideoCaptureFormat
current_format() const { return current_format_; }
Where is the code that calls this?  Note that there's a danger this method could
be removed if a project member does not see it being called from anywhere.  You
should probably just punt on this code review, and include this new method in
the change where you have code that calls it.

[Mikhail, 2016-02-08 08:13:06]

https://codereview.chromium.org/1672713002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/1672713002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_source.h:90: media::VideoCaptureFormat
current_format() const { return current_format_; }
On 2016/02/05 22:20:47, miu wrote:
> Where is the code that calls this?  Note that there's a danger this method
could
> be removed if a project member does not see it being called from anywhere. 
You
> should probably just punt on this code review, and include this new method in
> the change where you have code that calls it.

I will reference this CL when upload the client code. thanks

[Mikhail, 2016-03-11 21:08:05]

Patchset #3 (id:40001) has been deleted

[Mikhail, 2016-03-11 21:14:55]

mikhail.pozdnyakov@intel.com changed reviewers:
+ jochen@chromium.org

[Mikhail, 2016-03-11 21:17:07]

On 2016/02/08 08:13:06, Mikhail wrote:
>
https://codereview.chromium.org/1672713002/diff/1/content/renderer/media/medi...
> File content/renderer/media/media_stream_video_source.h (right):
> 
>
https://codereview.chromium.org/1672713002/diff/1/content/renderer/media/medi...
> content/renderer/media/media_stream_video_source.h:90:
media::VideoCaptureFormat
> current_format() const { return current_format_; }
> On 2016/02/05 22:20:47, miu wrote:
> > Where is the code that calls this?  Note that there's a danger this method
> could
> > be removed if a project member does not see it being called from anywhere. 
> You
> > should probably just punt on this code review, and include this new method
in
> > the change where you have code that calls it.
> 
> I will reference this CL when upload the client code. thanks

Done. Besides, public API is provided to make the functionality accessible from
the extensions code. PTAL.

[miu, 2016-03-12 01:23:15]

lgtm % one consideration.  BTW--I'll be unavailable for a week, so I'll leave it
to jochen@ to review from here.

https://codereview.chromium.org/1672713002/diff/60001/content/public/renderer...
File content/public/renderer/media_stream_track_api.h (right):

https://codereview.chromium.org/1672713002/diff/60001/content/public/renderer...
content/public/renderer/media_stream_track_api.h:20: CONTENT_EXPORT const
media::VideoCaptureFormat* GetCurrentVideoTrackFormat(
Maybe this function should go in content/public/renderer/media_stream_api.h/.cc?

(Also, note this change is renaming the file, so there might be clashing if you
make changes to it: https://codereview.chromium.org/1773273007/)

[Mikhail, 2016-03-14 09:11:01]

On 2016/03/12 01:23:15, miu_NO_REVIEWS_UNTIL_MAR_21 wrote:
> lgtm % one consideration.  BTW--I'll be unavailable for a week, so I'll leave
it
> to jochen@ to review from here.
> 
>
https://codereview.chromium.org/1672713002/diff/60001/content/public/renderer...
> File content/public/renderer/media_stream_track_api.h (right):
> 
>
https://codereview.chromium.org/1672713002/diff/60001/content/public/renderer...
> content/public/renderer/media_stream_track_api.h:20: CONTENT_EXPORT const
> media::VideoCaptureFormat* GetCurrentVideoTrackFormat(
> Maybe this function should go in
content/public/renderer/media_stream_api.h/.cc?
> 
> (Also, note this change is renaming the file, so there might be clashing if
you
> make changes to it: https://codereview.chromium.org/1773273007/)

Agree (done in the latest patch), considering renaming at crrev.com/1773273007
'media_stream_utils' looks generic enough to include the newly added API.
Thanks!

[jochen (gone - plz use gerrit), 2016-03-14 14:20:36]

lgtm

[Mikhail, 2016-03-14 14:24:42]

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

[Mikhail, 2016-03-14 14:24:43]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1672713002/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-03-14 14:24:57]

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

[commit-bot: I haz the power, 2016-03-14 14:29:43]

Description was changed from

==========
MediaStreamVideoSource exports current video format

This patch adds a getter to the MediaStreamVideoSource class which
returns the current video format.

The video format details are necessary during establishing of WiFi
Display session (for the capabilities negotiation with the sink).

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

to

==========
MediaStreamVideoSource exports current video format

This patch adds a getter to the MediaStreamVideoSource class which
returns the current video format.

The video format details are necessary during establishing of WiFi
Display session (for the capabilities negotiation with the sink).

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

[commit-bot: I haz the power, 2016-03-14 14:29:44]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-03-14 14:31:16]

Description was changed from

==========
MediaStreamVideoSource exports current video format

This patch adds a getter to the MediaStreamVideoSource class which
returns the current video format.

The video format details are necessary during establishing of WiFi
Display session (for the capabilities negotiation with the sink).

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

to

==========
MediaStreamVideoSource exports current video format

This patch adds a getter to the MediaStreamVideoSource class which
returns the current video format.

The video format details are necessary during establishing of WiFi
Display session (for the capabilities negotiation with the sink).

BUG=242107

Committed: https://crrev.com/d17b1c983f3eb6feb99dab564b21cecc6e699f80
Cr-Commit-Position: refs/heads/master@{#380969}
==========

[commit-bot: I haz the power, 2016-03-14 14:31:17]

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