Modify ScreenCaptureFrameQueue into a template

Modify ScreenCaptureFrameQueue into a template

BUG=

Committed: https://crrev.com/34cad48cfbd362ae0c9027365550bfe28e2e10ef
Cr-Commit-Position: refs/heads/master@{#12458}

Committed: https://crrev.com/809dcb4b3e11f054aa72d25bef19d402e0579d96
Cr-Commit-Position: refs/heads/master@{#12478}

[Hzj_jie, 2016-04-20 17:41:24]

zijiehe@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2016-04-20 21:22:34]

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/screen_capture_queue.h (right):

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capture_queue.h:20: // Represents a queue
of reusable resources. Provides access to the 'current'
we use it for a queue of frames, so it should be clear from the comment

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capture_queue.h:35: template <typename T>
s/T/FrameType/

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capture_queue.h:36: class
ScreenCaptureQueue {
Do we need to rename this class? In the DX capturer it will still be used to
store queue of frames.

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/shared_desktop_frame.h (right):

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/shared_desktop_frame.h:27: static
std::unique_ptr<SharedDesktopFrame> Wrap(
This will break code in chromium that currently uses Wrap(). 
So to change signature of this method you need to:
 1. Add new version of Wrap(), keep the old one
 2. Wait until that change is rolled to chromium
 3. Change chromium code to use the new Wrap();
 4. Remove old Wrap() from webrtc

[Hzj_jie, 2016-04-20 23:56:16]

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/screen_capture_queue.h (right):

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capture_queue.h:20: // Represents a queue
of reusable resources. Provides access to the 'current'
On 2016/04/20 21:22:34, Sergey Ulanov wrote:
> we use it for a queue of frames, so it should be clear from the comment

Done.

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capture_queue.h:36: class
ScreenCaptureQueue {
On 2016/04/20 21:22:34, Sergey Ulanov wrote:
> Do we need to rename this class? In the DX capturer it will still be used to
> store queue of frames.

I thought this is the main point we need this change, i.e. we are moving from a
frame queue into a common queue, so MoveToNextFrame needs to be changed into
MoveToNext. Is my understanding correctly?

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/shared_desktop_frame.h (right):

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/shared_desktop_frame.h:27: static
std::unique_ptr<SharedDesktopFrame> Wrap(
On 2016/04/20 21:22:34, Sergey Ulanov wrote:
> This will break code in chromium that currently uses Wrap(). 
> So to change signature of this method you need to:
>  1. Add new version of Wrap(), keep the old one
>  2. Wait until that change is rolled to chromium
>  3. Change chromium code to use the new Wrap();
>  4. Remove old Wrap() from webrtc

Oh, I thought this function is only used in webrtc. Then I prefer to revert this
change.

[Sergey Ulanov, 2016-04-21 17:16:18]

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/screen_capture_queue.h (right):

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capture_queue.h:36: class
ScreenCaptureQueue {
On 2016/04/20 23:56:15, Hzj_jie wrote:
> On 2016/04/20 21:22:34, Sergey Ulanov wrote:
> > Do we need to rename this class? In the DX capturer it will still be used to
> > store queue of frames.
> 
> I thought this is the main point we need this change, i.e. we are moving from
a
> frame queue into a common queue, so MoveToNextFrame needs to be changed into
> MoveToNext. Is my understanding correctly?

We need this change to allow types other than DesktopFrame to be stored in the
queue. The Surface class that you adding for the new capturer is used to
represent a frame. So this is still a queue of frames.

[Hzj_jie, 2016-04-21 18:44:55]

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/screen_capture_queue.h (right):

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capture_queue.h:35: template <typename T>
On 2016/04/20 21:22:34, Sergey Ulanov wrote:
> s/T/FrameType/

Done.

https://codereview.chromium.org/1902323002/diff/90001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/screen_capture_queue.h:36: class
ScreenCaptureQueue {
On 2016/04/21 17:16:17, Sergey Ulanov wrote:
> On 2016/04/20 23:56:15, Hzj_jie wrote:
> > On 2016/04/20 21:22:34, Sergey Ulanov wrote:
> > > Do we need to rename this class? In the DX capturer it will still be used
to
> > > store queue of frames.
> > 
> > I thought this is the main point we need this change, i.e. we are moving
from
> a
> > frame queue into a common queue, so MoveToNextFrame needs to be changed into
> > MoveToNext. Is my understanding correctly?
> 
> We need this change to allow types other than DesktopFrame to be stored in the
> queue. The Surface class that you adding for the new capturer is used to
> represent a frame. So this is still a queue of frames.

Done.

[Sergey Ulanov, 2016-04-21 18:53:18]

lgtm

[Hzj_jie, 2016-04-21 19:38:41]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-04-21 19:38:46]

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

[commit-bot: I haz the power, 2016-04-21 20:25:04]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2016-04-21 20:25:11]

Description was changed from

==========
Modify ScreenCaptureFrameQueue into a template

BUG=
==========

to

==========
Modify ScreenCaptureFrameQueue into a template

BUG=

Committed: https://crrev.com/34cad48cfbd362ae0c9027365550bfe28e2e10ef
Cr-Commit-Position: refs/heads/master@{#12458}
==========

[commit-bot: I haz the power, 2016-04-21 20:25:12]

Patchset 10 (id:??) landed as
https://crrev.com/34cad48cfbd362ae0c9027365550bfe28e2e10ef
Cr-Commit-Position: refs/heads/master@{#12458}

[torbjorng, 2016-04-21 21:07:06]

A revert of this CL (patchset #10 id:170001) has been created in
https://codereview.webrtc.org/1910203002/ by torbjorng@chromium.org.

The reason for reverting is: Breaks FYI bits, e.g. this one:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win%20Builder/build....

[Hzj_jie, 2016-04-22 18:11:22]

Description was changed from

==========
Modify ScreenCaptureFrameQueue into a template

BUG=

Committed: https://crrev.com/34cad48cfbd362ae0c9027365550bfe28e2e10ef
Cr-Commit-Position: refs/heads/master@{#12458}
==========

to

==========
Modify ScreenCaptureFrameQueue into a template

BUG=

Committed: https://crrev.com/34cad48cfbd362ae0c9027365550bfe28e2e10ef
Cr-Commit-Position: refs/heads/master@{#12458}
==========

[Hzj_jie, 2016-04-22 18:43:37]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-04-22 18:43:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1902323002/#ps200001
(title: "Fix build break in Chromium")

[commit-bot: I haz the power, 2016-04-22 18:43:43]

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

[Hzj_jie, 2016-04-22 19:08:35]

On 2016/04/22 18:43:43, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1902323002/200001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1902323002/200001

Manually build on my machine with Chromium, passed. Try to land again.

[commit-bot: I haz the power, 2016-04-22 20:11:59]

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

[commit-bot: I haz the power, 2016-04-22 20:11:59]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Hzj_jie, 2016-04-22 23:06:53]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-04-22 23:07:07]

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

[commit-bot: I haz the power, 2016-04-22 23:08:43]

Description was changed from

==========
Modify ScreenCaptureFrameQueue into a template

BUG=

Committed: https://crrev.com/34cad48cfbd362ae0c9027365550bfe28e2e10ef
Cr-Commit-Position: refs/heads/master@{#12458}
==========

to

==========
Modify ScreenCaptureFrameQueue into a template

BUG=

Committed: https://crrev.com/34cad48cfbd362ae0c9027365550bfe28e2e10ef
Cr-Commit-Position: refs/heads/master@{#12458}
==========

[commit-bot: I haz the power, 2016-04-22 23:08:44]

Committed patchset #12 (id:200001)

[commit-bot: I haz the power, 2016-04-22 23:08:52]

Description was changed from

==========
Modify ScreenCaptureFrameQueue into a template

BUG=

Committed: https://crrev.com/34cad48cfbd362ae0c9027365550bfe28e2e10ef
Cr-Commit-Position: refs/heads/master@{#12458}
==========

to

==========
Modify ScreenCaptureFrameQueue into a template

BUG=

Committed: https://crrev.com/34cad48cfbd362ae0c9027365550bfe28e2e10ef
Cr-Commit-Position: refs/heads/master@{#12458}

Committed: https://crrev.com/809dcb4b3e11f054aa72d25bef19d402e0579d96
Cr-Commit-Position: refs/heads/master@{#12478}
==========

[commit-bot: I haz the power, 2016-04-22 23:08:52]

Patchset 12 (id:??) landed as
https://crrev.com/809dcb4b3e11f054aa72d25bef19d402e0579d96
Cr-Commit-Position: refs/heads/master@{#12478}