Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
212183005:
Android video: support copying video to WebGL for remote texture. (Closed)
|
Created:
6 years, 9 months ago by dshwang Modified:
6 years, 8 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
https://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAndroid video: support copying video to WebGL when video is playing remotely.
WebGL and Canvas also should draw a fixed VideoFrame showing a Cast logo, which replaces the local video
frames in embedded <video> elements whenever a video is being played remotely.
TEST=Run http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html on Nexus 5.
BUG=284478
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260382
Patch Set 1 #
Total comments: 11
Patch Set 2 : Change comments to up-to-date #Messages
Total messages: 25 (0 generated)
After https://codereview.chromium.org/198923007, it become easy to support copying video to WebGL for remote texture. I didn't test this CL yet. Could you give me a tip how to test remote texture? When is is_remote_ become true?
On Wed, Mar 26, 2014 at 2:26 AM, <dongseong.hwang@intel.com> wrote: > I didn't test this CL yet. Could you give me a tip how to test remote > texture? > When is is_remote_ become true? > I hadn't heard of it before you asked but skimming code makes it look like this codepath is engaged when using ChromeCast. Maybe try that and see if you trigger the new code? I'll defer to sievers@' for actual CR here. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/26 16:20:08, Ami Fischman wrote: > On Wed, Mar 26, 2014 at 2:26 AM, <mailto:dongseong.hwang@intel.com> wrote: > > > I didn't test this CL yet. Could you give me a tip how to test remote > > texture? > > When is is_remote_ become true? > > > > I hadn't heard of it before you asked but skimming code makes it look like > this codepath is engaged when using ChromeCast. Maybe try that and see if > you trigger the new code? > > I'll defer to sievers@' for actual CR here. Thank you for reply. I hope there is another way rather than chromecast... sievers@, could you give me a hint?
On 2014/03/26 18:09:41, dshwang wrote: > Thank you for reply. I hope there is another way rather than chromecast... > sievers@, could you give me a hint? I found this issue, https://code.google.com/p/chromium/issues/detail?id=284478 remote texture is used when chromecast draws video on remote device. remote texture is just generated texture, which give text information like "Casting to 990 home". At that time, WebGL and Canvas also should draw kind of "Casting to 990 home" frame. Am I right?
I checked remote texture code works well using following hack. I use http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html on Nexus 5. --- a/content/renderer/media/android/webmediaplayer_android.cc +++ b/content/renderer/media/android/webmediaplayer_android.cc @@ -868,14 +868,13 @@ void WebMediaPlayerAndroid::DrawRemotePlaybackText( const std::string& remote_playback_message) { DCHECK(main_thread_checker_.CalledOnValidThread()); - if (!video_weblayer_) - return; - +// if (!video_weblayer_) +// return; // TODO(johnme): Should redraw this frame if the layer bounds change; but // there seems no easy way to listen for the layer resizing (as opposed to // OnVideoSizeChanged, which is when the frame sizes of the video file // change). Perhaps have to poll (on main thread of course)? - gfx::Size video_size_css_px = video_weblayer_->bounds(); + gfx::Size video_size_css_px = natural_size_; float device_scale_factor = frame_->view()->deviceScaleFactor(); // canvas_size will be the size in device pixels when pageScaleFactor == 1 gfx::Size canvas_size( @@ -1005,6 +1004,8 @@ void WebMediaPlayerAndroid::ReallocateVideoFrame() { NOTIMPLEMENTED() << "Hole punching not supported without VIDEO_HOLE flag"; #endif // defined(VIDEO_HOLE) } else if (!is_remote_ && texture_id_) { + DrawRemotePlaybackText("dshwang!!!!!!!!!!!!!!!!!"); + return; scoped_refptr<VideoFrame> new_frame = VideoFrame::WrapNativeTexture( make_scoped_ptr(new gpu::MailboxHolder(texture_mailbox_, kGLTextureExternalOES,
https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:433: // Don't allow clients to copy the texture of encrypted video. Do we need to do this? There is no texture here to access when using encryption, it punches a whole instead and creates a 1x1 black current_frame_, which we can just copy here. https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:467: web_graphics_context->waitSyncPoint(mailbox_holder->sync_point); Doesn't this change the behavior in that we synchronize with the compositor using the texture now? Which I think doesn't really make sense for this read access here. Could we just remove the waitSyncPoint() here entirely like my comment suggests? We actually flush after creating both textures (remote_playback_texture_id_ and texture_id_). https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:485: web_graphics_context->pixelStorei(GL_UNPACK_FLIP_Y_CHROMIUM, flip_y); Aren't flip_y and premultiply_alpha incorrect here for the remote_playback placeholder?
Thank you for review. Could you reply my reply? https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:433: // Don't allow clients to copy the texture of encrypted video. On 2014/03/26 23:51:22, sievers wrote: > Do we need to do this? There is no texture here to access when using encryption, > it punches a whole instead and creates a 1x1 black current_frame_, which we can > just copy here. The result is the same. If return false, WebGLRenderingContextBase draws black texture as fallback, although the operation is a bit more expensive because WebGLRenderingContextBase make the same size texture to video frame. Could we need to create temporary 1X1 black texture here? https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:467: web_graphics_context->waitSyncPoint(mailbox_holder->sync_point); On 2014/03/26 23:51:22, sievers wrote: > Doesn't this change the behavior in that we synchronize with the compositor > using the texture now? Which I think doesn't really make sense for this read > access here. > > Could we just remove the waitSyncPoint() here entirely like my comment suggests? > We actually flush after creating both textures (remote_playback_texture_id_ and > texture_id_). We can not make sure web_graphics_context and stream_factory's context3d are in the same shared group. So waitSyncPoint() is needed. piman@ told "Flush being only sufficient if the 2 contexts are in the same share group" Please refer to https://code.google.com/p/chromium/issues/detail?id=351092#c11 https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:485: web_graphics_context->pixelStorei(GL_UNPACK_FLIP_Y_CHROMIUM, flip_y); On 2014/03/26 23:51:22, sievers wrote: > Aren't flip_y and premultiply_alpha incorrect here for the remote_playback > placeholder? both stream texture and remote playback texture have the same flip_y direction. I check it by my hack code.
https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:467: web_graphics_context->waitSyncPoint(mailbox_holder->sync_point); On 2014/03/27 19:42:57, dshwang wrote: > On 2014/03/26 23:51:22, sievers wrote: > > Doesn't this change the behavior in that we synchronize with the compositor > > using the texture now? Which I think doesn't really make sense for this read > > access here. > > > > Could we just remove the waitSyncPoint() here entirely like my comment > suggests? > > We actually flush after creating both textures (remote_playback_texture_id_ > and > > texture_id_). > > We can not make sure web_graphics_context and stream_factory's context3d are in > the same shared group. So waitSyncPoint() is needed. > piman@ told "Flush being only sufficient if the 2 contexts are in the same share > group" > Please refer to https://code.google.com/p/chromium/issues/detail?id=351092#c11 I think he's talking about possible future semantics. All contexts in the same renderer process are still on the same IPC channel and their flush IPC messages are therefore ordered. The thing I'm worried about: Isn't the mailbox_holder updated from another thread (compositor thread)? While we are on the main thread here.
https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:467: web_graphics_context->waitSyncPoint(mailbox_holder->sync_point); On 2014/03/28 18:10:25, sievers wrote: > I think he's talking about possible future semantics. All contexts in the same > renderer process are still on the same IPC channel and their flush IPC messages > are therefore ordered. > > The thing I'm worried about: Isn't the mailbox_holder updated from another > thread (compositor thread)? While we are on the main thread here. That's good question. mailbox_holder is updated by only render thread. When the compositor finishes to use mailbox_holder, the compositor updates mailbox_holder->sync_point on the render thread by PostTask.
https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:433: // Don't allow clients to copy the texture of encrypted video. After rethinking, I find 1x1 texture breaks webgl. Web developer regards this texture as 100x100 texture although actually it is 1x1 texture. after that, glCopyTexSubImage2D or other texture-related API cause gl error. https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:467: web_graphics_context->waitSyncPoint(mailbox_holder->sync_point); In addition, if https://codereview.chromium.org/175223003/ is landed, only video decoder code updates mailbox_holder.
lgtm https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:467: web_graphics_context->waitSyncPoint(mailbox_holder->sync_point); On 2014/03/28 19:19:57, dshwang wrote: > In addition, if https://codereview.chromium.org/175223003/ is landed, only video > decoder code updates mailbox_holder. Great, thanks for fixing that!
On 2014/03/28 19:47:59, sievers wrote: > lgtm > > https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... > File content/renderer/media/android/webmediaplayer_android.cc (right): > > https://codereview.chromium.org/212183005/diff/1/content/renderer/media/andro... > content/renderer/media/android/webmediaplayer_android.cc:467: > web_graphics_context->waitSyncPoint(mailbox_holder->sync_point); > On 2014/03/28 19:19:57, dshwang wrote: > > In addition, if https://codereview.chromium.org/175223003/ is landed, only > video > > decoder code updates mailbox_holder. > > Great, thanks for fixing that! Thank you for review! Ami, could you review this CL to land?
LGTM
On 2014/03/28 20:53:14, Ami Fischman wrote: > LGTM Thank you for review
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/212183005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/212183005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/212183005/60001
Message was sent while issue was closed.
Change committed as 260382 |
