Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24762003:
Set the texture to cleared in VideoDecodeAccelerator::PictureReady. (Closed)
|
Created:
7 years, 2 months ago by wuchengli Modified:
7 years, 2 months ago CC:
chromium-reviews, jam, apatrick_chromium, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSet textures to cleared in VideoDecodeAccelerator::PictureReady.
Instead of clearing the textures in GVDA::AssignPictureBuffers, set
them to cleared in PictureReady. This can make OnAssignPictureBuffers
faster. This can reduce the latency when playing a video or starting
webrtc. This also make resolution change faster.
The test was done on Daisy running apprtc.appspot.com with 720p
decode. AssignPictureBuffers is reduced from 142ms to 24ms.
The decode time of the first three frames are reduced from
202ms, 163ms, 144ms to 74ms, 36ms, 26ms respectively.
BUG=298176
TEST=Play video. Run apprtc loopback. Run vda unittest.
Run resolution change in DASH youtube. Test raw resolution switch.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226492
Patch Set 1 #
Total comments: 16
Patch Set 2 : rebase #Patch Set 3 : address review comments #
Total comments: 14
Patch Set 4 : address review comments #
Total comments: 2
Patch Set 5 : add more comments in EVDA #
Total comments: 1
Patch Set 6 : fix a WeakPtr DCHECK fail and handle Reset/Flush cases #Patch Set 7 : rebase #Patch Set 8 : send PictureReady to IO thread during Reset or Flush if picture_clearing_count_ == 0 #Patch Set 9 : refactor SendPictureReady by kcwu's suggestion #
Messages
Total messages: 29 (0 generated)
PTAL.
LGTM % nits & piman confirmation that SLC requires being on the child thread. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:478: texture_ids_.erase(texture_ids_.begin() + i); Would require less book-keeping as a vector of pairs (buffer/texture ids) or a map from buffer id to texture id, FWIW. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.h:127: std::vector<uint32> texture_ids_; comments for this and previous field could be clearer (s/set level cleared/been cleared/) names for both fields would benefit from an uncleared_ prefix, IMO.
On 2013/09/26 16:21:55, Ami Fischman wrote: > LGTM % nits & piman confirmation that SLC requires being on the child thread. > > https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... > File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... > content/common/gpu/media/gpu_video_decode_accelerator.cc:478: > texture_ids_.erase(texture_ids_.begin() + i); > Would require less book-keeping as a vector of pairs (buffer/texture ids) or a > map from buffer id to texture id, FWIW. > > https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... > File content/common/gpu/media/gpu_video_decode_accelerator.h (right): > > https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... > content/common/gpu/media/gpu_video_decode_accelerator.h:127: std::vector<uint32> > texture_ids_; > comments for this and previous field could be clearer (s/set level cleared/been > cleared/) > > names for both fields would benefit from an uncleared_ prefix, IMO. Thanks for the comments. I'll update the CL tomorrow (I can't test the new patchset at home).
re: Ami's question, yes, SetLevelCleared has to be called on the main thread, the GPU data structures are not thread-safe. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exyn... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exyn... content/common/gpu/media/exynos_video_decode_accelerator.cc:1422: // reduce latency. So, if the main (child) thread is busy, this can mean the PictureReady can come out-of-order to the client. E.g. Picture N needs to be clear, we post task to the main thread, it waits in the task queue because main thread is busy. Picture N+1 doesn't need it, we post to the IO thread, it goes to the client right way. Main thread unblocks, we SetLevelCleared on the picture N's texture, and send that to the client. So the client receives N+1 before N. Is that ok? https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:186: if (child_message_loop_->BelongsToCurrentThread()) nit: needs brackets per style guide. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:188: return; Is there a way to DCHECK that we don't get here on the IO thread without the texture having been set cleared? For example check the list that you keep? You'd need a lock, but only in debug, so that would be ok I think. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:455: for (i = 0; i < buffer_ids_.size(); i++) nit: needs brackets per style guide https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:464: texture_ids_[i]); Can you, instead, keep a scoped_refptr<TextureRef> when you first lookup the texture in OnAssignPictureBuffers? It is possible that by the time this executes, the original texture has been deleted, and another one took its place on the same id. You don't want to touch the new one. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:478: texture_ids_.erase(texture_ids_.begin() + i); On 2013/09/26 16:21:56, Ami Fischman wrote: > Would require less book-keeping as a vector of pairs (buffer/texture ids) or a > map from buffer id to texture id, FWIW. +1 on map, given the linear lookup above.
It is not ok to deliver pictures out of order. I was thinking that delivery would be serialized but that's only true for a given buffer id since it has to be ReusePictureBuffer()'d before it can be reused, but it's not true between _different_ pix. Separately: please include at least anecdotal perf data with CLs that affect perf. On Thu, Sep 26, 2013 at 9:50 AM, <piman@chromium.org> wrote: > re: Ami's question, yes, SetLevelCleared has to be called on the main > thread, > the GPU data structures are not thread-safe. > > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/exynos_video_**decode_accelerator.cc<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exynos_video_decode_accelerator.cc> > File content/common/gpu/media/**exynos_video_decode_**accelerator.cc > (right): > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/exynos_video_**decode_accelerator.cc#**newcode1422<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exynos_video_decode_accelerator.cc#newcode1422> > content/common/gpu/media/**exynos_video_decode_**accelerator.cc:1422: // > reduce latency. > So, if the main (child) thread is busy, this can mean the PictureReady > can come out-of-order to the client. > E.g. Picture N needs to be clear, we post task to the main thread, it > waits in the task queue because main thread is busy. Picture N+1 doesn't > need it, we post to the IO thread, it goes to the client right way. Main > thread unblocks, we SetLevelCleared on the picture N's texture, and send > that to the client. > So the client receives N+1 before N. Is that ok? > > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc> > File content/common/gpu/media/gpu_**video_decode_accelerator.cc (right): > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode186<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode186> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**186: if > (child_message_loop_->**BelongsToCurrentThread()) > nit: needs brackets per style guide. > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode188<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode188> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**188: return; > Is there a way to DCHECK that we don't get here on the IO thread without > the texture having been set cleared? For example check the list that you > keep? You'd need a lock, but only in debug, so that would be ok I think. > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode455<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode455> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**455: for (i = > 0; i < buffer_ids_.size(); i++) > nit: needs brackets per style guide > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode464<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode464> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**464: > texture_ids_[i]); > Can you, instead, keep a scoped_refptr<TextureRef> when you first lookup > the texture in OnAssignPictureBuffers? > It is possible that by the time this executes, the original texture has > been deleted, and another one took its place on the same id. You don't > want to touch the new one. > > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode478<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode478> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**478: > texture_ids_.erase(texture_**ids_.begin() + i); > On 2013/09/26 16:21:56, Ami Fischman wrote: > >> Would require less book-keeping as a vector of pairs (buffer/texture >> > ids) or a > >> map from buffer id to texture id, FWIW. >> > > +1 on map, given the linear lookup above. > > https://codereview.chromium.**org/24762003/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It is not ok to deliver pictures out of order. I was thinking that delivery would be serialized but that's only true for a given buffer id since it has to be ReusePictureBuffer()'d before it can be reused, but it's not true between _different_ pix. Separately: please include at least anecdotal perf data with CLs that affect perf. On Thu, Sep 26, 2013 at 9:50 AM, <piman@chromium.org> wrote: > re: Ami's question, yes, SetLevelCleared has to be called on the main > thread, > the GPU data structures are not thread-safe. > > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/exynos_video_**decode_accelerator.cc<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exynos_video_decode_accelerator.cc> > File content/common/gpu/media/**exynos_video_decode_**accelerator.cc > (right): > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/exynos_video_**decode_accelerator.cc#**newcode1422<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exynos_video_decode_accelerator.cc#newcode1422> > content/common/gpu/media/**exynos_video_decode_**accelerator.cc:1422: // > reduce latency. > So, if the main (child) thread is busy, this can mean the PictureReady > can come out-of-order to the client. > E.g. Picture N needs to be clear, we post task to the main thread, it > waits in the task queue because main thread is busy. Picture N+1 doesn't > need it, we post to the IO thread, it goes to the client right way. Main > thread unblocks, we SetLevelCleared on the picture N's texture, and send > that to the client. > So the client receives N+1 before N. Is that ok? > > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc> > File content/common/gpu/media/gpu_**video_decode_accelerator.cc (right): > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode186<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode186> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**186: if > (child_message_loop_->**BelongsToCurrentThread()) > nit: needs brackets per style guide. > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode188<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode188> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**188: return; > Is there a way to DCHECK that we don't get here on the IO thread without > the texture having been set cleared? For example check the list that you > keep? You'd need a lock, but only in debug, so that would be ok I think. > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode455<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode455> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**455: for (i = > 0; i < buffer_ids_.size(); i++) > nit: needs brackets per style guide > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode464<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode464> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**464: > texture_ids_[i]); > Can you, instead, keep a scoped_refptr<TextureRef> when you first lookup > the texture in OnAssignPictureBuffers? > It is possible that by the time this executes, the original texture has > been deleted, and another one took its place on the same id. You don't > want to touch the new one. > > > https://codereview.chromium.**org/24762003/diff/1/content/** > common/gpu/media/gpu_video_**decode_accelerator.cc#**newcode478<https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode478> > content/common/gpu/media/gpu_**video_decode_accelerator.cc:**478: > texture_ids_.erase(texture_**ids_.begin() + i); > On 2013/09/26 16:21:56, Ami Fischman wrote: > >> Would require less book-keeping as a vector of pairs (buffer/texture >> > ids) or a > >> map from buffer id to texture id, FWIW. >> > > +1 on map, given the linear lookup above. > > https://codereview.chromium.**org/24762003/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exyn... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exyn... content/common/gpu/media/exynos_video_decode_accelerator.cc:1422: // reduce latency. On 2013/09/26 16:50:01, piman wrote: > So, if the main (child) thread is busy, this can mean the PictureReady can come > out-of-order to the client. > E.g. Picture N needs to be clear, we post task to the main thread, it waits in > the task queue because main thread is busy. Picture N+1 doesn't need it, we post > to the IO thread, it goes to the client right way. Main thread unblocks, we > SetLevelCleared on the picture N's texture, and send that to the client. > So the client receives N+1 before N. Is that ok? It's not OK. I cannot think of a solution now... I'll see if I can come up with a solution tomorrow. Sending on IO thread gains 4ms per frame. It will be good to keep it.
On Thu, Sep 26, 2013 at 10:02 AM, <wuchengli@chromium.org> wrote: > I cannot think of a solution now... How about: remove tracking of cleared/uncleared status from EVDA. Make GVDA track whether assigned pic buffers have been cleared or not. If GVDA::PictureReady is given a not-yet-marked-cleared picture and it's not on the child thread, PostTaskAndReply to the child thread to clear, and start queuing future PictureReady's. While queuing, GVDA::PR only PTAR's for other buffers needing clearing, but doesn't pass the pictures to IPC. The PTAR's Reply callback marks the pic as set-cleared, sends whatever portion of the queue is clear (in order!) and removes those pics from the queue. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/09/26 17:43:45, Ami Fischman wrote: > How about: remove tracking of cleared/uncleared status from EVDA. Make > GVDA track whether assigned pic buffers have been cleared or not. If > GVDA::PictureReady is given a not-yet-marked-cleared picture and it's not > on the child thread, PostTaskAndReply to the child thread to clear, and > start queuing future PictureReady's. While queuing, GVDA::PR only PTAR's > for other buffers needing clearing, but doesn't pass the pictures to IPC. > The PTAR's Reply callback marks the pic as set-cleared, sends whatever > portion of the queue is clear (in order!) and removes those pics from the > queue. I used a similar way like your suggestion. The difference is that I still maintain cleared status in EVDA. If I do it in GVDA, |uncleared_textures_| will be accessed on IO thread in GVDA::PictureReady and on the child thread in GVDA::AssignPictureBuffers. It will need a lock and it's bad to hold a lock on IO thread.
Done. PTAL. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exyn... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/exyn... content/common/gpu/media/exynos_video_decode_accelerator.cc:1422: // reduce latency. On 2013/09/26 16:50:01, piman wrote: > So, if the main (child) thread is busy, this can mean the PictureReady can come > out-of-order to the client. > E.g. Picture N needs to be clear, we post task to the main thread, it waits in > the task queue because main thread is busy. Picture N+1 doesn't need it, we post > to the IO thread, it goes to the client right way. Main thread unblocks, we > SetLevelCleared on the picture N's texture, and send that to the client. > So the client receives N+1 before N. Is that ok? The code is updated to handle the case you mentioned. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:186: if (child_message_loop_->BelongsToCurrentThread()) On 2013/09/26 16:50:01, piman wrote: > nit: needs brackets per style guide. Done. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:188: return; On 2013/09/26 16:50:01, piman wrote: > Is there a way to DCHECK that we don't get here on the IO thread without the > texture having been set cleared? For example check the list that you keep? You'd > need a lock, but only in debug, so that would be ok I think. Done. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:455: for (i = 0; i < buffer_ids_.size(); i++) On 2013/09/26 16:50:01, piman wrote: > nit: needs brackets per style guide Done. Code was changed. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:464: texture_ids_[i]); On 2013/09/26 16:50:01, piman wrote: > Can you, instead, keep a scoped_refptr<TextureRef> when you first lookup the > texture in OnAssignPictureBuffers? > It is possible that by the time this executes, the original texture has been > deleted, and another one took its place on the same id. You don't want to touch > the new one. Done. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.cc:478: texture_ids_.erase(texture_ids_.begin() + i); On 2013/09/26 16:50:01, piman wrote: > On 2013/09/26 16:21:56, Ami Fischman wrote: > > Would require less book-keeping as a vector of pairs (buffer/texture ids) or a > > map from buffer id to texture id, FWIW. > > +1 on map, given the linear lookup above. Done. Changed to a map. https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/24762003/diff/1/content/common/gpu/media/gpu_... content/common/gpu/media/gpu_video_decode_accelerator.h:127: std::vector<uint32> texture_ids_; On 2013/09/26 16:21:56, Ami Fischman wrote: > comments for this and previous field could be clearer (s/set level cleared/been > cleared/) > > names for both fields would benefit from an uncleared_ prefix, IMO. Done.
https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/exynos_video_decode_accelerator.h:449: std::queue<std::pair<bool, media::Picture> > pending_picture_ready_; Doco what the bool is https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/gpu_video_decode_accelerator.cc:490: return true; NOTREACHED? https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/gpu_video_decode_accelerator.cc:490: return true; return false? (as it is, this function always returns true, so might as well return void) https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/gpu_video_decode_accelerator.h:125: // Protects |uncleared_texture_| when DCHECK is on. This is for debugging s/uncleared_texture_/uncleared_textures_/ https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/gpu_video_decode_accelerator.h:126: // only. We don't want to hold a lock on IO thread. You should explicate that the reason this is ok is that when DCHECK is off uncleared_textures_ is accessed from only the child thread. https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/gpu_video_decode_accelerator.h:127: base::Lock lock_; s/lock_/debug_uncleared_textures_lock_/
LGTM+nits https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/exynos_video_decode_accelerator.cc:2489: DCHECK(picture_clearing_count_ > 0); nit: DCHECK_GT https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/exynos_video_decode_accelerator.h:449: std::queue<std::pair<bool, media::Picture> > pending_picture_ready_; On 2013/09/30 18:03:03, Ami Fischman wrote: > Doco what the bool is nit: Better yet, make it a struct with explicit field names rather than "first" and "second"
All done. PTAL. https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/exynos_video_decode_accelerator.cc:2489: DCHECK(picture_clearing_count_ > 0); On 2013/09/30 21:49:13, piman wrote: > nit: DCHECK_GT Done. https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/exynos_video_decode_accelerator.h:449: std::queue<std::pair<bool, media::Picture> > pending_picture_ready_; On 2013/09/30 21:49:13, piman wrote: > On 2013/09/30 18:03:03, Ami Fischman wrote: > > Doco what the bool is > > nit: Better yet, make it a struct with explicit field names rather than "first" > and "second" Done. https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... File content/common/gpu/media/gpu_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/gpu_video_decode_accelerator.cc:490: return true; On 2013/09/30 18:03:03, Ami Fischman wrote: > return false? > (as it is, this function always returns true, so might as well return void) Done. Changed to return void. https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... File content/common/gpu/media/gpu_video_decode_accelerator.h (right): https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/gpu_video_decode_accelerator.h:125: // Protects |uncleared_texture_| when DCHECK is on. This is for debugging On 2013/09/30 18:03:03, Ami Fischman wrote: > s/uncleared_texture_/uncleared_textures_/ Done. https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/gpu_video_decode_accelerator.h:126: // only. We don't want to hold a lock on IO thread. On 2013/09/30 18:03:03, Ami Fischman wrote: > You should explicate that the reason this is ok is that when DCHECK is off > uncleared_textures_ is accessed from only the child thread. Done. https://codereview.chromium.org/24762003/diff/16001/content/common/gpu/media/... content/common/gpu/media/gpu_video_decode_accelerator.h:127: base::Lock lock_; On 2013/09/30 18:03:03, Ami Fischman wrote: > s/lock_/debug_uncleared_textures_lock_/ Done.
lgtm
LGTM
evda parts lgtm % nit, please make sure it works on resolution switching in DASH YT as well as raw resolution switch streams. https://codereview.chromium.org/24762003/diff/36001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://codereview.chromium.org/24762003/diff/36001/content/common/gpu/media/... content/common/gpu/media/exynos_video_decode_accelerator.h:184: bool cleared; // whether the texture has been cleared I guess everyone here is familiar with what "cleared" means, but I feel a little bit more explanation here would help, because it's not exactly "clear" ;) if you don't know about the texture manager.
https://codereview.chromium.org/24762003/diff/36001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://codereview.chromium.org/24762003/diff/36001/content/common/gpu/media/... content/common/gpu/media/exynos_video_decode_accelerator.h:184: bool cleared; // whether the texture has been cleared On 2013/10/01 07:45:34, posciak wrote: > I guess everyone here is familiar with what "cleared" means, but I feel a little > bit more explanation here would help, because it's not exactly "clear" ;) if you > don't know about the texture manager. Done. https://codereview.chromium.org/24762003/diff/50001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://codereview.chromium.org/24762003/diff/50001/content/common/gpu/media/... content/common/gpu/media/exynos_video_decode_accelerator.cc:2484: weak_this_)); Pawel. Thanks for the suggestion to test DASH. It caught a bug in the debug build. |weak_this_| should be accessed on the child thread, not decoding thread. I'll fix this.
Pawel. I modified EVDA after testing DASH. PTAL. - Use Unretained to send PictureReady. - Drain all PictureReady in FlushTask and ResetTask.
On 2013/10/02 06:26:38, wuchengli wrote: > Pawel. I modified EVDA after testing DASH. PTAL. > - Use Unretained to send PictureReady. > - Drain all PictureReady in FlushTask and ResetTask. It's not clear to me why we need those additional SendPictureReady calls in Flush/ResetTask. On Flush, we still need to dequeue all buffers from gsc, so that should call SendPictureReady anyway... On Reset, we are not expected to return any buffers at all anymore...
On 2013/10/02 09:29:23, posciak wrote: > On 2013/10/02 06:26:38, wuchengli wrote: > > Pawel. I modified EVDA after testing DASH. PTAL. > > - Use Unretained to send PictureReady. > > - Drain all PictureReady in FlushTask and ResetTask. > > It's not clear to me why we need those additional SendPictureReady calls in > Flush/ResetTask. > On Flush, we still need to dequeue all buffers from gsc, so that should call > SendPictureReady anyway... SendPictureReady will be only called if there is a new completed gsc output buffer. Will Flush guarantee that? > On Reset, we are not expected to return any buffers at all anymore... It should be OK to return buffers before NotifyResetDone is sent. Also, from EVDA perspective, those buffers are already returned to clients (that is, GscOutputRecord.at_client is true and decoder_frames_at_client_ increased). It's hard to reverse those steps.
PTAL.
On 2013/10/02 12:27:05, wuchengli wrote:
> PTAL.
lgtm after a thorough analysis on chat. Another solution would be to
Notify{Flush,Reset}Done() from PictureCleared if we had any ready pictures
pending at the time of {Flush,Reset}Task(). This would make the timing of Notify
clearer, to be always after the last PictureCleared, but would result in two
calling sites for Notifies (PC() and Flush/ResetTask), which is complicating
things as well.
Right now we depend on PictureReady posted on io to arrive before Notifies(),
based on the fact that posting PR() to Child will then result in posting them
to io so they will be after those posted directly to io. Otherwise we'd have to
switch to Notify() from PC().
Refactored SendPictureReady by kcwu's suggestion. Kuang-che. PTAL.
lgtm
Submitting. Thanks all!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/24762003/72001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/24762003/72001
Message was sent while issue was closed.
Change committed as 226492 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
