Prototype HTMLVideoElement properties for WebGL texImage2D

Prototype HTMLVideoElement properties for WebGL texImage2D

These properties provide the width, height, and time of the last video
frame uploaded to a WebGL texture via texImage2D.

BUG=639174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2749653003
Cr-Commit-Position: refs/heads/master@{#460606}
Committed: https://chromium.googlesource.com/chromium/src/+/36eeff88e474d2e562f2d656bb94d0e37770e245

[Kai Ninomiya, 2017-03-14 20:16:04]

Description was changed from

==========
Prototype HTMLVideoElement properties for WebGL texImage2D

These properties provide the width, height, and time of the last video
frame uploaded to a texture via texImage2D.

BUG=639174
==========

to

==========
Prototype HTMLVideoElement properties for WebGL texImage2D

These properties provide the width, height, and time of the last video
frame uploaded to a texture via texImage2D.

BUG=639174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Kai Ninomiya, 2017-03-14 20:38:42]

kainino@chromium.org changed reviewers:
+ kbr@chromium.org

[Kai Ninomiya, 2017-03-14 20:38:43]

kbr: is this what you had in mind?

[DaleCurtis, 2017-03-14 20:50:16]

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

[DaleCurtis, 2017-03-14 20:50:16]

Hmm, don't you want the timestamp from the actual VideoFrame object?

[Kai Ninomiya, 2017-03-14 20:51:30]

On 2017/03/14 20:50:16, DaleCurtis wrote:
> Hmm, don't you want the timestamp from the actual VideoFrame object?

That sounds correct, thank you for the pointer. I'll look into it.

[Kai Ninomiya, 2017-03-14 21:43:33]

Description was changed from

==========
Prototype HTMLVideoElement properties for WebGL texImage2D

These properties provide the width, height, and time of the last video
frame uploaded to a texture via texImage2D.

BUG=639174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Prototype HTMLVideoElement properties for WebGL texImage2D

These properties provide the width, height, and time of the last video
frame uploaded to a WebGL texture via texImage2D.

BUG=639174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Kai Ninomiya, 2017-03-14 23:24:19]

On 2017/03/14 20:51:30, Kai Ninomiya wrote:
> On 2017/03/14 20:50:16, DaleCurtis wrote:
> > Hmm, don't you want the timestamp from the actual VideoFrame object?
> 
> That sounds correct, thank you for the pointer. I'll look into it.

@anyone: Updated, does this seem correct? It's pretty hacky and certainly won't
work in all cases (e.g. webmediaplayer_ms), but I'm not sure what we need right
at the moment.

[DaleCurtis, 2017-03-15 02:50:50]

No, this needs to be returned along with the paint() call or it's useless since
it may belong to a different frame.

[Ken Russell (switch to Gerrit), 2017-03-15 04:41:50]

Thanks Kai, yes, this is basically what I had in mind. A comment.

https://codereview.chromium.org/2749653003/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2749653003/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:708: scoped_refptr<VideoFrame> video_frame =
GetCurrentFrameFromCompositor();
As Dale points out, this isn't guaranteed to be correct because the current
frame may have changed since the point where the HTMLVideoElement called into
the WebMediaPlayer.

Instead, I suggest adding a helper method which records the width, height and
timestamp from a passed VideoFrame along the code paths for the three entry
points -- copyVideoTextureToPlatformTexture, paint, and texImageImpl -- and then
have getCurrentFrameInfo return those cached values.

[Kai Ninomiya, 2017-03-15 21:20:18]

kbr, dalecurtis: updated again, WDYT?

[DaleCurtis, 2017-03-15 22:53:50]

Still seems kind of roundabout, why not add some setters to the canvas object so
it can be set at render time instead of through a secondary method?

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:459: // Updates the cached video frame info
for getLastUploadedFrameInfo.
No need for method, just update at call site for get current frame.

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:720: unsigned last_uploaded_frame_width_ = 0;
gfx::Size and TimeDelta

[Ken Russell (switch to Gerrit), 2017-03-16 00:57:43]

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:459: // Updates the cached video frame info
for getLastUploadedFrameInfo.
On 2017/03/15 22:53:50, DaleCurtis wrote:
> No need for method, just update at call site for get current frame.

Thanks. The idea was to minimize the number of places where this would be done,
but if it's OK to do it all the time in GetCurrentFrameFromCompositor, that's
even easier.

[Kai Ninomiya, 2017-03-16 22:28:25]

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:459: // Updates the cached video frame info
for getLastUploadedFrameInfo.
On 2017/03/16 00:57:43, Ken Russell wrote:
> On 2017/03/15 22:53:50, DaleCurtis wrote:
> > No need for method, just update at call site for get current frame.
> 
> Thanks. The idea was to minimize the number of places where this would be
done,
> but if it's OK to do it all the time in GetCurrentFrameFromCompositor, that's
> even easier.

dalecurtis: Are you suggesting updating the (now 2) variables outside or inside
GetCurrentFrameFromCompositor?

It's possible we may end up needing to change the set of last-uploaded
variables, so I'd rather keep it in one place. Either
UpdateLastUploadedFrameInfo, or inside GetCurrentFrameFromCompositor.

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:720: unsigned last_uploaded_frame_width_ = 0;
On 2017/03/15 22:53:50, DaleCurtis wrote:
> gfx::Size and TimeDelta

Acknowledged.

[DaleCurtis, 2017-03-16 22:30:02]

That said, as mentioned in my previous comment I think this way is funky.
Instead it seems like the Canvas passed into paint() should have setters on it
for these fields.

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:459: // Updates the cached video frame info
for getLastUploadedFrameInfo.
On 2017/03/16 at 22:28:25, Kai Ninomiya wrote:
> On 2017/03/16 00:57:43, Ken Russell wrote:
> > On 2017/03/15 22:53:50, DaleCurtis wrote:
> > > No need for method, just update at call site for get current frame.
> > 
> > Thanks. The idea was to minimize the number of places where this would be
done,
> > but if it's OK to do it all the time in GetCurrentFrameFromCompositor,
that's
> > even easier.
> 
> dalecurtis: Are you suggesting updating the (now 2) variables outside or
inside GetCurrentFrameFromCompositor?
> 
> It's possible we may end up needing to change the set of last-uploaded
variables, so I'd rather keep it in one place. Either
UpdateLastUploadedFrameInfo, or inside GetCurrentFrameFromCompositor.

Inside.

[Kai Ninomiya, 2017-03-16 22:39:49]

On 2017/03/16 22:30:02, DaleCurtis wrote:
> That said, as mentioned in my previous comment I think this way is funky.
> Instead it seems like the Canvas passed into paint() should have setters on it
> for these fields.

Sorry, forgot to reply about this. Canvas isn't used for texImageImpl and
copyVideoTextureToPlatformTexture, so it would only work for one of the upload
paths. The only other alternative I was able to think of was to pass down an
extra argument or two through 2-3 layers of functions for 3 different upload
paths.

[DaleCurtis, 2017-03-16 22:47:42]

Ah, sorry, this is using copyVideoTextureToPlatformTexture() then? Mostly my
point is that instead of adding another method to get the values they should be
returned along with whatever is retrieving the frame so they are always in lock
step.

[Ken Russell (switch to Gerrit), 2017-03-16 23:01:05]

On 2017/03/16 22:47:42, DaleCurtis wrote:
> Ah, sorry, this is using copyVideoTextureToPlatformTexture() then? Mostly my
> point is that instead of adding another method to get the values they should
be
> returned along with whatever is retrieving the frame so they are always in
lock
> step.

The frame isn't returned to JavaScript as an entity. An API call is made against
WebGLRenderingContext which copies the HTMLVideoElement's current frame into a
WebGLTexture. The YouTube team wants to be able to query the last uploaded
frame's size and timestamp.

The intent of Kai's patch is to provide an experimental way to query the values
accurately, from HTMLVideoElement. We could hang these experimental properties
off the WebGLTexture class, come to think of it, but up until this point, these
have been treated as opaque objects that have no real properties.

We're going to recast this functionality in some other way, but that's further
down the road, so to let the YouTube team experiment, we need to expose these
somewhere, behind a flag.

Let us know if you feel strongly that these experimental properties shouldn't be
hung off HTMLVideoElement, but instead for example off WebGLTexture.

[DaleCurtis, 2017-03-16 23:13:21]

I think it makes sense to hang them off nearest to wherever they're used and can
be updated atomically. Putting them on another method like this makes me worry
about raciness and layering. It seems like you're saying that the upload action
is user controlled, so these properties will be valid as long as they don't call
upload again?

[Ken Russell (switch to Gerrit), 2017-03-16 23:29:30]

On 2017/03/16 23:13:21, DaleCurtis wrote:
> I think it makes sense to hang them off nearest to wherever they're used and
can
> be updated atomically. Putting them on another method like this makes me worry
> about raciness and layering. It seems like you're saying that the upload
action
> is user controlled, so these properties will be valid as long as they don't
call
> upload again?

Yes, the upload action is user controlled.

Kai, could you try putting these properties on the WebGLTexture class instead,
and let's see how that looks? Thanks.

[Kai Ninomiya, 2017-03-16 23:31:01]

On 2017/03/16 23:29:30, Ken Russell wrote:
> On 2017/03/16 23:13:21, DaleCurtis wrote:
> > I think it makes sense to hang them off nearest to wherever they're used and
> can
> > be updated atomically. Putting them on another method like this makes me
worry
> > about raciness and layering. It seems like you're saying that the upload
> action
> > is user controlled, so these properties will be valid as long as they don't
> call
> > upload again?
> 
> Yes, the upload action is user controlled.
> 
> Kai, could you try putting these properties on the WebGLTexture class instead,
> and let's see how that looks? Thanks.

OK, sure.

[DaleCurtis, 2017-03-16 23:53:42]

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

[DaleCurtis, 2017-03-16 23:53:43]

Thanks for experimenting. Since this is behind the experimental flag I think
either approach is okay, but +mlamouri to weigh in as the blink side media
owner.

[Kai Ninomiya, 2017-03-17 21:40:15]

Slightly simplified patch and moved the fields to WebGLTexture instead of
HTMLVideoElement.

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.h (right):

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:459: // Updates the cached video frame info
for getLastUploadedFrameInfo.
On 2017/03/16 22:30:02, DaleCurtis wrote:
> On 2017/03/16 at 22:28:25, Kai Ninomiya wrote:
> > On 2017/03/16 00:57:43, Ken Russell wrote:
> > > On 2017/03/15 22:53:50, DaleCurtis wrote:
> > > > No need for method, just update at call site for get current frame.
> > > 
> > > Thanks. The idea was to minimize the number of places where this would be
> done,
> > > but if it's OK to do it all the time in GetCurrentFrameFromCompositor,
> that's
> > > even easier.
> > 
> > dalecurtis: Are you suggesting updating the (now 2) variables outside or
> inside GetCurrentFrameFromCompositor?
> > 
> > It's possible we may end up needing to change the set of last-uploaded
> variables, so I'd rather keep it in one place. Either
> UpdateLastUploadedFrameInfo, or inside GetCurrentFrameFromCompositor.
> 
> Inside.

Done.

https://codereview.chromium.org/2749653003/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.h:720: unsigned last_uploaded_frame_width_ = 0;
On 2017/03/16 22:28:25, Kai Ninomiya wrote:
> On 2017/03/15 22:53:50, DaleCurtis wrote:
> > gfx::Size and TimeDelta
> 
> Acknowledged.

Done.

[Kai Ninomiya, 2017-03-17 21:41:54]

On 2017/03/17 21:40:15, Kai Ninomiya wrote:
> Slightly simplified patch and moved the fields to WebGLTexture instead of
> HTMLVideoElement.

I should clarify: this doesn't make the implementation any less complex (I don't
see a way of doing that). But it does keep the changes more localized to
modules/webgl.

[DaleCurtis, 2017-03-17 21:47:52]

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5288: if
(video->copyVideoTextureToPlatformTexture(contextGL(), texture->object(),
Sorry, can you instead add the parameters you want to get to this call? I.e.
copyVideoTextureToPlatformTexture() should now take pointers for width, height,
and timestamp. We want to avoid any method that is not this call from providing
parameters.

[Kai Ninomiya, 2017-03-18 01:08:58]

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5288: if
(video->copyVideoTextureToPlatformTexture(contextGL(), texture->object(),
Hm, adding it to copyVideoTextureToPlatformTexture and texImageImpl wouldn't be
hard (each only has one caller). But I may be missing something here: I think it
would have to also be added to paintCurrentFrame for the other paths, which has
6 callers and would be a little more intrusive than I want for a prototype.

Perhaps this could be implemented only for those 2 upload paths, but that might
not be reliable enough for YouTube's testing. I'm not sure what formats take
those paths on what platforms.

[DaleCurtis, 2017-03-20 17:58:13]

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5288: if
(video->copyVideoTextureToPlatformTexture(contextGL(), texture->object(),
On 2017/03/18 at 01:08:58, Kai Ninomiya wrote:
> Hm, adding it to copyVideoTextureToPlatformTexture and texImageImpl wouldn't
be hard (each only has one caller). But I may be missing something here: I think
it would have to also be added to paintCurrentFrame for the other paths, which
has 6 callers and would be a little more intrusive than I want for a prototype.
> 
> Perhaps this could be implemented only for those 2 upload paths, but that
might not be reliable enough for YouTube's testing. I'm not sure what formats
take those paths on what platforms.

Is there any other way to upload a frame? Also 6 callers isn't that much; plus
if you really want to Blink allows default values, so you can just set the new
parameters as nullptr for optional extraction.

[Ken Russell (switch to Gerrit), 2017-03-20 19:14:15]

I think this minimally intrusive approach is the best one for this prototype.

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5288: if
(video->copyVideoTextureToPlatformTexture(contextGL(), texture->object(),
On 2017/03/20 17:58:13, DaleCurtis wrote:
> On 2017/03/18 at 01:08:58, Kai Ninomiya wrote:
> > Hm, adding it to copyVideoTextureToPlatformTexture and texImageImpl wouldn't
> be hard (each only has one caller). But I may be missing something here: I
think
> it would have to also be added to paintCurrentFrame for the other paths, which
> has 6 callers and would be a little more intrusive than I want for a
prototype.
> > 
> > Perhaps this could be implemented only for those 2 upload paths, but that
> might not be reliable enough for YouTube's testing. I'm not sure what formats
> take those paths on what platforms.
> 
> Is there any other way to upload a frame? Also 6 callers isn't that much; plus
> if you really want to Blink allows default values, so you can just set the new
> parameters as nullptr for optional extraction.

Dale, there are three different entry points on HTMLVideoElement used to upload
the current frame to WebGL (as well as for other purposes) -- paintCurrentFrame,
copyVideoTextureToPlatformTexture, and texImageImpl.

Since this is known to be a prototype I think that we should minimize the impact
to the code base, and agree with Kai's decision in this area.

If we are going to plumb this query out to all of the call sites then I think a
struct containing this info should be defined somewhere (presumably
src/third_party/WebKit/public/platform/WebMediaPlayer.h) and we should let the
pointer to that be null, rather than passing three pointers to all of these
APIs.

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLTexture.h (right):

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLTexture.h:50: void
videoUpdateLastUploaded(WebMediaPlayer*);
I think these would read better by naming this:

updateLastUploadedVideo()
lastUploadedVideoWidth
lastUploadedVideoHeight
etc.

[DaleCurtis, 2017-03-20 21:59:54]

media/ parts lgtm

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2749653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5288: if
(video->copyVideoTextureToPlatformTexture(contextGL(), texture->object(),
On 2017/03/20 at 19:14:14, Ken Russell wrote:
> On 2017/03/20 17:58:13, DaleCurtis wrote:
> > On 2017/03/18 at 01:08:58, Kai Ninomiya wrote:
> > > Hm, adding it to copyVideoTextureToPlatformTexture and texImageImpl
wouldn't
> > be hard (each only has one caller). But I may be missing something here: I
think
> > it would have to also be added to paintCurrentFrame for the other paths,
which
> > has 6 callers and would be a little more intrusive than I want for a
prototype.
> > > 
> > > Perhaps this could be implemented only for those 2 upload paths, but that
> > might not be reliable enough for YouTube's testing. I'm not sure what
formats
> > take those paths on what platforms.
> > 
> > Is there any other way to upload a frame? Also 6 callers isn't that much;
plus
> > if you really want to Blink allows default values, so you can just set the
new
> > parameters as nullptr for optional extraction.
> 
> Dale, there are three different entry points on HTMLVideoElement used to
upload the current frame to WebGL (as well as for other purposes) --
paintCurrentFrame, copyVideoTextureToPlatformTexture, and texImageImpl.
> 
> Since this is known to be a prototype I think that we should minimize the
impact to the code base, and agree with Kai's decision in this area.
> 
> If we are going to plumb this query out to all of the call sites then I think
a struct containing this info should be defined somewhere (presumably
src/third_party/WebKit/public/platform/WebMediaPlayer.h) and we should let the
pointer to that be null, rather than passing three pointers to all of these
APIs.

Yes, this all sounds reasonable to me for the experiment. If we decide to launch
these properties out of experimental the API should change in the way you
suggest (details passed along in struct form at time of upload).

[Ken Russell (switch to Gerrit), 2017-03-20 22:33:37]

OK, great. Thanks Dale.

Kai: can you at least add one test verifying that these new fields are working?

Please take a look in src/third_party/WebKit/LayoutTests/VirtualTestSuites. That
file lets you define a subset of the tests to be run with a particular command
line argument (in this case --enable-experimental-canvas-features).

I don't remember off the top of my head the configurations that those tests run
under (like the "gpu" variant which is run with "--enable-accelerated-2d-canvas
--disable-display-list-2d-canvas" ). They may run just with Mesa.

If it would be more convenient to add for example a pixel test or some other
test which is known to run on the GPU bots, that's fine too.

Thanks.

[Kai Ninomiya, 2017-03-22 22:42:26]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 22:43:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kai Ninomiya, 2017-03-22 22:45:06]

The CQ bit was unchecked by kainino@chromium.org

[Kai Ninomiya, 2017-03-22 22:54:43]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 22:55:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 00:01:31]

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

[commit-bot: I haz the power, 2017-03-23 00:01:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Kai Ninomiya, 2017-03-23 00:16:35]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 00:17:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 00:56:17]

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

[commit-bot: I haz the power, 2017-03-23 00:56:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Kai Ninomiya, 2017-03-23 04:21:18]

On 2017/03/20 22:33:37, Ken Russell wrote:
> OK, great. Thanks Dale.
> 
> Kai: can you at least add one test verifying that these new fields are
working?
> 
> Please take a look in src/third_party/WebKit/LayoutTests/VirtualTestSuites.
That
> file lets you define a subset of the tests to be run with a particular command
> line argument (in this case --enable-experimental-canvas-features).
> 
> I don't remember off the top of my head the configurations that those tests
run
> under (like the "gpu" variant which is run with
"--enable-accelerated-2d-canvas
> --disable-display-list-2d-canvas" ). They may run just with Mesa.
> 
> If it would be more convenient to add for example a pixel test or some other
> test which is known to run on the GPU bots, that's fine too.
> 
> Thanks.

Mostly done.

kbr: Do you know why webexposed/global-interface-listing.html is able to see
these fields (hidden behind --enable-experimental-canvas-features)? I think I
remember running into this issue when working on getBufferSubDataAsync, but I
can't find what the solution was.

[Ken Russell (switch to Gerrit), 2017-03-23 04:39:10]

On 2017/03/23 04:21:18, Kai Ninomiya wrote:
> On 2017/03/20 22:33:37, Ken Russell wrote:
> > OK, great. Thanks Dale.
> > 
> > Kai: can you at least add one test verifying that these new fields are
> working?
> > 
> > Please take a look in src/third_party/WebKit/LayoutTests/VirtualTestSuites.
> That
> > file lets you define a subset of the tests to be run with a particular
command
> > line argument (in this case --enable-experimental-canvas-features).
> > 
> > I don't remember off the top of my head the configurations that those tests
> run
> > under (like the "gpu" variant which is run with
> "--enable-accelerated-2d-canvas
> > --disable-display-list-2d-canvas" ). They may run just with Mesa.
> > 
> > If it would be more convenient to add for example a pixel test or some other
> > test which is known to run on the GPU bots, that's fine too.
> > 
> > Thanks.
> 
> Mostly done.
> 
> kbr: Do you know why webexposed/global-interface-listing.html is able to see
> these fields (hidden behind --enable-experimental-canvas-features)? I think I
> remember running into this issue when working on getBufferSubDataAsync, but I
> can't find what the solution was.

It looks to me like most of the layout tests are actually run with the
experiment flags enabled. Only virtual/stable/webexposed/ doesn't. See
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webexpose...
and other interfaces like Touch and ImageData that have
RuntimeEnabled=ExperimentalCanvasFeatures attributes which show up in
global-interface-listing-expected.txt.

[Kai Ninomiya, 2017-03-23 23:32:37]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 23:33:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 23:37:48]

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

[commit-bot: I haz the power, 2017-03-23 23:37:49]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Kai Ninomiya, 2017-03-23 23:40:43]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 23:41:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Kai Ninomiya, 2017-03-24 00:34:30]

kbr, thanks. I think it's working now. PTAL

[commit-bot: I haz the power, 2017-03-24 01:06:24]

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

[commit-bot: I haz the power, 2017-03-24 01:06:25]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-03-24 21:47:55]

Excellent. Thanks for putting a test in place. LGTM

CC'ing junov@ as an FYI about the new experimental-features VirtualTestSuite.

https://codereview.chromium.org/2749653003/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas-experimental/webgl/texImage-video-last-uploaded-metadata.html
(right):

https://codereview.chromium.org/2749653003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas-experimental/webgl/texImage-video-last-uploaded-metadata.html:30:
passing &= texture.lastUploadedVideoTimestamp === 2.0;
It'd be awesome if this particular part of the test can be this precise! I have
some doubts about whether it'll ultimately be flaky though.

[Kai Ninomiya, 2017-03-24 22:01:43]

On 2017/03/24 21:47:55, Ken Russell wrote:
> Excellent. Thanks for putting a test in place. LGTM
> 
> CC'ing junov@ as an FYI about the new experimental-features VirtualTestSuite.
> 
>
https://codereview.chromium.org/2749653003/diff/200001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/fast/canvas-experimental/webgl/texImage-video-last-uploaded-metadata.html
> (right):
> 
>
https://codereview.chromium.org/2749653003/diff/200001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/fast/canvas-experimental/webgl/texImage-video-last-uploaded-metadata.html:30:
> passing &= texture.lastUploadedVideoTimestamp === 2.0;
> It'd be awesome if this particular part of the test can be this precise! I
have
> some doubts about whether it'll ultimately be flaky though.

Good point, I suppose it could. The video is 30fps and it seemed to only allow
seeking at a granularity of 3fps (probably the keyframes). So I'll give it some
threshold.

[Kai Ninomiya, 2017-03-24 22:05:01]

kainino@chromium.org changed reviewers:
+ esprehn@chromium.org

[Kai Ninomiya, 2017-03-24 22:05:02]

esprehn: Could you PTAL at WebKit/public/platform? We're doing a very early
prototype to expose some metadata for WebGL users. It requires plumbing through
public/platform.

[esprehn, 2017-03-24 22:47:13]

esprehn@chromium.org changed reviewers:
+ dglazkov@chromium.org
- esprehn@chromium.org

[esprehn, 2017-03-24 22:48:07]

On 2017/03/24 at 22:05:02, kainino wrote:
> esprehn: Could you PTAL at WebKit/public/platform? We're doing a very early
prototype to expose some metadata for WebGL users. It requires plumbing through
public/platform.

I'm going to defer this review to dglazkov@, personally I think you should onion
soup the media code so you don't need to keep changing the public API and
getting reviews from one of the public API owners.

[Kai Ninomiya, 2017-03-24 23:43:34]

On 2017/03/24 22:48:07, esprehn wrote:
> On 2017/03/24 at 22:05:02, kainino wrote:
> > esprehn: Could you PTAL at WebKit/public/platform? We're doing a very early
> prototype to expose some metadata for WebGL users. It requires plumbing
through
> public/platform.
> 
> I'm going to defer this review to dglazkov@, personally I think you should
onion
> soup the media code so you don't need to keep changing the public API and
> getting reviews from one of the public API owners.

Thanks, I'll await dglazkov's review.

esprehn or dglazkov: can you explain what it means to 'onion soup' this code? I
would love to avoid making too many public API changes, but I don't know how to
do that here.

[haraken, 2017-03-27 04:51:07]

On 2017/03/24 23:43:34, Kai Ninomiya wrote:
> On 2017/03/24 22:48:07, esprehn wrote:
> > On 2017/03/24 at 22:05:02, kainino wrote:
> > > esprehn: Could you PTAL at WebKit/public/platform? We're doing a very
early
> > prototype to expose some metadata for WebGL users. It requires plumbing
> through
> > public/platform.
> > 
> > I'm going to defer this review to dglazkov@, personally I think you should
> onion
> > soup the media code so you don't need to keep changing the public API and
> > getting reviews from one of the public API owners.
> 
> Thanks, I'll await dglazkov's review.
> 
> esprehn or dglazkov: can you explain what it means to 'onion soup' this code?
I
> would love to avoid making too many public API changes, but I don't know how
to
> do that here.

The basic idea is to move more code from the Chromium side to the Blink side and
thus avoid adding public APIs. In this case, you can consider moving
media/blink/webmediaplayer* to Blink. Then WebGL can directly access
WebMediaPlayer and thus no public API is needed. (I haven't checked how easy it
would be.)

[dglazkov, 2017-03-27 20:08:21]

I like the idea of migrating webmediaplayer_* to blink. I wouldn't block this
work on it though. Dale, Mounir -- do y'all have an idea of how much work this
would entail? Could we scope a chunky-enough project out of this?

LGTM % these comments:

https://codereview.chromium.org/2749653003/diff/220001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2749653003/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1848: if (video_frame) {
Early return might be easier to read?

if (!video_frame) return nullptr;

...

https://codereview.chromium.org/2749653003/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1866: if (video_frame) {
Same here.

https://codereview.chromium.org/2749653003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLTexture.idl (right):

https://codereview.chromium.org/2749653003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLTexture.idl:29:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute unsigned long
lastUploadedVideoWidth;
Was there an Intent to Implement for these? Seems like we need one?

[DaleCurtis, 2017-03-27 20:11:41]

It's not too much work just for media/blink if blink is allowed to pick up all
the DEPS listed here: https://cs.chromium.org/chromium/src/media/blink/DEPS

Unfortunately the only real gains here are no public API, we do not get to
reduce almost any layering (most things have multiple implementations) -- hence
this has not been prioritized; mlamouri@ is putting together a plan for this in
the coming Q though.

[dglazkov, 2017-03-27 20:19:47]

On 2017/03/27 at 20:11:41, dalecurtis wrote:
> It's not too much work just for media/blink if blink is allowed to pick up all
the DEPS listed here: https://cs.chromium.org/chromium/src/media/blink/DEPS
> 
> Unfortunately the only real gains here are no public API, we do not get to
reduce almost any layering (most things have multiple implementations) -- hence
this has not been prioritized; mlamouri@ is putting together a plan for this in
the coming Q though.

Thank you for the explanation! I am curious how y'all use gin. It's listed as a
dependency, but I couldn't easily see where exactly it's used.

[DaleCurtis, 2017-03-27 20:53:30]

On 2017/03/27 at 20:19:47, dglazkov wrote:
> On 2017/03/27 at 20:11:41, dalecurtis wrote:
> > It's not too much work just for media/blink if blink is allowed to pick up
all the DEPS listed here: https://cs.chromium.org/chromium/src/media/blink/DEPS
> > 
> > Unfortunately the only real gains here are no public API, we do not get to
reduce almost any layering (most things have multiple implementations) -- hence
this has not been prioritized; mlamouri@ is putting together a plan for this in
the coming Q though.
> 
> Thank you for the explanation! I am curious how y'all use gin. It's listed as
a dependency, but I couldn't easily see where exactly it's used.

That's required for unittests,
https://cs.chromium.org/chromium/src/media/blink/run_all_unittests.cc?l=84

[mlamouri (slow - plz ping), 2017-03-28 14:49:47]

lgtm with intent to implement sent and other comments applied

Sorry for the delay :)

[Kai Ninomiya, 2017-03-28 21:33:23]

Re: intent to implement, see below.

https://codereview.chromium.org/2749653003/diff/220001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2749653003/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1848: if (video_frame) {
On 2017/03/27 20:08:21, dglazkov wrote:
> Early return might be easier to read?
> 
> if (!video_frame) return nullptr;
> 
> ...

Done.

https://codereview.chromium.org/2749653003/diff/220001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:1866: if (video_frame) {
On 2017/03/27 20:08:21, dglazkov wrote:
> Same here.

Done.

https://codereview.chromium.org/2749653003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webgl/WebGLTexture.idl (right):

https://codereview.chromium.org/2749653003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webgl/WebGLTexture.idl:29:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute unsigned long
lastUploadedVideoWidth;
On 2017/03/27 20:08:21, dglazkov wrote:
> Was there an Intent to Implement for these? Seems like we need one?

No, there was not. This is an experimental API, has no standard, and is not
exposed by default (requires --enable-experimental-canvas-features), so I don't
think it makes sense. If an Intent to Implement is really needed currently, can
we change something to make it not needed?

[dglazkov, 2017-03-28 21:43:51]

On 2017/03/28 at 21:33:23, kainino wrote:
 
> No, there was not. This is an experimental API, has no standard, and is not
exposed by default (requires --enable-experimental-canvas-features), so I don't
think it makes sense. If an Intent to Implement is really needed currently, can
we change something to make it not needed?

Any web-exposed change (behind a flag or not), has to go through Blink process
for launching features, outlined in
https://www.chromium.org/blink/launching-features.

This enables us to track all work that has potential for becoming part of the
Web platform. In order for I2I to become unnecessary, this patch would need to
change to not include modifications to .idl files.

Could you help me understand why you're hoping to avoid sending an I2I?

[Ken Russell (switch to Gerrit), 2017-03-28 21:52:46]

On 2017/03/28 21:43:51, dglazkov wrote:
> On 2017/03/28 at 21:33:23, kainino wrote:
>  
> > No, there was not. This is an experimental API, has no standard, and is not
> exposed by default (requires --enable-experimental-canvas-features), so I
don't
> think it makes sense. If an Intent to Implement is really needed currently,
can
> we change something to make it not needed?
> 
> Any web-exposed change (behind a flag or not), has to go through Blink process
> for launching features, outlined in
> https://www.chromium.org/blink/launching-features.
> 
> This enables us to track all work that has potential for becoming part of the
> Web platform. In order for I2I to become unnecessary, this patch would need to
> change to not include modifications to .idl files.
> 
> Could you help me understand why you're hoping to avoid sending an I2I?

Dimitri, we never intend to ship this in its current form. It's an experiment to
understand what information the YouTube team would need for better 360 videos.
We are planning to eventually undo these changes and implement a more thorough,
though slightly more complex, API.

If the I2I is needed even in this case, then Kai, could you please draft it?

[Kai Ninomiya, 2017-03-28 21:53:19]

On 2017/03/28 21:43:51, dglazkov wrote:
> On 2017/03/28 at 21:33:23, kainino wrote:
>  
> > No, there was not. This is an experimental API, has no standard, and is not
> exposed by default (requires --enable-experimental-canvas-features), so I
don't
> think it makes sense. If an Intent to Implement is really needed currently,
can
> we change something to make it not needed?
> 
> Any web-exposed change (behind a flag or not), has to go through Blink process
> for launching features, outlined in
> https://www.chromium.org/blink/launching-features.
> 
> This enables us to track all work that has potential for becoming part of the
> Web platform. In order for I2I to become unnecessary, this patch would need to
> change to not include modifications to .idl files.
> 
> Could you help me understand why you're hoping to avoid sending an I2I?

Sorry, just my own misunderstanding of the I2I process. I thought that we would
try to fall under the following case for this experiment, since it is intended
to be thrown away.

"Your change does not affect web API behavior to the point that developers need
to be aware of it."

I'll look into sending an I2I.

[Kai Ninomiya, 2017-03-29 22:30:06]

I2I:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/YLR3v0t8WF0

[Kai Ninomiya, 2017-03-29 22:34:39]

The CQ bit was checked by kainino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 22:35:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 00:29:42]

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

[commit-bot: I haz the power, 2017-03-30 00:29:43]

Dry run: This issue passed the CQ dry run.

[Kai Ninomiya, 2017-03-30 00:44:25]

The CQ bit was checked by kainino@chromium.org

[Kai Ninomiya, 2017-03-30 00:44:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, kbr@chromium.org, mlamouri@chromium.org,
dglazkov@chromium.org
Link to the patchset: https://codereview.chromium.org/2749653003/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-30 00:45:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 00:55:37]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490834665369390,
"parent_rev": "44302d356149f11edfd1e1708e4c171ee073f436", "commit_rev":
"36eeff88e474d2e562f2d656bb94d0e37770e245"}

[commit-bot: I haz the power, 2017-03-30 00:56:46]

Description was changed from

==========
Prototype HTMLVideoElement properties for WebGL texImage2D

These properties provide the width, height, and time of the last video
frame uploaded to a WebGL texture via texImage2D.

BUG=639174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Prototype HTMLVideoElement properties for WebGL texImage2D

These properties provide the width, height, and time of the last video
frame uploaded to a WebGL texture via texImage2D.

BUG=639174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2749653003
Cr-Commit-Position: refs/heads/master@{#460606}
Committed:
https://chromium.googlesource.com/chromium/src/+/36eeff88e474d2e562f2d656bb94...
==========

[commit-bot: I haz the power, 2017-03-30 00:56:48]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/36eeff88e474d2e562f2d656bb94...