Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19151002:
Add switches: "target_fps" and "disable_rendering" to video_decode_accelerator_unittest. (Closed)
|
Created:
7 years, 5 months ago by Owen Lin Modified:
4 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd switches: "target_fps" and "disable_rendering" to video_decode_accelerator_unittest.
1. Add the switch "target_fps" to control the playing speed.
This is used to measure the frame drop rate and CPU usage.
2. Also add a switch "disable_rendering" to disable rendering.
This is used to measure just the decoding part.
On parrot, using test video: "test-25fps.h264" with target FPS: 60, the CPU
rate is about 12% and no dropped was frames.
With target FPS as 300, the CPU rate goes to 47.34% and 4/250 frames are
dropped.
BUG=250718
TEST=run the unittest on link and parrot
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215874
Patch Set 1 #
Total comments: 22
Patch Set 2 : address review comments #Patch Set 3 : initialize pending_rendering_ #Patch Set 4 : Add ThrottlingVDAClient to control fps #Patch Set 5 : address one more comment #
Total comments: 37
Patch Set 6 : address review comment #
Total comments: 8
Patch Set 7 : addressing review comments #Patch Set 8 : rebase and git cl format #Patch Set 9 : fix compiling errors on win_x64_rel #Messages
Total messages: 18 (0 generated)
PTAL. Thanks.
https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:196: CS_COMPLETE_RENDERING = 5, I'd call this RENDERED or RENDERING_DONE. IMHO this name is a bit ambiguous in whether rendering is to be completed or has been completed https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:272: float target_fps, Add doc above? https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:499: // frame dropped Actually, there might be more than one frame dropped here. I'm thinking that perhaps if we are past this frame's successor's delivery time, we should not render it at all either. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:510: ++pending_rendering_; There is a chance that when you loop in l.498, next_frame_delivered_time_ might become less than now, so the delayed task will not execute and your pending_rendering count will be off. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:517: This is render time, not frame delivery time, which was in PictureReady.
https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:92: float target_fps = 0; "target_fps" is too vague IMO. IIUC you want it to simulate the actions of a renderer that takes timestamps into account and buffers up frames that are delivered too early (and drops frames that arrive too late). If that's the case, then rendering_fps seems like a better name. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:196: CS_COMPLETE_RENDERING = 5, On 2013/07/15 08:07:06, posciak wrote: > I'd call this RENDERED or RENDERING_DONE. IMHO this name is a bit ambiguous in > whether rendering is to be completed or has been completed I think none of these names is good enough to use (because none of them tell me what this state is for). AFAICT from the uses of this value, it is never useful; there are no codepaths that set it without immediately setting CS_RESETTING, so there's nothing for the test to check for meaningfully (I see the added expectation, I just don't understand why it's useful; it's not possible to get to CS_RESETTING /without/ passing through this state, and the expectation of CS_RESETTING is already there). https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:352: base::TimeDelta frame_interval_; s/interval/duration/ https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:566: if (pending_rendering_ == 0) { Why do none of the other notifications test for this? I suspect that's a latent bug. I also suspect that a DelayingVDAClient might make sense, to encapsulate the concept of throttling delivery times. If you do that (i.e. wrap the GLRenderingVDAClient in a DelayingVDAClient) then I suspect you can make the CL much more locally-contained, while ensuring that it's easy to see whether you caught all the relevant codepaths (b/c ones where you ignore pending_rendering will look more obvious like blind pass-throughs). https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:569: SetState(CS_RESETTING); Duplicating this block here and at l.532 is a code-smell. I think the right thing to do is to fake a delay of delivery of NFD() until pending_rendering==0 (and drop l.532-536) https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:1091: content::target_fps = static_cast<float>(input_value); why not just use double instead of float and avoid the cast?
https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:92: float target_fps = 0; On 2013/07/15 18:54:14, Ami Fischman wrote: > "target_fps" is too vague IMO. > IIUC you want it to simulate the actions of a renderer that takes timestamps > into account and buffers up frames that are delivered too early (and drops > frames that arrive too late). > If that's the case, then rendering_fps seems like a better name. Done. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:196: CS_COMPLETE_RENDERING = 5, Are you suggesting removing this state ? My intention is to make it more clear that we are waiting for rendering. On 2013/07/15 18:54:14, Ami Fischman wrote: > On 2013/07/15 08:07:06, posciak wrote: > > I'd call this RENDERED or RENDERING_DONE. IMHO this name is a bit ambiguous in > > whether rendering is to be completed or has been completed > > I think none of these names is good enough to use (because none of them tell me > what this state is for). AFAICT from the uses of this value, it is never > useful; there are no codepaths that set it without immediately setting > CS_RESETTING, so there's nothing for the test to check for meaningfully (I see > the added expectation, I just don't understand why it's useful; it's not > possible to get to CS_RESETTING /without/ passing through this state, and the > expectation of CS_RESETTING is already there). https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:196: CS_COMPLETE_RENDERING = 5, On 2013/07/15 08:07:06, posciak wrote: > I'd call this RENDERED or RENDERING_DONE. IMHO this name is a bit ambiguous in > whether rendering is to be completed or has been completed Done. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:272: float target_fps, On 2013/07/15 08:07:06, posciak wrote: > Add doc above? Done. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:352: base::TimeDelta frame_interval_; On 2013/07/15 18:54:14, Ami Fischman wrote: > s/interval/duration/ Done. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:499: // frame dropped On 2013/07/15 08:07:06, posciak wrote: > Actually, there might be more than one frame dropped here. > > I'm thinking that perhaps if we are past this frame's successor's delivery time, > we should not render it at all either. Done. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:510: ++pending_rendering_; On 2013/07/15 08:07:06, posciak wrote: > There is a chance that when you loop in l.498, next_frame_delivered_time_ might > become less than now, so the delayed task will not execute and your > pending_rendering count will be off. Hi, I don't really get it. When we exit the loop in l.498, it is guaranteed that next_frame_delivered_time >= now. So, the delayed task will execute eventually. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:517: On 2013/07/15 08:07:06, posciak wrote: > This is render time, not frame delivery time, which was in PictureReady. You're right. It just makes it simple to write the autotest code. I will log the time in PictureReady and try to modified the code in autotest to measure the frame drop rate. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:566: if (pending_rendering_ == 0) { On 2013/07/15 18:54:14, Ami Fischman wrote: > Why do none of the other notifications test for this? > I suspect that's a latent bug. I don't get it. Can you be more specific ? Do you mean test for "pending_rendering_ == 0" ? We care about the pending_rendering_ only when we have flushed all the remaining pictures (i.e., state == CS_FLUSHED). And there are only two cases when we get the FlushDone notification: 1. There are some frames waiting for rendering ---> handled by l.532 2. There are no frames waiting for rendering ---> handled here > > I also suspect that a DelayingVDAClient might make sense, to encapsulate the > concept of throttling delivery times. If you do that (i.e. wrap the > GLRenderingVDAClient in a DelayingVDAClient) then I suspect you can make the CL > much more locally-contained, while ensuring that it's easy to see whether you > caught all the relevant codepaths (b/c ones where you ignore pending_rendering > will look more obvious like blind pass-throughs). If I understand correctly, you suggested me to add one more layer to delay the PictureReady callback. decoder_ --> DelayingVDAClient --delayed--> GLRenderingVDAClient It sounds really good. I did try to implement it. But I found it doesn't match our need. Powal suggested we skip rendering for those dropped frames. In that case, I will skip calling GLRenderingVDAClient::PictureReady. But it will lead to incorrect value of num_decoded_frames_. I don't have a good solution for that. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:569: SetState(CS_RESETTING); On 2013/07/15 18:54:14, Ami Fischman wrote: > Duplicating this block here and at l.532 is a code-smell. > I think the right thing to do is to fake a delay of delivery of NFD() until > pending_rendering==0 (and drop l.532-536) Do you suggest me the following: if (pending_rendering > 0) { PostDelayedTask(NotifyFlushDone, some_delay). } But it is also like a code-smell for the "some_delay". How can we decide the delay, 10ms? why? Especially when we can know the exact timing to do it. https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/vide... content/common/gpu/media/video_decode_accelerator_unittest.cc:1091: content::target_fps = static_cast<float>(input_value); On 2013/07/15 18:54:14, Ami Fischman wrote: > why not just use double instead of float and avoid the cast? Done.
> > https://codereview.chromium.**org/19151002/diff/1/content/** > common/gpu/media/video_decode_**accelerator_unittest.cc#**newcode196<https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/video_decode_accelerator_unittest.cc#newcode196> > content/common/gpu/media/**video_decode_accelerator_**unittest.cc:196: > CS_COMPLETE_RENDERING = 5, > Are you suggesting removing this state ? Yes. > My intention is to make it more > clear that we are waiting for rendering. But we're not waiting for rendering at that point; once CS_FLUSHED has hit all available pics have been rendered. > > > https://codereview.chromium.**org/19151002/diff/1/content/** > common/gpu/media/video_decode_**accelerator_unittest.cc#**newcode566<https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/video_decode_accelerator_unittest.cc#newcode566> > content/common/gpu/media/**video_decode_accelerator_**unittest.cc:566: if > (pending_rendering_ == 0) { > On 2013/07/15 18:54:14, Ami Fischman wrote: > >> Why do none of the other notifications test for this? >> I suspect that's a latent bug. > > I don't get it. Can you be more specific ? > Do you mean test for "pending_rendering_ == 0" ? > Yes. > We care about the pending_rendering_ only when we have flushed all the > remaining pictures (i.e., state == CS_FLUSHED). > And there are only two cases when we get the FlushDone notification: > 1. There are some frames waiting for rendering ---> handled by l.532 > 2. There are no frames waiting for rendering ---> handled here If I ask for 4 Decode()s and all 4 pics are decoded immediately (say) and then I Reset() while some of the pics are being held pending their timestamp arriving, then I will be NotifyResetDone() and then some more PictureReady's will trigger after the reset is done. This is an example of the bug I was describing. > > > > I also suspect that a DelayingVDAClient might make sense, to >> > encapsulate the > >> concept of throttling delivery times. If you do that (i.e. wrap the >> GLRenderingVDAClient in a DelayingVDAClient) then I suspect you can >> > make the CL > >> much more locally-contained, while ensuring that it's easy to see >> > whether you > >> caught all the relevant codepaths (b/c ones where you ignore >> > pending_rendering > >> will look more obvious like blind pass-throughs). >> > > If I understand correctly, you suggested me to add one more layer to > delay the PictureReady callback. > > decoder_ --> DelayingVDAClient --delayed--> GLRenderingVDAClient > Yes this is what I was describing. It sounds really good. I did try to implement it. But I found it doesn't > match our need. Powal suggested we skip rendering for those dropped > frames. In that case, I will skip calling > GLRenderingVDAClient::**PictureReady. But it will lead to incorrect value > of num_decoded_frames_. I don't have a good solution for that. Why not just ask the delaying client for num_decoded or num_dropped_frames? > https://codereview.chromium.**org/19151002/diff/1/content/** > common/gpu/media/video_decode_**accelerator_unittest.cc#**newcode569<https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/video_decode_accelerator_unittest.cc#newcode569> > content/common/gpu/media/**video_decode_accelerator_**unittest.cc:569: > SetState(CS_RESETTING); > On 2013/07/15 18:54:14, Ami Fischman wrote: > >> Duplicating this block here and at l.532 is a code-smell. >> I think the right thing to do is to fake a delay of delivery of NFD() >> > until > >> pending_rendering==0 (and drop l.532-536) > > Do you suggest me the following: > if (pending_rendering > 0) { > PostDelayedTask(**NotifyFlushDone, some_delay). > } > But it is also like a code-smell for the "some_delay". How can we decide > the delay, 10ms? why? Especially when we can know the exact timing to do > it. YOu can always track the latest time you deferred to (possibly instead of counting the # of pending frames) to post the NFD to just after that. > > https://codereview.chromium.**org/19151002/<https://codereview.chromium.org/1... >
Hi, Thank you for the comments. I have modified the code accordingly. But it seems there were some misunderstandings about the original code. So, please see my replies inline although most of them are obsolete. On Tue, Jul 16, 2013 at 11:27 PM, Ami Fischman <fischman@chromium.org>wrote: > > >> https://codereview.chromium.**org/19151002/diff/1/content/** >> common/gpu/media/video_decode_**accelerator_unittest.cc#**newcode196<https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/video_decode_accelerator_unittest.cc#newcode196> >> content/common/gpu/media/**video_decode_accelerator_**unittest.cc:196: >> CS_COMPLETE_RENDERING = 5, >> Are you suggesting removing this state ? > > > Yes. > > >> My intention is to make it more >> clear that we are waiting for rendering. > > > But we're not waiting for rendering at that point; once CS_FLUSHED has hit > all available pics have been rendered. > > No, when CS_FLUSHED hits, we just got all PictureReady()s and there could be some pending RenderPicture() function in the message loop. > >> >> https://codereview.chromium.**org/19151002/diff/1/content/** >> common/gpu/media/video_decode_**accelerator_unittest.cc#**newcode566<https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/video_decode_accelerator_unittest.cc#newcode566> >> content/common/gpu/media/**video_decode_accelerator_**unittest.cc:566: if >> (pending_rendering_ == 0) { >> On 2013/07/15 18:54:14, Ami Fischman wrote: >> >>> Why do none of the other notifications test for this? >>> I suspect that's a latent bug. >> >> I don't get it. Can you be more specific ? >> Do you mean test for "pending_rendering_ == 0" ? >> > > Yes. > > >> We care about the pending_rendering_ only when we have flushed all the >> remaining pictures (i.e., state == CS_FLUSHED). >> And there are only two cases when we get the FlushDone notification: >> 1. There are some frames waiting for rendering ---> handled by l.532 >> 2. There are no frames waiting for rendering ---> handled here > > > If I ask for 4 Decode()s and all 4 pics are decoded immediately (say) and > then I Reset() while some of the pics are being held pending their > timestamp arriving, then I will be NotifyResetDone() and then some more > PictureReady's will trigger after the reset is done. This is an example of > the bug I was describing. > I don't see any issue here. After NotifyResetDone(), what gets called is RenderPicture() not PictureReady(). >> >> >> I also suspect that a DelayingVDAClient might make sense, to >>> >> encapsulate the >> >>> concept of throttling delivery times. If you do that (i.e. wrap the >>> GLRenderingVDAClient in a DelayingVDAClient) then I suspect you can >>> >> make the CL >> >>> much more locally-contained, while ensuring that it's easy to see >>> >> whether you >> >>> caught all the relevant codepaths (b/c ones where you ignore >>> >> pending_rendering >> >>> will look more obvious like blind pass-throughs). >>> >> >> If I understand correctly, you suggested me to add one more layer to >> delay the PictureReady callback. >> >> decoder_ --> DelayingVDAClient --delayed--> GLRenderingVDAClient >> > > Yes this is what I was describing. > > It sounds really good. I did try to implement it. But I found it doesn't >> match our need. Powal suggested we skip rendering for those dropped >> frames. In that case, I will skip calling >> GLRenderingVDAClient::**PictureReady. But it will lead to incorrect value >> of num_decoded_frames_. I don't have a good solution for that. > > > Why not just ask the delaying client for num_decoded or num_dropped_frames? > Done. > > >> https://codereview.chromium.**org/19151002/diff/1/content/** >> common/gpu/media/video_decode_**accelerator_unittest.cc#**newcode569<https://codereview.chromium.org/19151002/diff/1/content/common/gpu/media/video_decode_accelerator_unittest.cc#newcode569> >> content/common/gpu/media/**video_decode_accelerator_**unittest.cc:569: >> SetState(CS_RESETTING); >> On 2013/07/15 18:54:14, Ami Fischman wrote: >> >>> Duplicating this block here and at l.532 is a code-smell. >>> I think the right thing to do is to fake a delay of delivery of NFD() >>> >> until >> >>> pending_rendering==0 (and drop l.532-536) >> >> Do you suggest me the following: >> if (pending_rendering > 0) { >> PostDelayedTask(**NotifyFlushDone, some_delay). >> } >> But it is also like a code-smell for the "some_delay". How can we decide >> the delay, 10ms? why? Especially when we can know the exact timing to do >> it. > > > YOu can always track the latest time you deferred to (possibly instead of > counting the # of pending frames) to post the NFD to just after that. > >> https://codereview.chromium.**org/19151002/<https://codereview.chromium.org/1... >> > > -- Owen Cheng-Ru Lin Google Taipei Software Engineer
PTAL. So many thanks. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:671: if (reset_after_frame_num_ != MID_STREAM_RESET) This is for the MidStreamReset test case. In the new code, we delay the "NotifyResetDone" until all PictureReady. While we are waiting for it, we still keep feeding data to the decoder. Eventually, it will call flush() and change the state to FLUSHING. This is not what we want. I think we should stop feeding data here.
https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:95: bool disable_rendering = false; This is not mentioned in the CL description. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:241: class ThrottlingVDAClient : public VideoDecodeAccelerator::Client { doco the point of this class. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:247: ThrottlingVDAClient(VideoDecodeAccelerator::Client*, missing param name https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:249: ReusePictureCB reuse_pic_cb); s/pic/picture/ https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:258: // Simple state changes. What's so "simple" about them? :) Generally chrome code docos overridden virtuals by grouping them all together (in the order of declaration in the base class) and commenting // FooInterface implementation. like https://code.google.com/p/chromium/codesearch#chromium/src/remoting/codec/vid... https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:276: void CallClientPictureReady(const media::Picture& picture); style: methods go above members https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:277: }; DISALLOW_IMPLICIT_CONSTRUCTORS https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:308: base::TimeTicks now = base::TimeTicks::Now(); indent is off here and elsewhere. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:312: last_frame_delivered_time_ += frame_duration_; This var name doesn't make sense. Is it supposed to embody "time of beginning of duration of the latest frame scheduled for rendering"? Because that's what I _think_ it's computing. But then the test at l.316 below is wrong, because if "now" falls in the middle of the frame's duration you wouldn't drop it, you'd still want to show it (for less than its scheduled duration if the next frame is delivered on time). Basically I think you want to rename some things and probably rejigger some other things in this method :) https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:360: if (pending_pictures_ > 0) { This delays reset for rendering, which is unlikely to mirror what a real VDA::Client does; real clients drop queued frames when they request a Reset(). I think you want to do the same here. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:393: // Use |suppress_rendering| to skip the rendering part, so it only decodes Rewrite this line to match the rest of the paragraph https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:636: ++num_decoded_frames_; Why did you move these two lines? https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:647: decoder_->AsWeakPtr(), picture.picture_buffer_id()), Should be gone after rebase? https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:671: if (reset_after_frame_num_ != MID_STREAM_RESET) On 2013/07/24 06:30:45, Owen Lin wrote: > This is for the MidStreamReset test case. > > In the new code, we delay the "NotifyResetDone" until all PictureReady. While we > are waiting for it, we still keep feeding data to the decoder. Eventually, it > will call flush() and change the state to FLUSHING. This is not what we want. I > think we should stop feeding data here. See comment above; I think you don't want this and instead want to change how TVC-based Resetting works. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:672: DecodeNextFragments(); AFAICT the point of this CL is to slow down decode to match a target rendering FPS, to evaluate the CPU power needed for a realistic decode (i.e. not a full-speed, non-rendering decode) and to ensure that the decoder can keep up with the desired frame-rate (i.e. that there are no dropped frames). IWBN if the CL description was updated to reflect the result of these goals on at least one platform. Something like: "using test video XXX with target FPS YYY CPU rate goes from xx to yy and dropped frames are at zzz". https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:681: decoder_->Reset(); Why do you no longer need the decoder_deleted() guard? https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:725: decoder_->ReusePictureBuffer(picture_buffer_id); Shouldn't this be counting dropped frames or something? (more useful would probably be to track max _dropped frames per second_, but if the total count of dropped frames is zero then that matters less) https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:934: content::disable_rendering; You dropped frame_delivery_log != NULL from the condition, but didn't update the comment. I'm not sure whether the right answer is to update the comment or to restore the condition. Can you choose one and explain your choice?
PTAL. Thanks. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:95: bool disable_rendering = false; On 2013/07/24 18:23:16, Ami Fischman wrote: > This is not mentioned in the CL description. Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:241: class ThrottlingVDAClient : public VideoDecodeAccelerator::Client { On 2013/07/24 18:23:16, Ami Fischman wrote: > doco the point of this class. Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:247: ThrottlingVDAClient(VideoDecodeAccelerator::Client*, On 2013/07/24 18:23:16, Ami Fischman wrote: > missing param name Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:249: ReusePictureCB reuse_pic_cb); On 2013/07/24 18:23:16, Ami Fischman wrote: > s/pic/picture/ Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:258: // Simple state changes. On 2013/07/24 18:23:16, Ami Fischman wrote: > What's so "simple" about them? :) I just copy the description from GLRenderingVDAClient. > Generally chrome code docos overridden virtuals by grouping them all together > (in the order of declaration in the base class) and commenting > // FooInterface implementation. > like > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/codec/vid... Thanks. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:276: void CallClientPictureReady(const media::Picture& picture); On 2013/07/24 18:23:16, Ami Fischman wrote: > style: methods go above members Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:277: }; On 2013/07/24 18:23:16, Ami Fischman wrote: > DISALLOW_IMPLICIT_CONSTRUCTORS Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:308: base::TimeTicks now = base::TimeTicks::Now(); On 2013/07/24 18:23:16, Ami Fischman wrote: > indent is off here and elsewhere. Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:312: last_frame_delivered_time_ += frame_duration_; On 2013/07/24 18:23:16, Ami Fischman wrote: > This var name doesn't make sense. > Is it supposed to embody "time of beginning of duration of the latest frame > scheduled for rendering"? Because that's what I _think_ it's computing. But > then the test at l.316 below is wrong, because if "now" falls in the middle of > the frame's duration you wouldn't drop it, you'd still want to show it (for less > than its scheduled duration if the next frame is delivered on time). > > Basically I think you want to rename some things and probably rejigger some > other things in this method :) The code has been changed to address the NotifyResetDone() issue you mentioned. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:360: if (pending_pictures_ > 0) { On 2013/07/24 18:23:16, Ami Fischman wrote: > This delays reset for rendering, which is unlikely to mirror what a real > VDA::Client does; real clients drop queued frames when they request a Reset(). > I think you want to do the same here. Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:393: // Use |suppress_rendering| to skip the rendering part, so it only decodes On 2013/07/24 18:23:16, Ami Fischman wrote: > Rewrite this line to match the rest of the paragraph Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:636: ++num_decoded_frames_; On 2013/07/24 18:23:16, Ami Fischman wrote: > Why did you move these two lines? Move them back. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:647: decoder_->AsWeakPtr(), picture.picture_buffer_id()), On 2013/07/24 18:23:16, Ami Fischman wrote: > Should be gone after rebase? Yes. It should be gone. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:671: if (reset_after_frame_num_ != MID_STREAM_RESET) On 2013/07/24 18:23:16, Ami Fischman wrote: > On 2013/07/24 06:30:45, Owen Lin wrote: > > This is for the MidStreamReset test case. > > > > In the new code, we delay the "NotifyResetDone" until all PictureReady. While > we > > are waiting for it, we still keep feeding data to the decoder. Eventually, it > > will call flush() and change the state to FLUSHING. This is not what we want. > I > > think we should stop feeding data here. > > See comment above; I think you don't want this and instead want to change how > TVC-based Resetting works. Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:672: DecodeNextFragments(); On 2013/07/24 18:23:16, Ami Fischman wrote: > AFAICT the point of this CL is to slow down decode to match a target rendering > FPS, to evaluate the CPU power needed for a realistic decode (i.e. not a > full-speed, non-rendering decode) and to ensure that the decoder can keep up > with the desired frame-rate (i.e. that there are no dropped frames). > > IWBN if the CL description was updated to reflect the result of these goals on > at least one platform. Something like: "using test video XXX with target FPS > YYY CPU rate goes from xx to yy and dropped frames are at zzz". Done. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:681: decoder_->Reset(); On 2013/07/24 18:23:16, Ami Fischman wrote: > Why do you no longer need the decoder_deleted() guard? Oops, reverted. I didn't notice that decoder would be deleted in SetState(). https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:725: decoder_->ReusePictureBuffer(picture_buffer_id); On 2013/07/24 18:23:16, Ami Fischman wrote: > Shouldn't this be counting dropped frames or something? > > (more useful would probably be to track max _dropped frames per second_, but if > the total count of dropped frames is zero then that matters less) I plan to derive the number of dropped frame from frame's delivery times. So, I don't need to test or log it here. https://codereview.chromium.org/19151002/diff/14001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:934: content::disable_rendering; On 2013/07/24 18:23:16, Ami Fischman wrote: > You dropped > frame_delivery_log != NULL > from the condition, but didn't update the comment. > > I'm not sure whether the right answer is to update the comment or to restore the > condition. Can you choose one and explain your choice? A new switch "--disable_rendering" was add in this change. So that we can control it when we have different type of test cases.
Did you forget to upload a new patchset?
On 2013/07/31 23:39:58, Ami Fischman wrote: > Did you forget to upload a new patchset? Indeed. But I did remember I typed the title for the patch set's ....
https://codereview.chromium.org/19151002/diff/21001/content/common/gpu/media/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/19151002/diff/21001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:33: #include "base/md5.h" 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/19151002/diff/21001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:94: double rendering_fps = 0; global vars should be prefixed with g_ to avoid conflicts and shadowing (applies to existing vars too) https://codereview.chromium.org/19151002/diff/21001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:375: if (next_frame_delivered_time_ + kFrameDelayTolerance < now) { Shouldn't kFrameDelayTolerance always be equal to the frame duration, i.e. 1/fps ? https://codereview.chromium.org/19151002/diff/21001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:418: reuse_picture_cb_.Run(pending_pictures_.front().picture_buffer_id()); indent is off here and elsewhere. why not give [git cl format] a try?
https://codereview.chromium.org/19151002/diff/21001/content/common/gpu/media/... File content/common/gpu/media/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/19151002/diff/21001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:33: #include "base/md5.h" On 2013/08/02 00:33:22, Ami Fischman wrote: > 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) I see. Sorry for the inconvenience. https://codereview.chromium.org/19151002/diff/21001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:94: double rendering_fps = 0; On 2013/08/02 00:33:22, Ami Fischman wrote: > global vars should be prefixed with g_ to avoid conflicts and shadowing > (applies to existing vars too) Done. https://codereview.chromium.org/19151002/diff/21001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:375: if (next_frame_delivered_time_ + kFrameDelayTolerance < now) { On 2013/08/02 00:33:22, Ami Fischman wrote: > Shouldn't kFrameDelayTolerance always be equal to the frame duration, i.e. 1/fps > ? Good point. Thanks. https://codereview.chromium.org/19151002/diff/21001/content/common/gpu/media/... content/common/gpu/media/video_decode_accelerator_unittest.cc:418: reuse_picture_cb_.Run(pending_pictures_.front().picture_buffer_id()); On 2013/08/02 00:33:22, Ami Fischman wrote: > indent is off here and elsewhere. > why not give [git cl format] a try? Done. I tried "git cl format" but it changed the code a lot. I think I should do that later.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/owenlin@chromium.org/19151002/31001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/owenlin@chromium.org/19151002/52001
Message was sent while issue was closed.
Change committed as 215874 |
