Remove V4L2CaptureDelegate{Single,Multi}Plane, VCD::Client::OnIncomingCapturedYuvData()

Remove V4L2CaptureDelegate{Single,Multi}Plane, VCD::Client::OnIncomingCapturedYuvData()

this CL:

- removes both V4L2CaptureDelegateMultiPlane and
V4L2CaptureDelegateSinglePlane, leaving only the single
plane version, which is intelligently blended into
their parent class V4L2CaptureDelegate.

- Since the former class is the only user of
VideoCaptureDevice::Client::OnIncomingCapturedYuvData(),
that method is removed (yay!).

- Also cleaned up a bit here and there.

- VideoCaptureDeviceClientTest also updated and
improved.

BUG=585519
Committed: https://crrev.com/a9349677919cf6ce59045869782109ce3ac0dd19
Cr-Commit-Position: refs/heads/master@{#376200}

[mcasas, 2016-02-09 22:28:00]

Description was changed from

==========
[Experimental] Removed V4L2CaptureDelegeate{Single,Multi}Plane, and
VCD::Client::OnIncomingCaptureYuvData

BUG=585519
==========

to

==========
[EXP] Remove V4L2CaptureDelegate{Single,Multi}Plane,
VCD::Client::OnIncomingCapturedYuvData()

BUG=585519
==========

[mcasas, 2016-02-10 03:20:00]

Description was changed from

==========
[EXP] Remove V4L2CaptureDelegate{Single,Multi}Plane,
VCD::Client::OnIncomingCapturedYuvData()

BUG=585519
==========

to

==========
[EXP] Remove V4L2CaptureDelegate{Single,Multi}Plane,
VCD::Client::OnIncomingCapturedYuvData()

this CL:

- removes both V4L2CaptureDelegateMultiPlane and
V4L2CaptureDelegateSinglePlane, leaving only the single
plane version, which is intelligently blended into
their parent class V4L2CaptureDelegate. 

- Since the former class is the only user of 
VideoCaptureDevice::Client::OnIncomingCapturedYuvData(),
that method is removed (yay!).

- Also cleaned up a bit here and there, particularly
made V4L2CaptureDelegate a SupportsWeakPtr instead of
reference counted.

- VideoCaptureDeviceClientTest also updated and
improved.

BUG=585519
==========

[mcasas, 2016-02-10 06:15:55]

Patchset #5 (id:80001) has been deleted

[mcasas, 2016-02-10 19:31:08]

Patchset #5 (id:100001) has been deleted

[mcasas, 2016-02-10 19:31:18]

Patchset #4 (id:60001) has been deleted

[mcasas, 2016-02-11 15:36:25]

mcasas@chromium.org changed reviewers:
+ perkj@chromium.org, posciak@chromium.org

[mcasas, 2016-02-11 15:36:26]

posciak@, perkj@ PTAL

[perkj_chrome, 2016-02-12 11:01:40]

Nice to get rid of code. 
I think you should keep the delegate as ref counted or not make it a normal
class. Since the thread it operates on is stopped - it is guaranteed that
nothing will happen on that thread after the delegate has been destroyed anyway.

https://codereview.chromium.org/1685713003/diff/120001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1685713003/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:92:
OnLog(media::VideoPixelFormatToString(frame_format.pixel_format));
Does VideoPixelFormatToString include a description of what it is? Other wise I
think "Pixel format:" is good to have so it can be found in the log.

https://codereview.chromium.org/1685713003/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:192: //
nagative src_height is supported. The negative src_height indicates
nit s nagative negative

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
File media/capture/video/linux/v4l2_capture_delegate.cc (right):

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/v4l2_capture_delegate.cc:50: // decode. However, is
needed for large resolutions due to USB bandwidth
nit s it is needed....

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/v4l2_capture_delegate.cc:404: FROM_HERE,
base::Bind(&V4L2CaptureDelegate::DoCapture, AsWeakPtr()));
If I remember the comments correct. WeaPtrs are not that easy to work with on
different threads. They must be invalidated on the same thread as they are used.
ie, 
VideoCapureDelegate must here be deleted on v4l2_task_runner_. 
See wez comments here https://codereview.chromium.org/1464183002/


If I where you - I would make this delegate a normal class. Neither ref counted
or weak.

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
File media/capture/video/linux/v4l2_capture_delegate.h (right):

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/v4l2_capture_delegate.h:13: #include
"base/memory/ref_counted.h"
remove ref_counted.h ?

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/v4l2_capture_delegate.h:34: : public
base::SupportsWeakPtr<V4L2CaptureDelegate> {
Use weakptr factory instead to make sure weak ptrs are invalidated before any
other members. See the problem description for SupportsWeakPtr in its header.

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
File media/capture/video/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/video_capture_device_linux.cc:126:
v4l2_thread_.Stop();
actually - since this own the thread where delegate works on - the delegate
needs neither to be ref counted nor week. 

 v4l2_thread_.Stop() guarantee that nothing will happen on that thread after
Stop. And the delegate is always deleted after stop. So you can simplify it all
be using unretained and just write a comment about that.

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
File media/capture/video/linux/video_capture_device_linux.h (right):

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/video_capture_device_linux.h:52: V4L2CaptureDelegate*
capture_impl_;
this looks like a memory leak. Should be scoped_ptr - or just keep 
V4L2CaptureDelegate as beeing ref counted.

[mcasas, 2016-02-12 20:37:24]

Description was changed from

==========
[EXP] Remove V4L2CaptureDelegate{Single,Multi}Plane,
VCD::Client::OnIncomingCapturedYuvData()

this CL:

- removes both V4L2CaptureDelegateMultiPlane and
V4L2CaptureDelegateSinglePlane, leaving only the single
plane version, which is intelligently blended into
their parent class V4L2CaptureDelegate. 

- Since the former class is the only user of 
VideoCaptureDevice::Client::OnIncomingCapturedYuvData(),
that method is removed (yay!).

- Also cleaned up a bit here and there, particularly
made V4L2CaptureDelegate a SupportsWeakPtr instead of
reference counted.

- VideoCaptureDeviceClientTest also updated and
improved.

BUG=585519
==========

to

==========
Remove V4L2CaptureDelegate{Single,Multi}Plane,
VCD::Client::OnIncomingCapturedYuvData()

this CL:

- removes both V4L2CaptureDelegateMultiPlane and
V4L2CaptureDelegateSinglePlane, leaving only the single
plane version, which is intelligently blended into
their parent class V4L2CaptureDelegate. 

- Since the former class is the only user of 
VideoCaptureDevice::Client::OnIncomingCapturedYuvData(),
that method is removed (yay!).

- Also cleaned up a bit here and there.

- VideoCaptureDeviceClientTest also updated and
improved.

BUG=585519
==========

[mcasas, 2016-02-12 21:32:43]

guys PTAL

https://codereview.chromium.org/1685713003/diff/120001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1685713003/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:92:
OnLog(media::VideoPixelFormatToString(frame_format.pixel_format));
On 2016/02/12 11:01:40, perkj_chrome wrote:
> Does VideoPixelFormatToString include a description of what it is? Other wise
I
> think "Pixel format:" is good to have so it can be found in the log.

Done.

https://codereview.chromium.org/1685713003/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:192: //
nagative src_height is supported. The negative src_height indicates
On 2016/02/12 11:01:40, perkj_chrome wrote:
> nit s nagative negative

Done.

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
File media/capture/video/linux/v4l2_capture_delegate.cc (right):

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/v4l2_capture_delegate.cc:50: // decode. However, is
needed for large resolutions due to USB bandwidth
On 2016/02/12 11:01:40, perkj_chrome wrote:
> nit s it is needed....

Done.

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/v4l2_capture_delegate.cc:404: FROM_HERE,
base::Bind(&V4L2CaptureDelegate::DoCapture, AsWeakPtr()));
On 2016/02/12 11:01:40, perkj_chrome wrote:
> If I remember the comments correct. WeaPtrs are not that easy to work with on
> different threads. They must be invalidated on the same thread as they are
used.
> ie, 
> VideoCapureDelegate must here be deleted on v4l2_task_runner_. 
> See wez comments here https://codereview.chromium.org/1464183002/
> 
> 
> If I where you - I would make this delegate a normal class. Neither ref
counted
> or weak.

Added a TODO somewhere and leaving this for
another subsequent CL.

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
File media/capture/video/linux/v4l2_capture_delegate.h (right):

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/v4l2_capture_delegate.h:34: : public
base::SupportsWeakPtr<V4L2CaptureDelegate> {
On 2016/02/12 11:01:40, perkj_chrome wrote:
> Use weakptr factory instead to make sure weak ptrs are invalidated before any
> other members. See the problem description for SupportsWeakPtr in its header.

Actually on second thoughts and seeing your other 
comments on this move I think is more prudent to 
leave this class a RefCountedThreadSafe for this CL
and make it a nothing being used as base::Unretained
in another posterior CL.

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
File media/capture/video/linux/video_capture_device_linux.cc (right):

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/video_capture_device_linux.cc:126:
v4l2_thread_.Stop();
On 2016/02/12 11:01:40, perkj_chrome wrote:
> actually - since this own the thread where delegate works on - the delegate
> needs neither to be ref counted nor week. 
> 
>  v4l2_thread_.Stop() guarantee that nothing will happen on that thread after
> Stop. And the delegate is always deleted after stop. So you can simplify it
all
> be using unretained and just write a comment about that.

Acknowledged.

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
File media/capture/video/linux/video_capture_device_linux.h (right):

https://codereview.chromium.org/1685713003/diff/120001/media/capture/video/li...
media/capture/video/linux/video_capture_device_linux.h:52: V4L2CaptureDelegate*
capture_impl_;
On 2016/02/12 11:01:40, perkj_chrome wrote:
> this looks like a memory leak. Should be scoped_ptr - or just keep 
> V4L2CaptureDelegate as beeing ref counted.

Acknowledged.

[perkj_chrome, 2016-02-14 15:24:03]

lgtm

[mcasas, 2016-02-16 17:56:04]

posciak@ ping

[mcasas, 2016-02-17 17:58:05]

mcasas@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[mcasas, 2016-02-17 17:58:06]

dalecurtis@ for BUILD.gn and media.gyp microremovals

[DaleCurtis, 2016-02-17 18:10:21]

media.gyp / build.gn lgtm

[mcasas, 2016-02-17 18:11:03]

posciak@ ping 2

[mcasas, 2016-02-18 16:53:40]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-02-18 16:55:46]

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

[commit-bot: I haz the power, 2016-02-18 18:10:07]

Description was changed from

==========
Remove V4L2CaptureDelegate{Single,Multi}Plane,
VCD::Client::OnIncomingCapturedYuvData()

this CL:

- removes both V4L2CaptureDelegateMultiPlane and
V4L2CaptureDelegateSinglePlane, leaving only the single
plane version, which is intelligently blended into
their parent class V4L2CaptureDelegate. 

- Since the former class is the only user of 
VideoCaptureDevice::Client::OnIncomingCapturedYuvData(),
that method is removed (yay!).

- Also cleaned up a bit here and there.

- VideoCaptureDeviceClientTest also updated and
improved.

BUG=585519
==========

to

==========
Remove V4L2CaptureDelegate{Single,Multi}Plane,
VCD::Client::OnIncomingCapturedYuvData()

this CL:

- removes both V4L2CaptureDelegateMultiPlane and
V4L2CaptureDelegateSinglePlane, leaving only the single
plane version, which is intelligently blended into
their parent class V4L2CaptureDelegate. 

- Since the former class is the only user of 
VideoCaptureDevice::Client::OnIncomingCapturedYuvData(),
that method is removed (yay!).

- Also cleaned up a bit here and there.

- VideoCaptureDeviceClientTest also updated and
improved.

BUG=585519
==========

[commit-bot: I haz the power, 2016-02-18 18:10:08]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-02-18 18:11:25]

Description was changed from

==========
Remove V4L2CaptureDelegate{Single,Multi}Plane,
VCD::Client::OnIncomingCapturedYuvData()

this CL:

- removes both V4L2CaptureDelegateMultiPlane and
V4L2CaptureDelegateSinglePlane, leaving only the single
plane version, which is intelligently blended into
their parent class V4L2CaptureDelegate. 

- Since the former class is the only user of 
VideoCaptureDevice::Client::OnIncomingCapturedYuvData(),
that method is removed (yay!).

- Also cleaned up a bit here and there.

- VideoCaptureDeviceClientTest also updated and
improved.

BUG=585519
==========

to

==========
Remove V4L2CaptureDelegate{Single,Multi}Plane,
VCD::Client::OnIncomingCapturedYuvData()

this CL:

- removes both V4L2CaptureDelegateMultiPlane and
V4L2CaptureDelegateSinglePlane, leaving only the single
plane version, which is intelligently blended into
their parent class V4L2CaptureDelegate.

- Since the former class is the only user of
VideoCaptureDevice::Client::OnIncomingCapturedYuvData(),
that method is removed (yay!).

- Also cleaned up a bit here and there.

- VideoCaptureDeviceClientTest also updated and
improved.

BUG=585519

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

[commit-bot: I haz the power, 2016-02-18 18:11:26]

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