|
|
Created:
6 years, 10 months ago by dshwang Modified:
6 years, 7 months ago Reviewers:
jamesr, Ami GONE FROM CHROMIUM, Cris Neckar, piman, danakj, jln (very slow on Chromium), no sievers 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. |
DescriptionHW Video: Make media::VideoFrame handle the sync point of the compositor as well as webgl and canvas.
This makes it compatible when the video hw texture and WebGL destination texture are
in different share groups.
media::VideoFrame must receive multiple sync points from clients, because clients can be many.
In WebGL case, only the compositor is the client of the mailbox in a frame. So we reuse
MailboxHolder::sync_point as release sync point. However, media::VideoFrame has multiple
clients, so media::VideoFrame must handle multiple release sync points.
Let me explain the lifecycle of the mailbox of a video frame in detail,
1. The video decoder receives a new mailbox from gpu process. The video decoder doesn't
insert a sync point, because all GPU operations for the mailbox already were executed
in the gpu process.
2. Blink or the compositor reads the mailbox. After that, all clients must insert
a release sync point.
3. When the ref count of the video frame is 0, the destructor of the video frame calls
recycle callback of the video decoder.
4. The video decoder notifies reusable mailboxes to the gpu process after waiting for
the release sync points.
Currently, there are three providers that can make a texture type video frame: GpuVideoDecoder,
RTCVideoDecoder, and VideoCapture. The video frame of VideoCapture is created in the browser
process, not the gpu process. So, VideoCapture inserts a sync point before providing it to
clients.
BUG=127940, 350925, 362521
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267290
Patch Set 1 #
Total comments: 1
Patch Set 2 : Cover all combinations: hw video, sw video, hw video to webgl, sw video to webgl. #Patch Set 3 : build fix cast_unittests #
Total comments: 7
Patch Set 4 : WIP: changing SetReleaseSyncPoint() to SwapReleaseSyncPoint() #Patch Set 5 : #Patch Set 6 : build fix unittests #Patch Set 7 : Add WaitSyncPoint in WMPImpl::paint(...) #
Total comments: 4
Patch Set 8 : Change VideoFrame to keep multiple release sync points. Deal with VideoCapture code also. #Patch Set 9 : previous patchset has unrelated code by mistake. #
Total comments: 10
Patch Set 10 : Make GpuVideoAcceleratorFactories::ReadPixels() receive mailbox, instead of texture #
Total comments: 3
Patch Set 11 : build fix for android #Patch Set 12 : remove unrelated changes #Patch Set 13 : Focus on this CL's goal and remove wrong change #
Total comments: 20
Patch Set 14 : make mailbox in VideoFrame const #
Total comments: 2
Patch Set 15 : Remove redundant vector::clean before Vector::assign #Patch Set 16 : Rebase to ToT #Patch Set 17 : rebase to ToT #Messages
Total messages: 86 (0 generated)
https://codereview.chromium.org/175223003/diff/1/content/renderer/media/webme... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/175223003/diff/1/content/renderer/media/webme... content/renderer/media/webmediaplayer_impl.cc:669: mailbox_holder->sync_point = web_graphics_context->insertSyncPoint(); I guess you did not insert the sync point for the purpose of performance, because webgl context and video gl context are in the same process. If so, I'll remove waitSyncPoint above and add a comment to be clear.
I think this LGTM under the assumption that this code and the VideoResourceUpdater are running on the same thread. I believe that is the case, that they are both on the main thread, so we can change this here without racing. And we waited on the sync point above, so we're not just overwriting and losing the existing one.
On 2014/02/21 16:39:35, danakj wrote: Thank you for review. I don't fully understand your word. > I think this LGTM under the assumption that this code and the > VideoResourceUpdater are running on the same thread. This code is not running on the same thread of VideoResourceUpdater. This code (WebMediaPlayerImpl::copyVideoTextureToPlatformTexture()) runs on Blink thread. VideoRendererImpl runs on video thread. VideoResourceUpdater runs on impl thread. Can I ask again whether this CL is lgtm or not? > I believe that is the case, > that they are both on the main thread, so we can change this here without > racing. And we waited on the sync point above, so we're not just overwriting and > losing the existing one. Can I ask again which is right? #1. add insertSyncPoint #2. remove waitSyncPoint
On 2014/02/21 17:44:49, dshwang wrote: > On 2014/02/21 16:39:35, danakj wrote: > Thank you for review. I don't fully understand your word. > > > I think this LGTM under the assumption that this code and the > > VideoResourceUpdater are running on the same thread. > > This code is not running on the same thread of VideoResourceUpdater. > > This code (WebMediaPlayerImpl::copyVideoTextureToPlatformTexture()) runs on > Blink thread. > VideoRendererImpl runs on video thread. > VideoResourceUpdater runs on impl thread. Oh right, it's on the impl side. Then you can race here. The impl side could insert a sync point between your calls to wait on the current sync point and insert your own, and you would overwrite it without waiting on it, possibly causing some the impl-side work to not complete before the texture is reused or destroyed. I think even accessing the sync point to wait on it can be a race though. It's good you came looking here. If this MailboxHolder's sync point is being used on multiple threads and being changed on at least one of those threads, we need to guard access to it with a lock. Our usual pattern for dealing with sync points is to instead just pass them across threads in post tasks, and pass a returning sync point back to the original thread later, so things are well ordered on that one thread where it can wait on each sync point returned. But here we're using a pointer through a lock instead, so I guess we need more locks. Maybe jamesr@ can double-check my thoughts here.
On 2014/02/21 18:25:37, danakj wrote: > On 2014/02/21 17:44:49, dshwang wrote: > > On 2014/02/21 16:39:35, danakj wrote: > > Thank you for review. I don't fully understand your word. > > > > > I think this LGTM under the assumption that this code and the > > > VideoResourceUpdater are running on the same thread. > > > > This code is not running on the same thread of VideoResourceUpdater. > > > > This code (WebMediaPlayerImpl::copyVideoTextureToPlatformTexture()) runs on > > Blink thread. > > VideoRendererImpl runs on video thread. > > VideoResourceUpdater runs on impl thread. > > Oh right, it's on the impl side. Then you can race here. The impl side could > insert a sync point between your calls to wait on the current sync point and > insert your own, and you would overwrite it without waiting on it, possibly > causing some the impl-side work to not complete before the texture is reused or > destroyed. > > I think even accessing the sync point to wait on it can be a race though. It's > good you came looking here. If this MailboxHolder's sync point is being used on > multiple threads and being changed on at least one of those threads, we need to > guard access to it with a lock. > > Our usual pattern for dealing with sync points is to instead just pass them > across threads in post tasks, and pass a returning sync point back to the > original thread later, so things are well ordered on that one thread where it > can wait on each sync point returned. But here we're using a pointer through a > lock instead, so I guess we need more locks. > > Maybe jamesr@ can double-check my thoughts here. I understand now why you ask on which thread VideoResourceUpdater runs. I think more lock is not needed because VideoFramePainter::GetCurrentFrame() has its own lock. scoped_refptr<VideoFrame> VideoFramePainter::GetCurrentFrame( bool frame_being_painted) { base::AutoLock auto_lock(lock_); if (frame_being_painted) current_frame_painted_ = true; return current_frame_; } In addition, now I think this CL is right, because - In my understanding, the sync point of the video frame was inserted by gpu process. Am I right? - renderer process must wait for the sync point the gpu process inserted, and insert sync point for gpu process reusing this resource. - WebMediaPlayerImpl::copyVideoTextureToPlatformTexture() must not rely on compositor's insert&wait sync point, because it's possible for the compositor to not use the webmediaplayer. For example, http://people.mozilla.org/~bjacob/mdn_samples_webgl_sample8/index.html
On Fri, Feb 21, 2014 at 1:55 PM, <dongseong.hwang@intel.com> wrote: > On 2014/02/21 18:25:37, danakj wrote: > >> On 2014/02/21 17:44:49, dshwang wrote: >> > On 2014/02/21 16:39:35, danakj wrote: >> > Thank you for review. I don't fully understand your word. >> > >> > > I think this LGTM under the assumption that this code and the >> > > VideoResourceUpdater are running on the same thread. >> > >> > This code is not running on the same thread of VideoResourceUpdater. >> > >> > This code (WebMediaPlayerImpl::copyVideoTextureToPlatformTexture()) >> runs on >> > Blink thread. >> > VideoRendererImpl runs on video thread. >> > VideoResourceUpdater runs on impl thread. >> > > Oh right, it's on the impl side. Then you can race here. The impl side >> could >> insert a sync point between your calls to wait on the current sync point >> and >> insert your own, and you would overwrite it without waiting on it, >> possibly >> causing some the impl-side work to not complete before the texture is >> reused >> > or > >> destroyed. >> > > I think even accessing the sync point to wait on it can be a race though. >> It's >> good you came looking here. If this MailboxHolder's sync point is being >> used >> > on > >> multiple threads and being changed on at least one of those threads, we >> need >> > to > >> guard access to it with a lock. >> > > Our usual pattern for dealing with sync points is to instead just pass >> them >> across threads in post tasks, and pass a returning sync point back to the >> original thread later, so things are well ordered on that one thread >> where it >> can wait on each sync point returned. But here we're using a pointer >> through a >> lock instead, so I guess we need more locks. >> > > Maybe jamesr@ can double-check my thoughts here. >> > > > I understand now why you ask on which thread VideoResourceUpdater runs. > I think more lock is not needed because VideoFramePainter::GetCurrentFrame() > has > its own lock. > > scoped_refptr<VideoFrame> VideoFramePainter::GetCurrentFrame( > bool frame_being_painted) { > base::AutoLock auto_lock(lock_); > if (frame_being_painted) > current_frame_painted_ = true; > > return current_frame_; > } > This lock guards access to the current_frame_ variable. But the MailboxHolder* is given to the VideoResourceUpdater here: https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/video... And to the WMPImpl here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... So these are 2 threads using the same physical memory. If the video resource updater updates the sync point here: https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/video... We need to ensure that someone calls waitSyncPoint on that before trying to use the texture. If then WMPImpl clobbers that sync point with its own, the sync point is lost. > > > In addition, now I think this CL is right, because > - In my understanding, the sync point of the video frame was inserted by > gpu > process. Am I right? > Sync points are generated on the client side and inserted into a CommandBuffer stream. Other CommandBuffer streams insert a waitSyncPoint to ensure the two CommandBuffers are synchronized. > - renderer process must wait for the sync point the gpu process inserted, > and > insert sync point for gpu process reusing this resource. > Inserting a sync point is not a sychronous call to the gpu process. > - WebMediaPlayerImpl::copyVideoTextureToPlatformTexture() must not rely on > compositor's insert&wait sync point, because it's possible for the > compositor to > not use the webmediaplayer. > WMPImpl doesn't need to wait for the sync point generated by the compositor in order to use the texture. But if it is going to insert a sync point of its own, it must do so after the compositor's sync point has been waited on if it exists. The race here is that both sides could insert a sync point, then you have racey synchronization between GL contexts. You need to make sure every sync point that was inserted was waited on before using the texture. And since there's only one sync point variable here, you need to wait on it before inserting a new one/replacing it. > For example, > http://people.mozilla.org/~bjacob/mdn_samples_webgl_sample8/index.html > > > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thank you for explanation. I need to investigate more deeply. let me see.... The most difficulty is because accelerated video is disabled in linux, and my cros build is in trouble for some reasons..
This is inserting a sync point into a main thread context. Who's going to wait for it? What resource is this sync point supposed to guard? The wait on line 637 seems fishy to me.
On 2014/02/21 22:15:43, jamesr wrote: > This is inserting a sync point into a main thread context. Who's going to wait > for it? What resource is this sync point supposed to guard? > > The wait on line 637 seems fishy to me. The wait on 637 is for the sync point inserted in the video decoder when it produced the mailbox and/or write the video frame texture contents. The wait here ensures the texture has the contents the frame expects. cc/ also waits on the same sync point. The insert here is meant to ensure the GPU operations run here are executed before the texture gets modified by the video decoder once it's returned there, to ensure we get the right contents copied into the output texture.
OK. But we'll race with the compositor thread if it grabs a reference to the same media::VideoFrame and starts mucking with the mailbox_holder. I guess the simplest fix would be to make sure that this code doesn't run concurrently with the compositor thread having access to the video frame. One way would be to have a mutex acquired by WebMediaPlayerImpl::GetCurrentFrame() (on the compositor thread) and released by WebMediaPlayerImpl::PutCurrentFrame() (on the same) and then have the body of copyTo...() guarded by this. This would prevent corruption of the mailbox_holder data, but wouldn't prevent both threads from wanting to reuse the sync point data. Maybe this needs a set of sync points for the factory to wait for before reusing the buffer in the case where the video frame is being used from multiple threads.
On Fri, Feb 21, 2014 at 5:27 PM, <jamesr@chromium.org> wrote: > OK. But we'll race with the compositor thread if it grabs a reference to > the > same media::VideoFrame and starts mucking with the mailbox_holder. I > guess the > simplest fix would be to make sure that this code doesn't run concurrently > with > the compositor thread having access to the video frame. One way would be > to > have a mutex acquired by WebMediaPlayerImpl::GetCurrentFrame() (on the > compositor thread) and released by WebMediaPlayerImpl::PutCurrentFrame() > (on the > same) and then have the body of copyTo...() guarded by this. This would > prevent > corruption of the mailbox_holder data, but wouldn't prevent both threads > from > wanting to reuse the sync point data. Maybe this needs a set of sync > points for > the factory to wait for before reusing the buffer in the case where the > video > frame is being used from multiple threads. > Reusing the sync point is okay, as long as you wait on the current sync point before replacing it. But I'm glad you see the same problem of concurrency that I do. It's problematic that the compositor can be changing this sync_point at the same time as this code is reading it. Your mutex suggestion sounds pretty reasonable. We could do something more fine-grained, but that would be perhaps the most obvious. > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
If the compositor and main thread maintained their own sync points, maybe we wouldn't need much or any locking. Grabbing a reference to the media::VideoFrame itself is already threadsafe, so we just have to make sure that the two threads do not clobber each other's data on a particular instance.
On Fri, Feb 21, 2014 at 5:36 PM, <jamesr@chromium.org> wrote: > If the compositor and main thread maintained their own sync points, maybe > we > wouldn't need much or any locking. Grabbing a reference to the > media::VideoFrame itself is already threadsafe, so we just have to make > sure > that the two threads do not clobber each other's data on a particular > instance. > yup i agree that'd work also. the decoder would write both sync points, and wait on both when it got them back. > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thank you for good discussion. Now it's more clear to me. :)
Hi, I update this CL that handle all concerns of jamesr@ and danakj@. I update the changelog also. Could you review again?
https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (left): https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:654: web_graphics_context->waitSyncPoint(mailbox_holder->sync_point); Why is this gone? This sync point ensures the mailbox is valid when we try to consume it here, and the contents of the texture are correct before we execute the copy. https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:539: skcanvas_video_renderer_.Paint(video_frame.get(), canvas, gfx_rect, alpha); Should someone be waiting on the video frame's sync point here? https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:695: video_frame->SetReleaseSyncPoint(web_graphics_context->insertSyncPoint()); While this solves the problem of two threads writing the same variable, I don't see how this solves the problem of clobbering the sync point. Both cc/ and here could write to this sync point variable, and in the end you'll only have 1 sync point get waited on. But you need both to be waited on. If you're going to overwrite it you have to wait on the existing one first (with a lock around both operations).
https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:539: skcanvas_video_renderer_.Paint(video_frame.get(), canvas, gfx_rect, alpha); On 2014/02/28 17:49:30, danakj wrote: > Should someone be waiting on the video frame's sync point here? we don't need to wait for the sync point. It's why I remove line 654 also. GpuVideoDecoder doesn't insert a sync point when it creates the video frame. scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture( make_scoped_ptr(new gpu::MailboxHolder( pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point */)), BindToCurrentLoop(base::Bind(&GpuVideoDecoder::ReusePictureBuffer, weak_this_, picture.picture_buffer_id())), pb.size(), visible_rect, natural_size, timestamp, base::Bind(&ReadPixelsSync, factories_, pb.texture_id(), visible_rect))); As I mentioned in CL description, at the time to create the video frame, gpu process doesn't have any pending GPU operations for the video frame, because the video frame is originated from gpu process. It's why GpuVideoDecoder doesn't insert a sync point. All clients can trust the video frame exists in gpu process. https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:695: video_frame->SetReleaseSyncPoint(web_graphics_context->insertSyncPoint()); On 2014/02/28 17:49:30, danakj wrote: > While this solves the problem of two threads writing the same variable, I don't > see how this solves the problem of clobbering the sync point. Both cc/ and here > could write to this sync point variable, and in the end you'll only have 1 sync > point get waited on. But you need both to be waited on. If you're going to > overwrite it you have to wait on the existing one first (with a lock around both > operations). you're right. GpuVideoDecoder will wait for only one sync point. it's issue. let me think..
On 2014/02/28 18:01:48, dshwang wrote: > https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... > content/renderer/media/webmediaplayer_impl.cc:695: > video_frame->SetReleaseSyncPoint(web_graphics_context->insertSyncPoint()); > On 2014/02/28 17:49:30, danakj wrote: > > While this solves the problem of two threads writing the same variable, I > don't > > see how this solves the problem of clobbering the sync point. Both cc/ and > here > > could write to this sync point variable, and in the end you'll only have 1 > sync > > point get waited on. But you need both to be waited on. If you're going to > > overwrite it you have to wait on the existing one first (with a lock around > both > > operations). > > you're right. GpuVideoDecoder will wait for only one sync point. it's issue. let > me think.. I'm about changing SetReleaseSyncPoint() to SwapReleaseSyncPoint(). If return value is not zero, the client calling SwapReleaseSyncPoint() will wait for the sync point. It's not very elegant but can be solution. wdyt?
https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:539: skcanvas_video_renderer_.Paint(video_frame.get(), canvas, gfx_rect, alpha); On 2014/02/28 18:01:48, dshwang wrote: > On 2014/02/28 17:49:30, danakj wrote: > > Should someone be waiting on the video frame's sync point here? > > we don't need to wait for the sync point. It's why I remove line 654 also. > > GpuVideoDecoder doesn't insert a sync point when it creates the video frame. > scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture( > make_scoped_ptr(new gpu::MailboxHolder( > pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point */)), > BindToCurrentLoop(base::Bind(&GpuVideoDecoder::ReusePictureBuffer, > weak_this_, > picture.picture_buffer_id())), > pb.size(), > visible_rect, > natural_size, > timestamp, > base::Bind(&ReadPixelsSync, factories_, pb.texture_id(), visible_rect))); > > As I mentioned in CL description, at the time to create the video frame, gpu > process doesn't have any pending GPU operations for the video frame, because the > video frame is originated from gpu process. > It's why GpuVideoDecoder doesn't insert a sync point. All clients can trust the > video frame exists in gpu process. All GL is actually run on the gpu process so I'm not sure what you mean here. The sync points are to synchronize between different command buffers using the gpu process. Also there is this sync point that needs to be waited on to ensure the mailbox is valid/created: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:695: video_frame->SetReleaseSyncPoint(web_graphics_context->insertSyncPoint()); On 2014/02/28 18:01:48, dshwang wrote: > On 2014/02/28 17:49:30, danakj wrote: > > While this solves the problem of two threads writing the same variable, I > don't > > see how this solves the problem of clobbering the sync point. Both cc/ and > here > > could write to this sync point variable, and in the end you'll only have 1 > sync > > point get waited on. But you need both to be waited on. If you're going to > > overwrite it you have to wait on the existing one first (with a lock around > both > > operations). > > you're right. GpuVideoDecoder will wait for only one sync point. it's issue. let > me think.. That will work if and only if the person doing the swapping does an insert after doing the wait. So you can't give a sync point at the time of calling swap.
On 2014/02/28 18:41:18, danakj wrote: > https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... > File content/renderer/media/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... > content/renderer/media/webmediaplayer_impl.cc:539: > skcanvas_video_renderer_.Paint(video_frame.get(), canvas, gfx_rect, alpha); > On 2014/02/28 18:01:48, dshwang wrote: > > On 2014/02/28 17:49:30, danakj wrote: > > > Should someone be waiting on the video frame's sync point here? > > > > we don't need to wait for the sync point. It's why I remove line 654 also. > > > > GpuVideoDecoder doesn't insert a sync point when it creates the video frame. > > scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture( > > make_scoped_ptr(new gpu::MailboxHolder( > > pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point */)), > > BindToCurrentLoop(base::Bind(&GpuVideoDecoder::ReusePictureBuffer, > > weak_this_, > > picture.picture_buffer_id())), > > pb.size(), > > visible_rect, > > natural_size, > > timestamp, > > base::Bind(&ReadPixelsSync, factories_, pb.texture_id(), > visible_rect))); > > > > As I mentioned in CL description, at the time to create the video frame, gpu > > process doesn't have any pending GPU operations for the video frame, because > the > > video frame is originated from gpu process. > > It's why GpuVideoDecoder doesn't insert a sync point. All clients can trust > the > > video frame exists in gpu process. > > All GL is actually run on the gpu process so I'm not sure what you mean here. > The sync points are to synchronize between different command buffers using the > gpu process. > > Also there is this sync point that needs to be waited on to ensure the mailbox > is valid/created: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... yes, all gpu operations run in gpu process. The difference is where is actor. GpuVideoDecodeAccelerator lives in gpu process while WebGLRenderingContext lives in render process. After WebGL makes a mailbox, the compositor must wait for the sync point of WebGL, because the compositor must make sure the mailbox exists in gpu process. However, after GpuVideoDecodeAccelerator makes a mailbox, the compositor don't need to wait for the sync point for GpuVideoDecodeAccelerator, because the mailbox exists in gpu process, because GpuVideoDecodeAccelerator lives in gpu process. When GpuVideoDecodeAcceleratorHost receives the mailbox, the event like WaitSyncPoint already occurs. It's why WebMediaPlayerImpl::paint(..) and VideoLayerImpl::WillDraw(...) don't wait for the sync point from origin of the mailbox. Currently, only WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(..) falsely waits for the sync point. > > https://codereview.chromium.org/175223003/diff/250001/content/renderer/media/... > content/renderer/media/webmediaplayer_impl.cc:695: > video_frame->SetReleaseSyncPoint(web_graphics_context->insertSyncPoint()); > On 2014/02/28 18:01:48, dshwang wrote: > > On 2014/02/28 17:49:30, danakj wrote: > > > While this solves the problem of two threads writing the same variable, I > > don't > > > see how this solves the problem of clobbering the sync point. Both cc/ and > > here > > > could write to this sync point variable, and in the end you'll only have 1 > > sync > > > point get waited on. But you need both to be waited on. If you're going to > > > overwrite it you have to wait on the existing one first (with a lock around > > both > > > operations). > > > > you're right. GpuVideoDecoder will wait for only one sync point. it's issue. > let > > me think.. > > That will work if and only if the person doing the swapping does an insert after > doing the wait. So you can't give a sync point at the time of calling swap. I do similar to your suggestion. could you review PatchSet4?
On Fri, Feb 28, 2014 at 1:54 PM, <dongseong.hwang@intel.com> wrote: > On 2014/02/28 18:41:18, danakj wrote: > > https://codereview.chromium.org/175223003/diff/250001/ > content/renderer/media/webmediaplayer_impl.cc > >> File content/renderer/media/webmediaplayer_impl.cc (right): >> > > > https://codereview.chromium.org/175223003/diff/250001/ > content/renderer/media/webmediaplayer_impl.cc#newcode539 > >> content/renderer/media/webmediaplayer_impl.cc:539: >> skcanvas_video_renderer_.Paint(video_frame.get(), canvas, gfx_rect, >> alpha); >> On 2014/02/28 18:01:48, dshwang wrote: >> > On 2014/02/28 17:49:30, danakj wrote: >> > > Should someone be waiting on the video frame's sync point here? >> > >> > we don't need to wait for the sync point. It's why I remove line 654 >> also. >> > >> > GpuVideoDecoder doesn't insert a sync point when it creates the video >> frame. >> > scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture( >> > make_scoped_ptr(new gpu::MailboxHolder( >> > pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point >> > */)), > >> > BindToCurrentLoop(base::Bind(&GpuVideoDecoder:: >> ReusePictureBuffer, >> > weak_this_, >> > picture.picture_buffer_id())), >> > pb.size(), >> > visible_rect, >> > natural_size, >> > timestamp, >> > base::Bind(&ReadPixelsSync, factories_, pb.texture_id(), >> visible_rect))); >> > >> > As I mentioned in CL description, at the time to create the video >> frame, gpu >> > process doesn't have any pending GPU operations for the video frame, >> because >> the >> > video frame is originated from gpu process. >> > It's why GpuVideoDecoder doesn't insert a sync point. All clients can >> trust >> the >> > video frame exists in gpu process. >> > > All GL is actually run on the gpu process so I'm not sure what you mean >> here. >> The sync points are to synchronize between different command buffers >> using the >> gpu process. >> > > Also there is this sync point that needs to be waited on to ensure the >> mailbox >> is valid/created: >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/content/renderer/media/renderer_gpu_video_ > accelerator_factories.cc&rcl=1393574299&l=113 > > yes, all gpu operations run in gpu process. The difference is where is > actor. > GpuVideoDecodeAccelerator lives in gpu process while WebGLRenderingContext > lives > in render process. > > After WebGL makes a mailbox, the compositor must wait for the sync point of > WebGL, because the compositor must make sure the mailbox exists in gpu > process. > However, after GpuVideoDecodeAccelerator makes a mailbox, the compositor > don't > need to wait for the sync point for GpuVideoDecodeAccelerator, because the > mailbox exists in gpu process, because GpuVideoDecodeAccelerator lives in > gpu > process. > I don't think I understand the behaviour inside the gpu process enough to agree or disagree with this. It doesn't match my understanding of the requirements that I've taken from piman@ so far though. piman or sievers can clarify this I hope. > When GpuVideoDecodeAcceleratorHost receives the mailbox, the event like > WaitSyncPoint already occurs. > > It's why WebMediaPlayerImpl::paint(..) and VideoLayerImpl::WillDraw(...) > don't > wait for the sync point from origin of the mailbox. > This isn't quite true. The WillDraw method doesn't wait for it directly. But the ResourceProvider will wait on the sync point given here https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/textu... before using it here https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou... > Currently, only WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(..) > falsely waits for the sync point. > > > > > https://codereview.chromium.org/175223003/diff/250001/ > content/renderer/media/webmediaplayer_impl.cc#newcode695 > >> content/renderer/media/webmediaplayer_impl.cc:695: >> video_frame->SetReleaseSyncPoint(web_graphics_context-> >> insertSyncPoint()); >> On 2014/02/28 18:01:48, dshwang wrote: >> > On 2014/02/28 17:49:30, danakj wrote: >> > > While this solves the problem of two threads writing the same >> variable, I >> > don't >> > > see how this solves the problem of clobbering the sync point. Both >> cc/ and >> > here >> > > could write to this sync point variable, and in the end you'll only >> have 1 >> > sync >> > > point get waited on. But you need both to be waited on. If you're >> going to >> > > overwrite it you have to wait on the existing one first (with a lock >> > around > >> > both >> > > operations). >> > >> > you're right. GpuVideoDecoder will wait for only one sync point. it's >> issue. >> let >> > me think.. >> > > That will work if and only if the person doing the swapping does an insert >> > after > >> doing the wait. So you can't give a sync point at the time of calling >> swap. >> > > I do similar to your suggestion. could you review PatchSet4? > > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/28 19:09:52, danakj wrote: > > https://code.google.com/p/chromium/codesearch#chromium/ > > src/content/renderer/media/renderer_gpu_video_ > > accelerator_factories.cc&rcl=1393574299&l=113 > > > > yes, all gpu operations run in gpu process. The difference is where is > > actor. > > GpuVideoDecodeAccelerator lives in gpu process while WebGLRenderingContext > > lives > > in render process. > > > > After WebGL makes a mailbox, the compositor must wait for the sync point of > > WebGL, because the compositor must make sure the mailbox exists in gpu > > process. > > However, after GpuVideoDecodeAccelerator makes a mailbox, the compositor > > don't > > need to wait for the sync point for GpuVideoDecodeAccelerator, because the > > mailbox exists in gpu process, because GpuVideoDecodeAccelerator lives in > > gpu > > process. > > > > I don't think I understand the behaviour inside the gpu process enough to > agree or disagree with this. It doesn't match my understanding of the > requirements that I've taken from piman@ so far though. piman or sievers > can clarify this I hope. Thank you for your review. piman@, sievers@, could you have a chance to look at this CL. I hope you verify my theory: - webgl or the compositor doesn't need to wait for sync point of video frame. - It's because -- GpuVideoDecodeAccelerator in gpu process draws video frame. -- GpuVideoDecodeAccelerator acks to GpuVideoDecodeAcceleratorHost. It is the same effect of WaitSyncPoint. -- WebGL can use the video frame without calling waitSyncPoint because the texture of the video frame is inside gpu memory already. > > When GpuVideoDecodeAcceleratorHost receives the mailbox, the event like > > WaitSyncPoint already occurs. > > > > It's why WebMediaPlayerImpl::paint(..) and VideoLayerImpl::WillDraw(...) > > don't > > wait for the sync point from origin of the mailbox. > > > > This isn't quite true. The WillDraw method doesn't wait for it directly. > But the ResourceProvider will wait on the sync point given here > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/textu... > before using it here > https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou... > However, resource_provider doesn't wait anything, because the sync point is "0". GpuVideoDecoder doesn't insert a sync point when it creates the video frame. scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture( make_scoped_ptr(new gpu::MailboxHolder( pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point */)), <----------- Don't insert. ....); If Ubercompositor is enabled, the parent waits for the sync point that the child inserted, not GpuVideoDecoder inserted.
On 2014/02/28 19:21:16, dshwang wrote: > On 2014/02/28 19:09:52, danakj wrote: > > > https://code.google.com/p/chromium/codesearch#chromium/ > > > src/content/renderer/media/renderer_gpu_video_ > > > accelerator_factories.cc&rcl=1393574299&l=113 > > > > > > yes, all gpu operations run in gpu process. The difference is where is > > > actor. > > > GpuVideoDecodeAccelerator lives in gpu process while WebGLRenderingContext > > > lives > > > in render process. > > > > > > After WebGL makes a mailbox, the compositor must wait for the sync point of > > > WebGL, because the compositor must make sure the mailbox exists in gpu > > > process. > > > However, after GpuVideoDecodeAccelerator makes a mailbox, the compositor > > > don't > > > need to wait for the sync point for GpuVideoDecodeAccelerator, because the > > > mailbox exists in gpu process, because GpuVideoDecodeAccelerator lives in > > > gpu > > > process. > > > > > > > I don't think I understand the behaviour inside the gpu process enough to > > agree or disagree with this. It doesn't match my understanding of the > > requirements that I've taken from piman@ so far though. piman or sievers > > can clarify this I hope. > > Thank you for your review. > > piman@, sievers@, could you have a chance to look at this CL. > > I hope you verify my theory: > - webgl or the compositor doesn't need to wait for sync point of video frame. > - It's because > -- GpuVideoDecodeAccelerator in gpu process draws video frame. > -- GpuVideoDecodeAccelerator acks to GpuVideoDecodeAcceleratorHost. It is the > same effect of WaitSyncPoint. > -- WebGL can use the video frame without calling waitSyncPoint because the > texture of the video frame is inside gpu memory already. > > I think you are saying that after the texture and mailbox is created, the texture ownership passes to the GPU process through an IPC, because the decoder impl lives in the GPU process. When the buffer is ready for display the GPU process sends AcceleratedVideoDecoderHostMsg_PictureReady to pass ownership back to the renderer. That would mean the client does not have to wait for the sync point *if* the renderer sends this IPC to GPU process after InsertSyncPoint and getting the reply (PictureReady) would at least imply one hop through the GPU main thread here (just doing stuff on the GPU IO thread is not good enough). Because then we know that a task posted to the GPU main thread would imply that the Flush from InsertSyncPoint has already run on that same thread and all previously inserted commands have been parsed. Your description does seem to match the comment here (and similarly in rtc_video_decoder.cc): GpuVideoDecoder::ProvidePictureBuffers() { // Discards the sync point returned here since PictureReady will imply that // the produce has already happened, and the texture is ready for use. if (!factories_->CreateTextures(count, However, it seems a bit odd to me to hardcode the assumptions about one component into another component. What if we added another source of video frames to be used with WebMediaPlayerImpl? glWaitSyncPoint() is not expensive if the sync point is already passed. It just inserts a token into the cmdbuffer and in the decoder will immediately see that the sync point has passed and not deschedule. So IMHO it would be nicer to keep the code functional in a more generic way.
Thank you for your opinion. On 2014/02/28 20:30:03, sievers wrote: > I think you are saying that after the texture and mailbox is created, the > texture ownership passes to the GPU process through an IPC, because the decoder > impl lives in the GPU process. > When the buffer is ready for display the GPU process sends > AcceleratedVideoDecoderHostMsg_PictureReady to pass ownership back to the > renderer. Yes. > That would mean the client does not have to wait for the sync point *if* the > renderer sends this IPC to GPU process after InsertSyncPoint your explanation is good, but we want to separate AcceleratedVideoDecoderMsg_AssignPictureBuffers and AcceleratedVideoDecoderMsg_ReusePictureBuffer When renderer make new mailbox, it send AcceleratedVideoDecoderMsg_AssignPictureBuffers to gpu process after InsertSyncPoint. Because gpu process must run all GPU operation before InsertSyncPoint to make sure getting relevant texture. When renderer reuse the mailbox, it send AcceleratedVideoDecoderMsg_ReusePictureBuffer to gpu process. At that time, InsertSyncPoint is not needed, while renderer need to WaitSyncPoint for the compositor or WebGL. > and getting the > reply (PictureReady) would at least imply one hop through the GPU main thread > here (just doing stuff on the GPU IO thread is not good enough). > > Because then we know that a task posted to the GPU main thread would imply that > the Flush from InsertSyncPoint has already run on that same thread and all > previously inserted commands have been parsed. > > Your description does seem to match the comment here (and similarly in > rtc_video_decoder.cc): > > GpuVideoDecoder::ProvidePictureBuffers() { > // Discards the sync point returned here since PictureReady will imply that > // the produce has already happened, and the texture is ready for use. > if (!factories_->CreateTextures(count, That's right. > However, it seems a bit odd to me to hardcode the assumptions about one > component into another component. > What if we added another source of video frames to be used with > WebMediaPlayerImpl? > > glWaitSyncPoint() is not expensive if the sync point is already passed. It just > inserts a token into the cmdbuffer and in the decoder will immediately see that > the sync point has passed and not deschedule. So IMHO it would be nicer to keep > the code functional in a more generic way. I don't mind adding glWaitSyncPoint(). All I want is consistency. We should add or remove glWaitSyncPoint() on below three sites. - WebMediaPlayerImpl::paint(..) - WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(...) - VideoLayerImpl::WillDraw(...) However, if we remove glWaitSyncPoint(), someone perhaps can be confused, because GpuVideoDecoder::PictureReady() didn't insert sync point. void GpuVideoDecoder::PictureReady(const media::Picture& picture) { scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture( make_scoped_ptr(new gpu::MailboxHolder( pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point */)), ....); And could I ask to review this CL also?
On Fri, Feb 28, 2014 at 3:53 PM, <dongseong.hwang@intel.com> wrote: > Thank you for your opinion. > > > On 2014/02/28 20:30:03, sievers wrote: > >> I think you are saying that after the texture and mailbox is created, the >> texture ownership passes to the GPU process through an IPC, because the >> > decoder > >> impl lives in the GPU process. >> When the buffer is ready for display the GPU process sends >> AcceleratedVideoDecoderHostMsg_PictureReady to pass ownership back to the >> renderer. >> > > Yes. > > > That would mean the client does not have to wait for the sync point *if* >> the >> renderer sends this IPC to GPU process after InsertSyncPoint >> > > your explanation is good, but we want to separate > AcceleratedVideoDecoderMsg_AssignPictureBuffers and > AcceleratedVideoDecoderMsg_ReusePictureBuffer > When renderer make new mailbox, it send > AcceleratedVideoDecoderMsg_AssignPictureBuffers to gpu process after > InsertSyncPoint. > Because gpu process must run all GPU operation before InsertSyncPoint to > make > sure getting relevant texture. > > When renderer reuse the mailbox, it send > AcceleratedVideoDecoderMsg_ReusePictureBuffer to gpu process. > At that time, InsertSyncPoint is not needed, while renderer need to > WaitSyncPoint for the compositor or WebGL. > > > and getting the >> reply (PictureReady) would at least imply one hop through the GPU main >> thread >> here (just doing stuff on the GPU IO thread is not good enough). >> > > Because then we know that a task posted to the GPU main thread would imply >> > that > >> the Flush from InsertSyncPoint has already run on that same thread and all >> previously inserted commands have been parsed. >> > > Your description does seem to match the comment here (and similarly in >> rtc_video_decoder.cc): >> > > GpuVideoDecoder::ProvidePictureBuffers() { >> // Discards the sync point returned here since PictureReady will imply >> that >> // the produce has already happened, and the texture is ready for use. >> if (!factories_->CreateTextures(count, >> > > That's right. > > > However, it seems a bit odd to me to hardcode the assumptions about one >> component into another component. >> What if we added another source of video frames to be used with >> WebMediaPlayerImpl? >> > > glWaitSyncPoint() is not expensive if the sync point is already passed. It >> > just > >> inserts a token into the cmdbuffer and in the decoder will immediately see >> > that > >> the sync point has passed and not deschedule. So IMHO it would be nicer to >> > keep > >> the code functional in a more generic way. >> > > I don't mind adding glWaitSyncPoint(). All I want is consistency. > We should add or remove glWaitSyncPoint() on below three sites. > - WebMediaPlayerImpl::paint(..) > - WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(...) > - VideoLayerImpl::WillDraw(...) > > > However, if we remove glWaitSyncPoint(), someone perhaps can be confused, > because GpuVideoDecoder::PictureReady() didn't insert sync point. > void GpuVideoDecoder::PictureReady(const media::Picture& picture) { > > scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture( > make_scoped_ptr(new gpu::MailboxHolder( > pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point > */)), > ....); > As long as cc/ is waiting on the sync point if it's present, not just ignoring it, then if this code actually knows that the sync point isn't needed, using a 0 seems fine. WMPImpl would try wait on it like cc does also. It seems that paint() just should have been waiting but wasn't? > > > And could I ask to review this CL also? > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/28 21:16:38, danakj wrote: > > I don't mind adding glWaitSyncPoint(). All I want is consistency. > > We should add or remove glWaitSyncPoint() on below three sites. > > - WebMediaPlayerImpl::paint(..) > > - WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(...) > > - VideoLayerImpl::WillDraw(...) > > > > > > However, if we remove glWaitSyncPoint(), someone perhaps can be confused, > > because GpuVideoDecoder::PictureReady() didn't insert sync point. > > void GpuVideoDecoder::PictureReady(const media::Picture& picture) { > > > > scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture( > > make_scoped_ptr(new gpu::MailboxHolder( > > pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point > > */)), > > ....); > > > > As long as cc/ is waiting on the sync point if it's present, not just > ignoring it, then if this code actually knows that the sync point isn't > needed, using a 0 seems fine. WMPImpl would try wait on it like cc does > also. It seems that paint() just should have been waiting but wasn't? > you're right. the compoisitor piggy back ResourceProvider::LockForRead() to wait for the sync point, instead of explicit wait in VideoLayerImpl::WillDraw(...). Currently, WMPImpl::paint(..) don't wait. I guess consensus is adding waitSyncPoint into all sites. So I'll add waitSyncPoint to WMPImpl::paint(..)
On Fri, Feb 28, 2014 at 4:24 PM, <dongseong.hwang@intel.com> wrote: > On 2014/02/28 21:16:38, danakj wrote: > >> > I don't mind adding glWaitSyncPoint(). All I want is consistency. >> > We should add or remove glWaitSyncPoint() on below three sites. >> > - WebMediaPlayerImpl::paint(..) >> > - WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(...) >> > - VideoLayerImpl::WillDraw(...) >> > >> > >> > However, if we remove glWaitSyncPoint(), someone perhaps can be >> confused, >> > because GpuVideoDecoder::PictureReady() didn't insert sync point. >> > void GpuVideoDecoder::PictureReady(const media::Picture& picture) { >> > >> > scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture( >> > make_scoped_ptr(new gpu::MailboxHolder( >> > pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point >> > */)), >> > ....); >> > >> > > As long as cc/ is waiting on the sync point if it's present, not just >> ignoring it, then if this code actually knows that the sync point isn't >> needed, using a 0 seems fine. WMPImpl would try wait on it like cc does >> also. It seems that paint() just should have been waiting but wasn't? >> > > > you're right. > the compoisitor piggy back ResourceProvider::LockForRead() to wait for > the sync > point, instead of explicit wait in VideoLayerImpl::WillDraw(...). > > Currently, WMPImpl::paint(..) don't wait. > I guess consensus is adding waitSyncPoint into all sites. So I'll add > waitSyncPoint to WMPImpl::paint(..) > If you can push the wait down into the code that actually grabs/uses the mailboxholder, that'd be even better. > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/28 21:26:16, danakj wrote: > > Currently, WMPImpl::paint(..) don't wait. > > I guess consensus is adding waitSyncPoint into all sites. So I'll add > > waitSyncPoint to WMPImpl::paint(..) > > > > If you can push the wait down into the code that actually grabs/uses the > mailboxholder, that'd be even better. > Done.
https://codereview.chromium.org/175223003/diff/330001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/175223003/diff/330001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:321: // this mailbox. If below code causes critical issue, ResourceProvider This is kinda ugly.. and since cc is giving the resource back it doesn't have a good reason to wait on the sync point other than making sure its sync point gets heard. I do like the idea of holding multiple sync points on the video frame more than inserting buggy weakptr code here. https://codereview.chromium.org/175223003/diff/330001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:325: GLC(gl, gl->WaitSyncPointCHROMIUM(previous_sync_point)); This is inverted, you'd need to wait on the existing sync point before inserting a new one. stuff1 insert1 stuff2 insert2 <- waiting on this should guarantee stuff1 and stuff2 complete <- someone else reads the sync point and does wait2, it expects stuff1 and stuff2 to be done but it's not -> wait1 <- now stuff1 is done.
https://codereview.chromium.org/175223003/diff/330001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/175223003/diff/330001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:321: // this mailbox. If below code causes critical issue, ResourceProvider On 2014/02/28 21:47:03, danakj wrote: > This is kinda ugly.. and since cc is giving the resource back it doesn't have a > good reason to wait on the sync point other than making sure its sync point gets > heard. I do like the idea of holding multiple sync points on the video frame > more than inserting buggy weakptr code here. Yeah, I agree. your idea is better. I'll change VideoFrame can keep multiple sync points. It makes client code clean also. https://codereview.chromium.org/175223003/diff/330001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:325: GLC(gl, gl->WaitSyncPointCHROMIUM(previous_sync_point)); On 2014/02/28 21:47:03, danakj wrote: > This is inverted, you'd need to wait on the existing sync point before inserting > a new one. > > stuff1 > insert1 > > stuff2 > insert2 <- waiting on this should guarantee stuff1 and stuff2 complete > > <- someone else reads the sync point and does wait2, it expects stuff1 and > stuff2 to be done but it's not -> > > wait1 <- now stuff1 is done. Thank you for explanation. After VideoFrame keep multiple release sync points, this code will be removed. I'll take care of release sync point ordering as your explanation :) Time to sleep in Europe. have a nice weekend.
Hi, I change VideoFrame to keep multiple release sync points. VideoFrame deals with VideoCapture code also. The CL is growing up. Could you review again?
On 2014/03/03 13:14:34, dshwang wrote: > Could you review again? I also improve the CL description.
On 2014/03/03 13:15:24, dshwang wrote: > On 2014/03/03 13:14:34, dshwang wrote: > > Could you review again?
On 2014/03/04 17:22:51, dshwang wrote: > On 2014/03/03 13:15:24, dshwang wrote: > > On 2014/03/03 13:14:34, dshwang wrote: > > > Could you review again?
This looks better overall than crazy locking, thanks. https://codereview.chromium.org/175223003/diff/350001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/175223003/diff/350001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:346: // Don't share the mailbox of the video frame with the compositor, because What does this comment mean? https://codereview.chromium.org/175223003/diff/350001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/175223003/diff/350001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:109: const std::vector<uint32>&); needs a variable name https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:171: const scoped_refptr<media::VideoFrame>& video_frame, Is it a bit weird that this method takes a texture_id and a whole VideoFrame and then makes an assumption that the video frame's backed by the given texture_id? Shouldn't this assumption be done at another level that actually knows this/generates the texture_id? And this method would just take a sync point if needed? https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:207: gfx::Rect visible_rect = video_frame->visible_rect(); Unrelated change, do this separately? https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:217: gles2->Flush(); Why is this flush needed? https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:319: media::VideoFrame::ReadPixelsCB()); Unrelated change, do this separately?
Thank you for review. I'll address your improvement points soon. https://codereview.chromium.org/175223003/diff/350001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/175223003/diff/350001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:346: // Don't share the mailbox of the video frame with the compositor, because On 2014/03/06 19:31:31, danakj wrote: > What does this comment mean? Sorry for broken explanation. ResourceProvider actually keeps this copied mailbox, instead of referencing to video_frame->mailbox_holder(). So ubercompositor can read/write mailbox_holder->sync_point safely. https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:171: const scoped_refptr<media::VideoFrame>& video_frame, On 2014/03/06 19:31:31, danakj wrote: > Is it a bit weird that this method takes a texture_id and a whole VideoFrame and > then makes an assumption that the video frame's backed by the given texture_id? > Shouldn't this assumption be done at another level that actually knows > this/generates the texture_id? And this method would just take a sync point if > needed? That's very good point. This method is used by gpu video decoder in the media thread, which made the video frame. It's why providing texture_id is possible. Actually, this method does not need wait/insert sync point because it runs in the media thread. I'll rollback the change in this method, and add this explanation as comment. https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:217: gles2->Flush(); On 2014/03/06 19:31:31, danakj wrote: > Why is this flush needed? I use flush as idiom. As I mentioned above, I'll rollback this change in this method. https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/175223003/diff/350001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:319: media::VideoFrame::ReadPixelsCB()); On 2014/03/06 19:31:31, danakj wrote: > Unrelated change, do this separately? ok, i'll do this change in separate CL.
When I tried to address the change in renderer_gpu_video_accelerator_factories.cc, I found interesting fact. I changes GpuVideoAcceleratorFactories::ReadPixels() to be ignostic to GpuVideoDecore, RTCVideoDecore and VideoSource. Could you review again? https://codereview.chromium.org/175223003/diff/360001/content/renderer/media/... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/175223003/diff/360001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:170: const scoped_refptr<media::VideoFrame>& video_frame, This CL changes GpuVideoAcceleratorFactories::ReadPixels() to receive VideoFrame, instead of texture id. Currently, this method can be called by only clients that knows texture id, which means that the clients made the texture. However, VideoCapture don't know texture id. https://codereview.chromium.org/175223003/diff/360001/content/renderer/media/... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:227: gles2->Flush(); Now, this API can be called for the video frame that is created in other process. https://codereview.chromium.org/175223003/diff/360001/content/renderer/media/... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/175223003/diff/360001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:294: NOTIMPLEMENTED(); Now GpuVideoAcceleratorFactories::ReadPixels() receive VideoFrame, instead of texture. So I'll implement it in next CL. After that, software 2d canvas can draws video based on VideoSource which is HW accelerated.
danajk@, can I ask a question? When two context3d is in the same process, don't we need a sync point? context3d of WebGL and context3d of GpuVideoDecoder are always in the same render process, while context3D of the compositor can be in other process because of Ubercompositor. In this case, don't we need sync point synchronization between context3d of WebGL and context3d of GpuVideoDecoder?
My POV is usually "always use a sync point between contexts because that's the safest and most future proof" piman gave me this as a better explanation of where are at and going: "Today, you don't need a sync point if the 2 contexts are on the same channel (i.e. in practice, in the same process), a Flush is enough in that case. Now, there's interest in relaxing the semantics, needing a sync point if 2 contexts are on the same channel, Flush being only sufficient if the 2 contexts are in the same share group. So I think it's good practice to start using sync points when sharing across share groups, even though it won't make a difference." On Wed, Mar 12, 2014 at 7:40 AM, <dongseong.hwang@intel.com> wrote: > danajk@, can I ask a question? When two context3d is in the same process, > don't > we need a sync point? > > context3d of WebGL and context3d of GpuVideoDecoder are always in the same > render process, while context3D of the compositor can be in other process > because of Ubercompositor. > In this case, don't we need sync point synchronization between context3d of > WebGL and context3d of GpuVideoDecoder? > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
danakj@, could you review again? Now I'm sure this CL is needed because of shared group. The CL makes VideoFrame be able to receive multiple sync point for the compositor and WebGL. And then VideoFrame sends the release sync points to release callback of VideoFrame providers such as GVD, RVD, VideoCapture.
https://codereview.chromium.org/175223003/diff/420001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/175223003/diff/420001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:687: video_frame->AppendReleaseSyncPoint(web_graphics_context->insertSyncPoint()); sievers@, I didnot add this line to webmediaplayer_android.cc, because WMPA uses one stream texture, so it is not needed.
https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:272: frame->AppendReleaseSyncPoint(sync_points[i]); Do you know where is the callback that will eventually receive these sync points? I'm trying to follow this back, but the VideoFrames I see are: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... and https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... which are both WrapExternalPackedMemory. I'm failing to see how a native texture frame would end up here.
https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:272: frame->AppendReleaseSyncPoint(sync_points[i]); That's good question. AFAIK, currently VidoeCaptureController don't create NATIVE_TEXTURE VideoFrame. It means this code exist just for future. Ami, jln@, am I right?
Hi, could you have a time to look at this CL? Sorry for big CL. On 2014/03/20 16:11:20, dshwang wrote: > https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... > File content/browser/renderer_host/media/video_capture_controller.cc (right): > > https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... > content/browser/renderer_host/media/video_capture_controller.cc:272: > frame->AppendReleaseSyncPoint(sync_points[i]); > That's good question. AFAIK, currently VidoeCaptureController don't create > NATIVE_TEXTURE VideoFrame. It means this code exist just for future. > Ami, jln@, am I right? In detail, VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread(..) in browser process deliveries video_frame. This method deliveries only video_frame made by media::VideoFrame::WrapExternalPackedMemory(). There are two producing methods, which creates a ExternalPackedMemory VideoFrame. 1. ThreadSafeCaptureOracle::ObserveEventAndDecideCapture() 2. VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedData()
On 2014/03/24 18:16:18, dshwang wrote: > Hi, could you have a time to look at this CL? Sorry for big CL.
On 2014/03/24 18:16:18, dshwang wrote: > Hi, could you have a time to look at this CL? Sorry for big CL. > > On 2014/03/20 16:11:20, dshwang wrote: > > > https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... > > File content/browser/renderer_host/media/video_capture_controller.cc (right): > > > > > https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... > > content/browser/renderer_host/media/video_capture_controller.cc:272: > > frame->AppendReleaseSyncPoint(sync_points[i]); > > That's good question. AFAIK, currently VidoeCaptureController don't create > > NATIVE_TEXTURE VideoFrame. It means this code exist just for future. > > Ami, jln@, am I right? > > In detail, VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread(..) in > browser process deliveries video_frame. > This method deliveries only video_frame made by > media::VideoFrame::WrapExternalPackedMemory(). > > There are two producing methods, which creates a ExternalPackedMemory > VideoFrame. > 1. ThreadSafeCaptureOracle::ObserveEventAndDecideCapture() > 2. VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedData() So we don't in fact support NATIVE_TEXTURE here? We keep passing around sync points that will never get used then. Can we remove this entirely?
No, there should be capture paths using textures as well (e.g. desktop/tab/window capture). OnIncomingCapturedVideoFrame is the path those take, I believe. <https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...> On Wed, Mar 26, 2014 at 11:10 AM, <danakj@chromium.org> wrote: > On 2014/03/24 18:16:18, dshwang wrote: > >> Hi, could you have a time to look at this CL? Sorry for big CL. >> > > On 2014/03/20 16:11:20, dshwang wrote: >> > >> > > https://codereview.chromium.org/175223003/diff/420001/ > content/browser/renderer_host/media/video_capture_controller.cc > >> > File content/browser/renderer_host/media/video_capture_controller.cc >> > (right): > >> > >> > >> > > https://codereview.chromium.org/175223003/diff/420001/ > content/browser/renderer_host/media/video_capture_controller.cc#newcode272 > >> > content/browser/renderer_host/media/video_capture_controller.cc:272: >> > frame->AppendReleaseSyncPoint(sync_points[i]); >> > That's good question. AFAIK, currently VidoeCaptureController don't >> create >> > NATIVE_TEXTURE VideoFrame. It means this code exist just for future. >> > Ami, jln@, am I right? >> > > In detail, VideoCaptureController::DoIncomingCapturedVideoFrameOn >> IOThread(..) >> > in > >> browser process deliveries video_frame. >> This method deliveries only video_frame made by >> media::VideoFrame::WrapExternalPackedMemory(). >> > > There are two producing methods, which creates a ExternalPackedMemory >> VideoFrame. >> 1. ThreadSafeCaptureOracle::ObserveEventAndDecideCapture() >> 2. VideoCaptureController::VideoCaptureDeviceClient:: >> OnIncomingCapturedData() >> > > So we don't in fact support NATIVE_TEXTURE here? We keep passing around > sync > points that will never get used then. Can we remove this entirely? > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/26 18:10:11, danakj wrote: > > There are two producing methods, which creates a ExternalPackedMemory > > VideoFrame. > > 1. ThreadSafeCaptureOracle::ObserveEventAndDecideCapture() > > 2. VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedData() > > So we don't in fact support NATIVE_TEXTURE here? We keep passing around sync > points that will never get used then. Can we remove this entirely? I think remaining hw path is better, because it can be used soon. For example, VideoCaptureDeviceLinux uses L4V2 library to get cam video frame. L4V2 library's HW acceleration is almost done. imo, ChromeOS can take advantage of L4V2's gpu acceleration soon. On 2014/03/26 18:19:20, Ami Fischman wrote: > No, there should be capture paths using textures as well (e.g. > desktop/tab/window capture). > OnIncomingCapturedVideoFrame is the path those take, I believe. Currently, NATIVE_TEXTURE video frame is created by only three classes: webmediaplayer_android, rtc_video_decoder, gpu_video_decoder. FYI, video_capture_impl is proxy in render process. So not real source of video frame. $ git grep "VideoFrame::WrapNative" content/renderer/media/android/webmediaplayer_android.cc: scoped_refptr<VideoFrame> new_frame = VideoFrame::WrapNativeTexture( content/renderer/media/android/webmediaplayer_android.cc: scoped_refptr<VideoFrame> new_frame = VideoFrame::WrapNativeTexture( content/renderer/media/rtc_video_decoder.cc: return media::VideoFrame::WrapNativeTexture( content/renderer/media/video_capture_impl.cc: scoped_refptr<media::VideoFrame> frame = media::VideoFrame::WrapNativeTexture( media/filters/gpu_video_decoder.cc: scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture(
> > On 2014/03/26 18:19:20, Ami Fischman wrote: > >> No, there should be capture paths using textures as well (e.g. >> desktop/tab/window capture). >> OnIncomingCapturedVideoFrame is the path those take, I believe. > > Currently, NATIVE_TEXTURE video frame is created by only three classes: > webmediaplayer_android, rtc_video_decoder, gpu_video_decoder. > https://chromiumcodereview.appspot.com/213253005/ has been sitting uncommitted for a long time, but should land soon. So the codepaths won't be dead in a bit. Sorry for the confusion. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/26 19:39:08, Ami Fischman wrote: > > > > On 2014/03/26 18:19:20, Ami Fischman wrote: > > > >> No, there should be capture paths using textures as well (e.g. > >> desktop/tab/window capture). > >> OnIncomingCapturedVideoFrame is the path those take, I believe. > > > > Currently, NATIVE_TEXTURE video frame is created by only three classes: > > webmediaplayer_android, rtc_video_decoder, gpu_video_decoder. > > > > https://chromiumcodereview.appspot.com/213253005/ has been sitting > uncommitted for a long time, but should land soon. So the codepaths won't > be dead in a bit. Sorry for the confusion. Thank you for explaining what's going on. Without this CL, gpu desktop capture causes following gl error when WebGL draws the desktop capture. GL ERROR :GL_INVALID_OPERATION :glConsumeTextureCHROMIUM:invalid mailbox name
On 2014/03/26 19:39:08, Ami Fischman wrote: > > > > On 2014/03/26 18:19:20, Ami Fischman wrote: > > > >> No, there should be capture paths using textures as well (e.g. > >> desktop/tab/window capture). > >> OnIncomingCapturedVideoFrame is the path those take, I believe. > > > > Currently, NATIVE_TEXTURE video frame is created by only three classes: > > webmediaplayer_android, rtc_video_decoder, gpu_video_decoder. > > > > https://chromiumcodereview.appspot.com/213253005/ has been sitting > uncommitted for a long time, but should land soon. So the codepaths won't > be dead in a bit. Sorry for the confusion. Thank you for explaining what's going on. Without this CL, gpu desktop capture causes following gl error when WebGL draws the desktop capture. GL ERROR :GL_INVALID_OPERATION :glConsumeTextureCHROMIUM:invalid mailbox name
LGTM https://codereview.chromium.org/175223003/diff/420001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/175223003/diff/420001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:686: web_graphics_context->flush(); I wonder if this flush was trying to deal with this issue, or what "performance" it means now. Maybe you can revisit this flush afterwards?
Thank you for review. Ami, sievers@, could you take a review content/.*/media and media/? https://codereview.chromium.org/175223003/diff/420001/content/renderer/media/... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/175223003/diff/420001/content/renderer/media/... content/renderer/media/webmediaplayer_impl.cc:686: web_graphics_context->flush(); On 2014/04/11 16:23:28, danakj wrote: > I wonder if this flush was trying to deal with this issue, or what "performance" > it means now. Maybe you can revisit this flush afterwards? This flush is logically needed because we want to make sure gpu driver execute WebGL's all operations when GPU video decoder context waits for below sync point.
I like this CL a lot! https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:272: frame->AppendReleaseSyncPoint(sync_points[i]); On 2014/03/20 16:11:20, dshwang wrote: > That's good question. AFAIK, currently VidoeCaptureController don't create > NATIVE_TEXTURE VideoFrame. It means this code exist just for future. > Ami, jln@, am I right? Yep. https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:269: called_release_sync_points->swap(sync_points); nit: ditto comment elsewhere personally I find clear+insert to be more readable than ctor+swap https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.cc:426: base::AutoLock locker(release_sync_point_lock_); locking in a dtor is a code-smell. If anything can race with the dtor then losing the race means it will read deleted memory. Since VideoFrame is RefCountedThreadSafe I believe this lock (and the swap below) is unnecessary. https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.cc:434: DCHECK_EQ(debug_initial_sync_point_, mailbox_holder_->sync_point); This would be even easier to guarantee if nobody had the ability to modify mailbox_holder_->sync_point, which would be the case if mailbox_holder() died. Can you delete that accessor in favor of direct (const!) accessors to *mailbox_holder_'s members? Then debug_initial_sync_point_ would no longer be needed (assuming I'm right about l.436 below also being deletable because it's enforcing a contract that nobody guarantees). https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.cc:436: DCHECK_NE(debug_initial_sync_point_, release_sync_points[i]); What guarantees that wrap-around doesn't hit this? https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.cc:521: if (!sync_point) OOC where does it say that a sync_point of zero means "no sync point"? (I'm a relative n00b to the world of mailboxes) https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.h:91: typedef base::Callback<void(const std::vector<uint32>&)> ReleaseMailboxCB; FYI this is now a blocker for https://code.google.com/p/chromium/issues/detail?id=362521 so I'm happy to see this CL :) https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.h:234: // Multiple clients can append multiple sync points in one frame. s/in/to/ https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame_... File media/base/video_frame_unittest.cc (right): https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame_... media/base/video_frame_unittest.cc:254: called_sync_point->swap(sync_points); nit: simpler as: called_sync_point->clear(); called_sync_point->insert(sync_points->begin(), sync_points->end()); ?
https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.cc:521: if (!sync_point) On 2014/04/11 20:55:17, Ami Fischman wrote: > OOC where does it say that a sync_point of zero means "no sync point"? > (I'm a relative n00b to the world of mailboxes) Hm, it's a convention we follow to return 0 when we want to say "didn't use it, just use the old sync point" Maybe we should add this to ./gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt
On 2014/04/14 13:19:31, danakj wrote: > https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.cc > File media/base/video_frame.cc (right): > > https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... > media/base/video_frame.cc:521: if (!sync_point) > On 2014/04/11 20:55:17, Ami Fischman wrote: > > OOC where does it say that a sync_point of zero means "no sync point"? > > (I'm a relative n00b to the world of mailboxes) > > Hm, it's a convention we follow to return 0 when we want to say "didn't use it, > just use the old sync point" > > Maybe we should add this to > ./gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt We return 1 from the very first InsertSyncPoint() call received, after that it's just an increasing counter. Calling waitSyncPoint() with 0 doesn't block, because it looks like the sync point is already retired. For the sake of keeping the client code simpler, we could also special case this as little as possible.
On 2014/04/15 17:32:23, sievers wrote: > On 2014/04/14 13:19:31, danakj wrote: > > > https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.cc > > File media/base/video_frame.cc (right): > > > > > https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... > > media/base/video_frame.cc:521: if (!sync_point) > > On 2014/04/11 20:55:17, Ami Fischman wrote: > > > OOC where does it say that a sync_point of zero means "no sync point"? > > > (I'm a relative n00b to the world of mailboxes) > > > > Hm, it's a convention we follow to return 0 when we want to say "didn't use > it, > > just use the old sync point" > > > > Maybe we should add this to > > ./gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt > > We return 1 from the very first InsertSyncPoint() call received, after that it's > just an increasing counter. Calling waitSyncPoint() with 0 doesn't block, > because it looks like the sync point is already retired. For the sake of keeping > the client code simpler, we could also special case this as little as possible. Well, as long as we don't drop another legit/newer sync point for that (since you're saying "didn't use it, just use the old sync point").
@sievers: do you make sure to avoid 0 on wrap-around? On Tue, Apr 15, 2014 at 10:32 AM, <sievers@chromium.org> wrote: > On 2014/04/14 13:19:31, danakj wrote: > > https://codereview.chromium.org/175223003/diff/420001/ > media/base/video_frame.cc > >> File media/base/video_frame.cc (right): >> > > > https://codereview.chromium.org/175223003/diff/420001/ > media/base/video_frame.cc#newcode521 > >> media/base/video_frame.cc:521: if (!sync_point) >> On 2014/04/11 20:55:17, Ami Fischman wrote: >> > OOC where does it say that a sync_point of zero means "no sync point"? >> > (I'm a relative n00b to the world of mailboxes) >> > > Hm, it's a convention we follow to return 0 when we want to say "didn't >> use >> > it, > >> just use the old sync point" >> > > Maybe we should add this to >> ./gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt >> > > We return 1 from the very first InsertSyncPoint() call received, after > that it's > just an increasing counter. Calling waitSyncPoint() with 0 doesn't block, > because it looks like the sync point is already retired. For the sake of > keeping > the client code simpler, we could also special case this as little as > possible. > > > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/15 17:35:57, Ami Fischman wrote: > @sievers: do you make sure to avoid 0 on wrap-around? > We actually don't, though we easily could. I think we haven't really deemed it necessary, because it would take years for normal operations (a few sync points per frame). > > On Tue, Apr 15, 2014 at 10:32 AM, <mailto:sievers@chromium.org> wrote: > > > On 2014/04/14 13:19:31, danakj wrote: > > > > https://codereview.chromium.org/175223003/diff/420001/ > > media/base/video_frame.cc > > > >> File media/base/video_frame.cc (right): > >> > > > > > > https://codereview.chromium.org/175223003/diff/420001/ > > media/base/video_frame.cc#newcode521 > > > >> media/base/video_frame.cc:521: if (!sync_point) > >> On 2014/04/11 20:55:17, Ami Fischman wrote: > >> > OOC where does it say that a sync_point of zero means "no sync point"? > >> > (I'm a relative n00b to the world of mailboxes) > >> > > > > Hm, it's a convention we follow to return 0 when we want to say "didn't > >> use > >> > > it, > > > >> just use the old sync point" > >> > > > > Maybe we should add this to > >> ./gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt > >> > > > > We return 1 from the very first InsertSyncPoint() call received, after > > that it's > > just an increasing counter. Calling waitSyncPoint() with 0 doesn't block, > > because it looks like the sync point is already retired. For the sake of > > keeping > > the client code simpler, we could also special case this as little as > > possible. > > > > > > > > https://codereview.chromium.org/175223003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Thank you for review, all! Sorry for not-responsible for few days. I've been OOO until next week. On 2014/04/15 17:53:48, sievers_OOOuntil4_22 wrote: > On 2014/04/15 17:35:57, Ami Fischman wrote: > > @sievers: do you make sure to avoid 0 on wrap-around? > > > > > We actually don't, though we easily could. > I think we haven't really deemed it necessary, because it would take years for > normal operations (a few sync points per frame). I agree, but ChromeOS seems to need more stable :) > > > > On Tue, Apr 15, 2014 at 10:32 AM, <mailto:sievers@chromium.org> wrote: > > > > > On 2014/04/14 13:19:31, danakj wrote: > > > > > > https://codereview.chromium.org/175223003/diff/420001/ > > > media/base/video_frame.cc > > > > > >> File media/base/video_frame.cc (right): > > >> > > > > > > > > > https://codereview.chromium.org/175223003/diff/420001/ > > > media/base/video_frame.cc#newcode521 > > > > > >> media/base/video_frame.cc:521: if (!sync_point) > > >> On 2014/04/11 20:55:17, Ami Fischman wrote: > > >> > OOC where does it say that a sync_point of zero means "no sync point"? > > >> > (I'm a relative n00b to the world of mailboxes) > > >> > > > > > > Hm, it's a convention we follow to return 0 when we want to say "didn't > > >> use > > >> > > > it, > > > > > >> just use the old sync point" > > >> > > > > > > Maybe we should add this to > > >> ./gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt > > >> I'll change From "if(!sync_point) return;" To "DCHECK(sync_point)" AFAIK, InsertSyncPoint never return 0, and WaitSyncPoint(0) is no-op. In my opinion, saying "didn't use it, just use the old sync point" is not necessary, because WaitSyncPoint(0) is just no-op. > > > We return 1 from the very first InsertSyncPoint() call received, after > > > that it's > > > just an increasing counter. Calling waitSyncPoint() with 0 doesn't block, > > > because it looks like the sync point is already retired. For the sake of > > > keeping > > > the client code simpler, we could also special case this as little as > > > possible. Thank you for explanation. On 2014/04/11 20:55:16, Ami Fischman wrote: > I like this CL a lot! Thank you! > https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... > File content/browser/renderer_host/media/video_capture_controller_unittest.cc > (right): > > https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... > content/browser/renderer_host/media/video_capture_controller_unittest.cc:269: > called_release_sync_points->swap(sync_points); > nit: ditto comment elsewhere personally I find clear+insert to be more readable > than ctor+swap I'll change. > https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.cc > File media/base/video_frame.cc (right): > > https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... > media/base/video_frame.cc:426: base::AutoLock locker(release_sync_point_lock_); > locking in a dtor is a code-smell. If anything can race with the dtor then > losing the race means it will read deleted memory. > > Since VideoFrame is RefCountedThreadSafe I believe this lock (and the swap > below) is unnecessary. I agree lock in dtor is smell, but in my opinion, it's clear from thread-safe code perspective. Mutex release_sync_point_lock_ exists only for Vector release_sync_points_. When we r/w release_sync_points_, release_sync_point_lock_ is always used in other places. Piggyback of RefCountedThreadSafe to ensure visibility of release_sync_points_ in other thread is possible, because memory fence of x86 and arm is global, not applied to local memory. However, it looks dangerous and if someone change RefCountedThreadSafe to RefCounted, thread bug will slightly be emerged. > https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... > media/base/video_frame.cc:434: DCHECK_EQ(debug_initial_sync_point_, > mailbox_holder_->sync_point); > This would be even easier to guarantee if nobody had the ability to modify > mailbox_holder_->sync_point, which would be the case if mailbox_holder() died. > Can you delete that accessor in favor of direct (const!) accessors to > *mailbox_holder_'s members? That's good point. However, the compositor changes WebGL's mailbox_holder. VideoFrame shares the same MailboxHolder class with WebGL, Canvas, etc. > https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... > media/base/video_frame.cc:436: DCHECK_NE(debug_initial_sync_point_, > release_sync_points[i]); > What guarantees that wrap-around doesn't hit this? Nobody guarantee. We have a lot of similar code. I think when ChromeOS is used for server, we need to handle it :)
On Wed, Apr 16, 2014 at 9:42 PM, <dongseong.hwang@intel.com> wrote: > Thank you for review, all! Sorry for not-responsible for few days. I've > been OOO > until next week. > > > On 2014/04/15 17:53:48, sievers_OOOuntil4_22 wrote: > >> On 2014/04/15 17:35:57, Ami Fischman wrote: >> > @sievers: do you make sure to avoid 0 on wrap-around? >> > >> > > > We actually don't, though we easily could. >> I think we haven't really deemed it necessary, because it would take >> years for >> normal operations (a few sync points per frame). >> > > I agree, but ChromeOS seems to need more stable :) > > > > >> > On Tue, Apr 15, 2014 at 10:32 AM, <mailto:sievers@chromium.org> wrote: >> > >> > > On 2014/04/14 13:19:31, danakj wrote: >> > > >> > > https://codereview.chromium.org/175223003/diff/420001/ >> > > media/base/video_frame.cc >> > > >> > >> File media/base/video_frame.cc (right): >> > >> >> > > >> > > >> > > https://codereview.chromium.org/175223003/diff/420001/ >> > > media/base/video_frame.cc#newcode521 >> > > >> > >> media/base/video_frame.cc:521: if (!sync_point) >> > >> On 2014/04/11 20:55:17, Ami Fischman wrote: >> > >> > OOC where does it say that a sync_point of zero means "no sync >> point"? >> > >> > (I'm a relative n00b to the world of mailboxes) >> > >> >> > > >> > > Hm, it's a convention we follow to return 0 when we want to say >> "didn't >> > >> use >> > >> >> > > it, >> > > >> > >> just use the old sync point" >> > >> >> > > >> > > Maybe we should add this to >> > >> ./gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt >> > >> >> > > I'll change > From "if(!sync_point) return;" > To "DCHECK(sync_point)" > > AFAIK, InsertSyncPoint never return 0, and WaitSyncPoint(0) is no-op. > In my opinion, saying "didn't use it, just use the old sync point" is not > necessary, because WaitSyncPoint(0) is just no-op. There are places in the code that return 0 when they don't use the mailbox though. Grep for "Run(0, false)" to see (most of?) them. Maybe none will hit this method today, but I don't think that 0 should be considered an unexpected error. I'd prefer we drop them as you had done. > > > > > > We return 1 from the very first InsertSyncPoint() call received, after >> > > that it's >> > > just an increasing counter. Calling waitSyncPoint() with 0 doesn't >> block, >> > > because it looks like the sync point is already retired. For the sake >> of >> > > keeping >> > > the client code simpler, we could also special case this as little as >> > > possible. >> > > Thank you for explanation. > > > > On 2014/04/11 20:55:16, Ami Fischman wrote: > >> I like this CL a lot! >> > > Thank you! > > > > https://codereview.chromium.org/175223003/diff/420001/ > content/browser/renderer_host/media/video_capture_controller_unittest.cc > >> File content/browser/renderer_host/media/video_capture_ >> controller_unittest.cc >> (right): >> > > > https://codereview.chromium.org/175223003/diff/420001/ > content/browser/renderer_host/media/video_capture_controller_unittest.cc# > newcode269 > >> content/browser/renderer_host/media/video_capture_ >> controller_unittest.cc:269: >> called_release_sync_points->swap(sync_points); >> nit: ditto comment elsewhere personally I find clear+insert to be more >> > readable > >> than ctor+swap >> > > I'll change. > > > > https://codereview.chromium.org/175223003/diff/420001/ > media/base/video_frame.cc > >> File media/base/video_frame.cc (right): >> > > > https://codereview.chromium.org/175223003/diff/420001/ > media/base/video_frame.cc#newcode426 > >> media/base/video_frame.cc:426: base::AutoLock >> > locker(release_sync_point_lock_); > >> locking in a dtor is a code-smell. If anything can race with the dtor >> then >> losing the race means it will read deleted memory. >> > > Since VideoFrame is RefCountedThreadSafe I believe this lock (and the swap >> below) is unnecessary. >> > > I agree lock in dtor is smell, but in my opinion, it's clear from > thread-safe > code perspective. > Mutex release_sync_point_lock_ exists only for Vector > release_sync_points_. When > we r/w release_sync_points_, release_sync_point_lock_ is always used in > other > places. > Piggyback of RefCountedThreadSafe to ensure visibility of > release_sync_points_ > in other thread is possible, because memory fence of x86 and arm is > global, not > applied to local memory. However, it looks dangerous and if someone change > RefCountedThreadSafe to RefCounted, thread bug will slightly be emerged. > > > > > https://codereview.chromium.org/175223003/diff/420001/ > media/base/video_frame.cc#newcode434 > >> media/base/video_frame.cc:434: DCHECK_EQ(debug_initial_sync_point_, >> mailbox_holder_->sync_point); >> This would be even easier to guarantee if nobody had the ability to modify >> mailbox_holder_->sync_point, which would be the case if mailbox_holder() >> died. >> > > Can you delete that accessor in favor of direct (const!) accessors to >> *mailbox_holder_'s members? >> > > That's good point. > However, the compositor changes WebGL's mailbox_holder. VideoFrame shares > the > same MailboxHolder class with WebGL, Canvas, etc. > > > > > https://codereview.chromium.org/175223003/diff/420001/ > media/base/video_frame.cc#newcode436 > >> media/base/video_frame.cc:436: DCHECK_NE(debug_initial_sync_point_, >> release_sync_points[i]); >> What guarantees that wrap-around doesn't hit this? >> > > Nobody guarantee. We have a lot of similar code. I think when ChromeOS is > used > for server, we need to handle it :) > > > > https://codereview.chromium.org/175223003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry for delayed response. I was ooo. Could you have a chance to review again? https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/175223003/diff/420001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller_unittest.cc:269: called_release_sync_points->swap(sync_points); On 2014/04/11 20:55:17, Ami Fischman wrote: > nit: ditto comment elsewhere personally I find clear+insert to be more readable > than ctor+swap Done. https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.cc:426: base::AutoLock locker(release_sync_point_lock_); I reserve this code as I explained. IMO ensuring thread visibility via piggy-back is more difficult to maintain. https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.cc:434: DCHECK_EQ(debug_initial_sync_point_, mailbox_holder_->sync_point); That's good idea. I make mailbox_holder const. And this change requires changing VideoFrame ctor little bit. https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame.... media/base/video_frame.cc:521: if (!sync_point) On 2014/04/17 14:24:27, danakj wrote: > There are places in the code that return 0 when they don't use the mailbox > though. Grep for "Run(0, false)" to see (most of?) them. > > Maybe none will hit this method today, but I don't think that 0 should be > considered an unexpected error. I'd prefer we drop them as you had done. Thank you for kind explanation. I agree, so I reserve this code. https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame_... File media/base/video_frame_unittest.cc (right): https://codereview.chromium.org/175223003/diff/420001/media/base/video_frame_... media/base/video_frame_unittest.cc:254: called_sync_point->swap(sync_points); On 2014/04/11 20:55:17, Ami Fischman wrote: > nit: simpler as: > called_sync_point->clear(); > called_sync_point->insert(sync_points->begin(), sync_points->end()); > ? Done.
LGTM BTW, in the future, please don't rebase while there are outstanding comments as it makes reviewing inter-patchset diffs unnecessarily difficult. (see the _Important_ note in http://dev.chromium.org/developers/contributing-code#TOC-The-review-process) https://codereview.chromium.org/175223003/diff/440001/media/base/video_frame_... File media/base/video_frame_unittest.cc (right): https://codereview.chromium.org/175223003/diff/440001/media/base/video_frame_... media/base/video_frame_unittest.cc:246: called_sync_point->assign(release_sync_points.begin(), assign obviates the need for clear?
On 2014/04/24 01:19:49, Ami Fischman wrote: > LGTM Thank you for review! > BTW, in the future, please don't rebase while there are outstanding comments as > it makes reviewing inter-patchset diffs unnecessarily difficult. > (see the _Important_ note in > http://dev.chromium.org/developers/contributing-code#TOC-The-review-process) Ah, I didn't know although I was afraid how reviewer trace only diff. Thank you for education. > https://codereview.chromium.org/175223003/diff/440001/media/base/video_frame_... > File media/base/video_frame_unittest.cc (right): > > https://codereview.chromium.org/175223003/diff/440001/media/base/video_frame_... > media/base/video_frame_unittest.cc:246: > called_sync_point->assign(release_sync_points.begin(), > assign obviates the need for clear? You're right. I'll update. jln@, could you review content/common/media/video_capture_messages.h ? Ami, I guess you should belong to OWNER of content/common/media also. :)
https://codereview.chromium.org/175223003/diff/440001/media/base/video_frame_... File media/base/video_frame_unittest.cc (right): https://codereview.chromium.org/175223003/diff/440001/media/base/video_frame_... media/base/video_frame_unittest.cc:246: called_sync_point->assign(release_sync_points.begin(), On 2014/04/24 01:19:50, Ami Fischman wrote: > assign obviates the need for clear? Done in new patch set.
On Wed, Apr 23, 2014 at 9:34 PM, <dongseong.hwang@intel.com> wrote: > Ami, I guess you should belong to OWNER of content/common/media also. :) > I am: http://src.chromium.org/viewvc/chrome/trunk/src/content/common/media/OWNERS *_messages.h files must be reviewed by security-team members as well, hence the noparent in that file. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/24 06:23:54, Ami Fischman wrote: > On Wed, Apr 23, 2014 at 9:34 PM, <mailto:dongseong.hwang@intel.com> wrote: > > > Ami, I guess you should belong to OWNER of content/common/media also. :) > > > > I am: > http://src.chromium.org/viewvc/chrome/trunk/src/content/common/media/OWNERS > *_messages.h files must be reviewed by security-team members as well, hence > the noparent in that file. aha, it's why Chromite Butler complained content/common/media/video_capture_messages.h is not reviewed. jln@, could you have a change to review the messages file?
On 2014/04/24 07:04:52, dshwang wrote: > On 2014/04/24 06:23:54, Ami Fischman wrote: > > On Wed, Apr 23, 2014 at 9:34 PM, <mailto:dongseong.hwang@intel.com> wrote: > > > > > Ami, I guess you should belong to OWNER of content/common/media also. :) > > > > > > > I am: > > http://src.chromium.org/viewvc/chrome/trunk/src/content/common/media/OWNERS > > *_messages.h files must be reviewed by security-team members as well, hence > > the noparent in that file. > > aha, it's why Chromite Butler complained > content/common/media/video_capture_messages.h is not reviewed. > jln@, could you have a change to review the messages file? jln@, cdn@, could you have a change to review the messages file? content/common/media/video_capture_messages.h
On 2014/04/26 18:48:55, dshwang wrote: > On 2014/04/24 07:04:52, dshwang wrote: > > On 2014/04/24 06:23:54, Ami Fischman wrote: > > > On Wed, Apr 23, 2014 at 9:34 PM, <mailto:dongseong.hwang@intel.com> wrote: > > > > > > > Ami, I guess you should belong to OWNER of content/common/media also. :) > > > > > > > > > > I am: > > > http://src.chromium.org/viewvc/chrome/trunk/src/content/common/media/OWNERS > > > *_messages.h files must be reviewed by security-team members as well, hence > > > the noparent in that file. > > > > aha, it's why Chromite Butler complained > > content/common/media/video_capture_messages.h is not reviewed. > > jln@, could you have a change to review the messages file? > > jln@, cdn@, could you have a change to review the messages file? > content/common/media/video_capture_messages.h IPC changes LGTM
On 2014/04/28 17:45:36, Cris Neckar wrote: > IPC changes LGTM Thank you for review! Finally, this CL pass review. Thank you all. I'll keep eyes on if it breaks something.
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/175223003/48...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium ios_rel_device on tryserver.chromium
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/175223003/48...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
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/175223003/50...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
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/175223003/50...
Message was sent while issue was closed.
Change committed as 267290 |