cc: Video layers without a frame should not occlude

cc: Video layers without a frame should not occlude

Added VisibleContentOpaqueRegion() to the VideoLayerImpl, if
a frame hasn't been set yet, then we don't have an opaque region.

+ test

BUG=460612
Committed: https://crrev.com/e19cc6a61419d38f3ec658c5e74363d77e900c09
Cr-Commit-Position: refs/heads/master@{#330449}

[hendrikw, 2015-05-18 16:09:35]

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

[hendrikw, 2015-05-18 16:10:53]

hendrikw@chromium.org changed reviewers:
+ vmpstr@chromium.org

[hendrikw, 2015-05-18 16:11:10]

PTAL, thanks!

[danakj, 2015-05-18 17:19:52]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-05-18 17:19:52]

https://codereview.chromium.org/1134663005/diff/20001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1134663005/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider_client_impl.cc:103: return provider_ &&
provider_->GetCurrentFrame();
You need to match calls to GetCurrentFrame() with PutCurrentFrame(), but I think
it might be nicer to add a HasCurrentFrame() to the VideoFrameProvider as well.
Or is that horrible?

[hendrikw, 2015-05-18 17:46:58]

https://codereview.chromium.org/1134663005/diff/20001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1134663005/diff/20001/cc/layers/video_frame_p...
cc/layers/video_frame_provider_client_impl.cc:103: return provider_ &&
provider_->GetCurrentFrame();
On 2015/05/18 17:19:52, danakj wrote:
> You need to match calls to GetCurrentFrame() with PutCurrentFrame(), but I
think
> it might be nicer to add a HasCurrentFrame() to the VideoFrameProvider as
well.
> Or is that horrible?

That's an weird requirement.  WebMediaPlayerMS::GetCurrentFrame changes state :/
 I don't like. I'll add the HasCurrentFrame() to the provider.

[hendrikw, 2015-05-18 18:13:20]

Added HasCurrentFrame to VideoFrameProvider. PTAL

[danakj, 2015-05-18 18:15:46]

https://codereview.chromium.org/1134663005/diff/40001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1134663005/diff/40001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:70: virtual bool HasCurrentFrame() const = 0;
can you comment what this is?

https://codereview.chromium.org/1134663005/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1134663005/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1284: return
current_frame_;
you need to lock current_frame_lock_ no?

https://codereview.chromium.org/1134663005/diff/40001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/1134663005/diff/40001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:468: return current_frame_;
and here?

https://codereview.chromium.org/1134663005/diff/40001/media/blink/video_frame...
File media/blink/video_frame_compositor.cc (right):

https://codereview.chromium.org/1134663005/diff/40001/media/blink/video_frame...
media/blink/video_frame_compositor.cc:128: return current_frame_;
can you DCHECK the thread here also?

[hendrikw, 2015-05-18 18:34:00]

PTAL (+question)

https://codereview.chromium.org/1134663005/diff/40001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1134663005/diff/40001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:70: virtual bool HasCurrentFrame() const = 0;
On 2015/05/18 18:15:45, danakj wrote:
> can you comment what this is?

Done.

https://codereview.chromium.org/1134663005/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1134663005/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1284: return
current_frame_;
On 2015/05/18 18:15:45, danakj wrote:
> you need to lock current_frame_lock_ no?

Yeah, I was unsure about this.  We have a lock in the caller, and
VideoFrameCompositor doesn't lock on the GetCurrentFrame.  
Calling (bool)scoped_ptr is atomic.

I can play it safe and put the locks in here, but I'll lose constness.

Let me know that you think.

https://codereview.chromium.org/1134663005/diff/40001/media/blink/video_frame...
File media/blink/video_frame_compositor.cc (right):

https://codereview.chromium.org/1134663005/diff/40001/media/blink/video_frame...
media/blink/video_frame_compositor.cc:128: return current_frame_;
On 2015/05/18 18:15:45, danakj wrote:
> can you DCHECK the thread here also?

Good catch, Done.

[danakj, 2015-05-18 18:44:57]

https://codereview.chromium.org/1134663005/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1134663005/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1284: return
current_frame_;
On 2015/05/18 18:33:59, hendrikw wrote:
> On 2015/05/18 18:15:45, danakj wrote:
> > you need to lock current_frame_lock_ no?
> 
> Yeah, I was unsure about this.  We have a lock in the caller, and
> VideoFrameCompositor doesn't lock on the GetCurrentFrame.  

I /think/ in VFC the lock_ is locking access to the callback_ and not the
current_frame_. I guess it doesn't access the frame from other threads.

> Calling (bool)scoped_ptr is atomic.

What makes you say it's atomic? It's implemented as:

return impl_.get() ? &scoped_ptr::impl_ : nullptr;

> I can play it safe and put the locks in here, but I'll lose constness.

I think we need to for correctness.

[hendrikw, 2015-05-18 19:59:23]

PTAL

[DaleCurtis, 2015-05-18 20:32:58]

media/ usage lgtm though the Windows bots might complain about returning a
scoped_refptr as bool.

[danakj, 2015-05-18 21:04:56]

LGTM

[hendrikw, 2015-05-18 21:42:33]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-05-18 21:44:15]

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

[commit-bot: I haz the power, 2015-05-18 22:00:43]

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

[commit-bot: I haz the power, 2015-05-18 22:00:44]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[hendrikw, 2015-05-18 22:05:05]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-05-18 22:05:05]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1134663005/#ps120001
(title: "missed const on android")

[commit-bot: I haz the power, 2015-05-18 22:06:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134663005/120001

[commit-bot: I haz the power, 2015-05-18 23:14:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-18 23:15:15]

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