Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1862303002:
Delay actual flush and reset in AVDA (Closed)
|
Created:
4 years, 8 months ago by Tima Vaisburd Modified:
4 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelay actual flush and release in AVDA
Postpone the actual call to MediaCodec.flush() and .release()
to let the AVDA drain the MediaCodec. This seems necessary to
at least one VP8 file, otherwise .flash() and .release() hangs.
The CL attempts to generalize the codec draining mechanism
and uses it in 2 additional places: Reset() for Vp8 and
Destroy() for Vp8.
This CL also release all pending output buffer in Release()
and Destroy() for all codecs.
BUG=598963
Committed: https://crrev.com/e69b434c2d5c6fd5415a772cda5e5de2f140996c
Cr-Commit-Position: refs/heads/master@{#389273}
Patch Set 1 #Patch Set 2 : Drain decoder before both flush() and release() #Patch Set 3 : Uses drain with EOS instead of abitrary delay #
Total comments: 16
Patch Set 4 : Addressed comments #Patch Set 5 : Removed an empty line #
Total comments: 6
Patch Set 6 : More comments addressed #Patch Set 7 : Notifications are now set with BindToCurrentLoop() #
Total comments: 21
Patch Set 8 : Addressed comments, rebased #Patch Set 9 : Kept OUTPUT_FORMAT_CHANGED processing while draining, rebased #
Messages
Total messages: 43 (13 generated)
Description was changed from ========== Delay actual flush in AVDA - DO NOT LAND! this is an experiment Postpone the actual call to MediaCodec.flush() to let the AVDA drain the MediaCodec. This seems necessary to a resulution changing VP8 on Nexus 5, otherwise the flush() hangs. The chosen value of 100 ms delay is quite arbitrary. BUG=598963 ========== to ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL introduces yet another WAITING_FOR_DRAIN state during which the input to MediaCodec is blocked, but the output continues for 50ms and all valid output buffers that come out are released back to codec. In the case of Destroy() we release all pending output buffers before going into WAITING_FOR_DRAIN state. The chosen value of 50 ms delay is quite arbitrary. BUG=598963 ==========
Description was changed from ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL introduces yet another WAITING_FOR_DRAIN state during which the input to MediaCodec is blocked, but the output continues for 50ms and all valid output buffers that come out are released back to codec. In the case of Destroy() we release all pending output buffers before going into WAITING_FOR_DRAIN state. The chosen value of 50 ms delay is quite arbitrary. BUG=598963 ========== to ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL introduces yet another WAITING_FOR_DRAIN state during which the input to MediaCodec is blocked, but the output continues for 50ms and all valid output buffers that come out are released back to codec. In the case of Destroy() we release all pending output buffers before going into WAITING_FOR_DRAIN state. The chosen value of 50 ms delay is quite arbitrary. BUG=598963 ==========
Description was changed from ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL introduces yet another WAITING_FOR_DRAIN state during which the input to MediaCodec is blocked, but the output continues for 50ms and all valid output buffers that come out are released back to codec. In the case of Destroy() we release all pending output buffers before going into WAITING_FOR_DRAIN state. The chosen value of 50 ms delay is quite arbitrary. BUG=598963 ========== to ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL introduces yet another WAITING_FOR_DRAIN state during which the input to MediaCodec is blocked, but the output continues for 50ms and all valid output buffers that come out are released back to codec. In the case of Destroy() we release all pending output buffers before going into WAITING_FOR_DRAIN state. The chosen value of 50 ms delay is quite arbitrary. BUG=598963 ==========
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
FWIW, you can't delay destroy like this since it deletes |this| -- which in your case will immediately invalidate the weakptr without running the destroy task. https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... Some other notes: - If we got his route, we'll want to make sure its only done for vp8.
Thank you for taking a look! > - If we got his route, we'll want to make sure its only done for vp8. I still hope we can avoid these kind of things altogether. And your idea to drain with EOS seems more robust. > FWIW, you can't delay destroy like this since it deletes |this| -- which in your > case will immediately invalidate the weakptr without running the destroy task. Sorry, I did not quite understand - I invalidate the weak pointers in the ActualDestroy() ? As far as I get releasing the smart pointer calls Destroy() which postpones ActualDestroy() which, in turn, will |delete this| (but there is a preceding explicit call InvalidateWeakPtrs()). https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_...
On 2016/04/11 at 22:02:46, timav wrote: > Thank you for taking a look! > > > - If we got his route, we'll want to make sure its only done for vp8. > > I still hope we can avoid these kind of things altogether. And your idea to drain with EOS seems more robust. > > > FWIW, you can't delay destroy like this since it deletes |this| -- which in your > > case will immediately invalidate the weakptr without running the destroy task. > > Sorry, I did not quite understand - I invalidate the weak pointers in the ActualDestroy() ? > As far as I get releasing the smart pointer calls Destroy() which postpones ActualDestroy() which, > in turn, will |delete this| (but there is a preceding explicit call InvalidateWeakPtrs()). > > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... Oh right, I forgot that Destroy() self-deletes, I thought the host was doing the deletion, in this case that's fine -- there's no problem. Sorry for the noise!
Description was changed from ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL introduces yet another WAITING_FOR_DRAIN state during which the input to MediaCodec is blocked, but the output continues for 50ms and all valid output buffers that come out are released back to codec. In the case of Destroy() we release all pending output buffers before going into WAITING_FOR_DRAIN state. The chosen value of 50 ms delay is quite arbitrary. BUG=598963 ========== to ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL attempts to generalize the codec draining mechanism and uses it in 2 additional places: Reset() for Vp8 and Destroy() for Vp8. This CL also release all pending output buffer in Release() and Destroy() for all codecs. BUG=598963 ==========
timav@chromium.org changed reviewers: + liberato@chromium.org, watk@chromium.org
Hi, this is the next attempt at the Vp8 hanging bug. I tried to follow Dale's advice to use the existing draining mechanism, but making the proper codec drain seems not easy. Please take a critical look.
On 2016/04/16 02:09:58, Tima Vaisburd wrote: > Hi, this is the next attempt at the Vp8 hanging bug. > I tried to follow Dale's advice to use the existing draining mechanism, but > making the proper codec drain seems not easy. > Please take a critical look. I can't seem to comment on individual lines. Sorry for that. This isn't the most thorough review, just high level stuff. Sorry about that, too :) Why is drain type not part of |state_|? Avda::732: why not release the output buffer if draining for destroy too? Oh, I just got to the fn definition . Maybe IsDrainingForReset is a confusing name. IsDrainingAndIgnoringOutput? Don't know. But since "for reset" is a drain type, definitely not that :) 727: the drain completed callback can destroy |this|. Please make this clear at call sites. 1035: why would the drain completed cb be null?
On 2016/04/18 08:56:04, liberato-ooo wrote: > On 2016/04/16 02:09:58, Tima Vaisburd wrote: > > Hi, this is the next attempt at the Vp8 hanging bug. > > I tried to follow Dale's advice to use the existing draining mechanism, but > > making the proper codec drain seems not easy. > > Please take a critical look. > > I can't seem to comment on individual lines. Sorry for that. This isn't the most > thorough review, just high level stuff. Sorry about that, too :) > > Why is drain type not part of |state_|? > > Avda::732: why not release the output buffer if draining for destroy too? Oh, I > just got to the fn definition . Maybe IsDrainingForReset is a confusing name. > IsDrainingAndIgnoringOutput? Don't know. But since "for reset" is a drain type, > definitely not that :) > > 727: the drain completed callback can destroy |this|. Please make this clear at > call sites. > > 1035: why would the drain completed cb be null? For "deleted this": can you post the cb rather than call it?
Nice. I agree with Frank that the drain type should probably be merged into state. Because once you're draining you don't stop draining until you move to an error state or finish. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:651: // Do not post an error if we are draining for reset. Instead, try Remove "try" since we are unconditionally running it. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1020: void AndroidVideoDecodeAccelerator::RequestDrain( IMO RequestDrain makes it sound like there's a chance the request will be rejected. What about DrainCodec? Or BeginDrainingCodec. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1022: base::Closure drain_completed_cb) { I'm wondering if it would be clearer to structure it like this: you have a OnCodecDrained which gets called whenever we dequeue EOS. It switches on drain_type to call Notify{Flush,Reset}Done, ActualDestroy and ResetCodecState as required. It can also reset the drain_type back to NONE. I can't tell if it's better without writing the code, but maybe you already tried? https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1031: DecodeBuffer(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0)); If there's an EOS already queued for a DRAIN_FOR_FLUSH, for example, and this is DRAIN_FOR_DESTROY, should we queue another EOS? It seems like we may as well wait for the one already queued. It probably doesn't matter but worth testing. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1040: base::Closure notification_cb) { I would be inclined to name this consistently with drain_completed_cb even if it is a bit long. Maybe done_cb if you want to make it shorter? https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1124: base::Closure notification_cb = media::BindToCurrentLoop( I don't think you need BindToCurrentLoop because this is all single threaded right? https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1142: nit: Remove empty line to keep the LOG and thread DCHECK as out of the way as possible. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.h:385: // come while prior is still being processed. It might be worth mentioning that we need to be able to distinguish flush drains from reset/destroy drains because we release the codec buffers immediately for the latter case.
On 2016/04/18 21:12:19, watk wrote: > Nice. I agree with Frank that the drain type should probably be merged into > state. Because once you're draining you don't stop draining until you move to an > error state or finish. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > File content/common/gpu/media/android_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:651: // Do not post > an error if we are draining for reset. Instead, try > Remove "try" since we are unconditionally running it. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1020: void > AndroidVideoDecodeAccelerator::RequestDrain( > IMO RequestDrain makes it sound like there's a chance the request will be > rejected. What about DrainCodec? Or BeginDrainingCodec. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1022: base::Closure > drain_completed_cb) { > I'm wondering if it would be clearer to structure it like this: you have a > OnCodecDrained which gets called whenever we dequeue EOS. It switches on > drain_type to call Notify{Flush,Reset}Done, ActualDestroy and ResetCodecState as > required. It can also reset the drain_type back to NONE. > > I can't tell if it's better without writing the code, but maybe you already > tried? > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1031: > DecodeBuffer(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0)); > If there's an EOS already queued for a DRAIN_FOR_FLUSH, for example, and this is > DRAIN_FOR_DESTROY, should we queue another EOS? It seems like we may as well > wait for the one already queued. It probably doesn't matter but worth testing. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1040: base::Closure > notification_cb) { > I would be inclined to name this consistently with drain_completed_cb even if it > is a bit long. Maybe done_cb if you want to make it shorter? > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1124: base::Closure > notification_cb = media::BindToCurrentLoop( > I don't think you need BindToCurrentLoop because this is all single threaded > right? > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1142: > nit: Remove empty line to keep the LOG and thread DCHECK as out of the way as > possible. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > File content/common/gpu/media/android_video_decode_accelerator.h (right): > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.h:385: // come while > prior is still being processed. > It might be worth mentioning that we need to be able to distinguish flush drains > from reset/destroy drains because we release the codec buffers immediately for > the latter case. I have two other requests. Would you mind updating the media codec loop document to include whatever parts of this new flush behavior that you think will affect MCL (most of it, probably)? I think that you have edit permission already. Second, how much latency does this introduce? Still haven't done line by line review since I can't add comments on this iPad. :( Thanks -fl
liberato> Why is drain type not part of |state_|? Could you elaborate? I had the |state_| set to a new DRAINING_FOR_RESET_OR_DESTROY state instead of the method IsDrainingForResetOrDestroy(), do you mean that? Or, how to combine? I thought it would be simpler in the current way. When setting the state we need to be careful not to erase the prior one. I renamed the IsDrainingForReset() to IsDrainingForResetOrDestroy(), but I'm happy to accept any other name. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:651: // Do not post an error if we are draining for reset. Instead, try On 2016/04/18 21:12:18, watk wrote: > Remove "try" since we are unconditionally running it. Done. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1020: void AndroidVideoDecodeAccelerator::RequestDrain( On 2016/04/18 21:12:18, watk wrote: > IMO RequestDrain makes it sound like there's a chance the request will be > rejected. What about DrainCodec? Or BeginDrainingCodec. Renamed to StartCodecDrain(). I have to strong preference, can do any if you still like yours better. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1022: base::Closure drain_completed_cb) { On 2016/04/18 21:12:18, watk wrote: > I'm wondering if it would be clearer to structure it like this: you have a > OnCodecDrained which gets called whenever we dequeue EOS. It switches on > drain_type to call Notify{Flush,Reset}Done, ActualDestroy and ResetCodecState as > required. It can also reset the drain_type back to NONE. > > I can't tell if it's better without writing the code, but maybe you already > tried? Yes, thank you! I tried now, and I think this way is better. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1031: DecodeBuffer(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0)); On 2016/04/18 21:12:18, watk wrote: > If there's an EOS already queued for a DRAIN_FOR_FLUSH, for example, and this is > DRAIN_FOR_DESTROY, should we queue another EOS? It seems like we may as well > wait for the one already queued. It probably doesn't matter but worth testing. Done. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1040: base::Closure notification_cb) { On 2016/04/18 21:12:18, watk wrote: > I would be inclined to name this consistently with drain_completed_cb even if it > is a bit long. Maybe done_cb if you want to make it shorter? Done. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1124: base::Closure notification_cb = media::BindToCurrentLoop( On 2016/04/18 21:12:18, watk wrote: > I don't think you need BindToCurrentLoop because this is all single threaded > right? Removed BindToCurrentLoop. The original code always posted the task, I'm not quite sure why though. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1142: On 2016/04/18 21:12:18, watk wrote: > nit: Remove empty line to keep the LOG and thread DCHECK as out of the way as > possible. Done. https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.h:385: // come while prior is still being processed. On 2016/04/18 21:12:18, watk wrote: > It might be worth mentioning that we need to be able to distinguish flush drains > from reset/destroy drains because we release the codec buffers immediately for > the latter case. I referred to IsDrainingForresetOrDestroy() where I updated the description.
On 2016/04/20 01:21:19, liberato-ooo wrote: > I have two other requests. Would you mind updating the media codec loop document > to include whatever parts of this new flush behavior that you think will affect > MCL (most of it, probably)? I think that you have edit permission already. > > Second, how much latency does this introduce? > Thank you for review! These requests are not done yet. Unfortunately, the drain affects the MCL :( The latency was not noticeable because we remove all pending buffers and left with the internal MediaCodec capacity, which in that particular case was 3 buffers, but it was just one observation. Please see the new code though, maybe you'll find more stuff. Thank you!
https://codereview.chromium.org/1862303002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1862303002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1007: void AndroidVideoDecodeAccelerator::StartCodecDrain(DrainType drain_type) { Since we have thread checkers everywhere else, you may as well put one here for consistency. https://codereview.chromium.org/1862303002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1026: void AndroidVideoDecodeAccelerator::OnDrainCompleted() { I like it! Thread checker here too. https://codereview.chromium.org/1862303002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1122: nit: no blank line
https://codereview.chromium.org/1862303002/diff/80001/content/common/gpu/medi... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1862303002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1007: void AndroidVideoDecodeAccelerator::StartCodecDrain(DrainType drain_type) { On 2016/04/20 18:13:45, watk wrote: > Since we have thread checkers everywhere else, you may as well put one here for > consistency. Done. https://codereview.chromium.org/1862303002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1026: void AndroidVideoDecodeAccelerator::OnDrainCompleted() { On 2016/04/20 18:13:44, watk wrote: > I like it! > > Thread checker here too. Done. https://codereview.chromium.org/1862303002/diff/80001/content/common/gpu/medi... content/common/gpu/media/android_video_decode_accelerator.cc:1122: On 2016/04/20 18:13:45, watk wrote: > nit: no blank line Done.
watk@> I don't think you need BindToCurrentLoop because this is all single threaded right? I had to restore BindToCurrentLoop() because NotifyPictureReady() is posted https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... Otherwise NotifyResetDone() might go to the renderer before a prior NotifyPictureReady() and the latter asserts since in cannot find a buffer id. On the other hand, do you know why do we post all notifications, it looks like they end by a Send(). liberato@> Latency? I tried N5 (hardware codec) and N7v2 (software codec). The typical time between Reset() and OnDrainCompleted was 15ms. With hardware codec (N5) and the file that changes the resolution I observed peaks of 100ms latency, I think it happens when we were trying to flush the frame that was causing a resolution change.
On 2016/04/20 23:39:42, Tima Vaisburd wrote: > Otherwise NotifyResetDone() might go to the renderer before > a prior NotifyPictureReady() and the latter asserts The GpuVideoDecoder asserts.
On 2016/04/20 23:39:42, Tima Vaisburd wrote: > watk@> I don't think you need BindToCurrentLoop because this is all single > threaded right? > > I had to restore BindToCurrentLoop() because NotifyPictureReady() is posted > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > Otherwise NotifyResetDone() might go to the renderer before > a prior NotifyPictureReady() and the latter asserts since in cannot find a > buffer id. > > On the other hand, do you know why do we post all notifications, it looks like > they end by a Send(). That's a shame.. I've been meaning to get rid the PostTasks we do to send messages to the client. It's never been clear why they were posted instead of called directly. I double checked that it's super cheap to Send from the main thread, so it's not a performance thing. We could do the BindToCurrentLoop thing for now and clean it up later.
On 2016/04/20 23:53:24, watk wrote: > We could do the BindToCurrentLoop thing for now and clean it up later. Good, thank you. Is there anything else I should do on this CL or let's wait for Frank and Dale's reviews?
On 2016/04/20 23:56:27, Tima Vaisburd wrote: > On 2016/04/20 23:53:24, watk wrote: > > We could do the BindToCurrentLoop thing for now and clean it up later. > > Good, thank you. Is there anything else I should do on this CL or let's wait for > Frank and Dale's reviews? This lgtm! Waiting for Frank's non-ipad review sounds good.
https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:857: if (client_) { Why this? https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1014: DCHECK(drain_type != DRAIN_TYPE_NONE); DCHECK_NE https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1015: DCHECK(drain_type_ == DRAIN_TYPE_NONE || drain_type == DRAIN_FOR_DESTROY); add << drain_type_ print so this DCHECK will print useful info. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1017: const bool enqueue_eos = (drain_type_ == DRAIN_TYPE_NONE); No unnecessary parens. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1087: const bool did_codec_error_happen = (state_ == ERROR); No unnecessary parens. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1145: DCHECK(strategy_); Don't think this can every be null? https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1146: strategy_->ReleaseCodecBuffers(output_picture_buffers_); Did we decide to do this for all codecs and not just VP8? https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1177: client_ = nullptr; Despite clearing this, none of the nullptr checks on it should be necessary right? Did you find that these methods were actually getting invoked? https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.h:305: // of Reset() or Destroy() VP8 workaround. (http://598963). We won't display http://crbug.com/598963 https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.h:319: void ResetCodecState(base::Closure done_cb); const&
Just the explanations, will do fixes later. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:857: if (client_) { On 2016/04/21 00:16:07, DaleCurtis wrote: > Why this? See below. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1146: strategy_->ReleaseCodecBuffers(output_picture_buffers_); On 2016/04/21 00:16:07, DaleCurtis wrote: > Did we decide to do this for all codecs and not just VP8? No, we have not, but I went ahead myself because I think this is the right thing to do in general (in other words I do not trust MediaCodec implementations that much) and pretty cheap. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1177: client_ = nullptr; On 2016/04/21 00:16:07, DaleCurtis wrote: > Despite clearing this, none of the nullptr checks on it should be necessary > right? Did you find that these methods were actually getting invoked? Yes, I think I did. Before this change I observed crashes which could be explained as invalid |client_| pointers in Notify... methods. I believe that the |client_| is invalid at the time of Destroy(). Before this CL a weak pointer invalidation was enough to prevent them, but now machinery keeps running until we drain.
Seems fine then after other nits are fixed. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1177: client_ = nullptr; On 2016/04/21 at 00:28:45, Tima Vaisburd wrote: > On 2016/04/21 00:16:07, DaleCurtis wrote: > > Despite clearing this, none of the nullptr checks on it should be necessary > > right? Did you find that these methods were actually getting invoked? > > Yes, I think I did. Before this change I observed crashes which could be explained as invalid |client_| pointers in Notify... methods. I believe that the |client_| is invalid at the time of Destroy(). Before this CL a weak pointer invalidation was enough to prevent them, but now machinery keeps running until we drain. I see. It's risky to invalidate the weakptrs and cross your fingers, so this sounds better.
https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1014: DCHECK(drain_type != DRAIN_TYPE_NONE); On 2016/04/21 00:16:07, DaleCurtis wrote: > DCHECK_NE Done. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1015: DCHECK(drain_type_ == DRAIN_TYPE_NONE || drain_type == DRAIN_FOR_DESTROY); On 2016/04/21 00:16:07, DaleCurtis wrote: > add << drain_type_ print so this DCHECK will print useful info. Done. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1017: const bool enqueue_eos = (drain_type_ == DRAIN_TYPE_NONE); On 2016/04/21 00:16:07, DaleCurtis wrote: > No unnecessary parens. Done. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1087: const bool did_codec_error_happen = (state_ == ERROR); On 2016/04/21 00:16:07, DaleCurtis wrote: > No unnecessary parens. Done. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:1145: DCHECK(strategy_); On 2016/04/21 00:16:07, DaleCurtis wrote: > Don't think this can every be null? As far as I understand |strategy_| can be null only before initialization. There is another such DCHECK on l.1252 https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.h (right): https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.h:305: // of Reset() or Destroy() VP8 workaround. (http://598963). We won't display On 2016/04/21 00:16:07, DaleCurtis wrote: > http://crbug.com/598963 Done. https://codereview.chromium.org/1862303002/diff/120001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.h:319: void ResetCodecState(base::Closure done_cb); On 2016/04/21 00:16:07, DaleCurtis wrote: > const& Done.
lgtm
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org Link to the patchset: https://codereview.chromium.org/1862303002/#ps140001 (title: "Addressed comments, rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862303002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862303002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Hey! Our tests caught bugs :)
On 2016/04/22 at 00:34:35, DaleCurtis wrote: > Hey! Our tests caught bugs :) Also, sorry you'll have to rebase if https://codereview.chromium.org/1910063005 lands first.
On 2016/04/20 01:54:19, Tima Vaisburd wrote: > liberato> Why is drain type not part of |state_|? > > Could you elaborate? I had the |state_| set to a new > DRAINING_FOR_RESET_OR_DESTROY state instead of the method > IsDrainingForResetOrDestroy(), do you mean that? Or, how to combine? > > I thought it would be simpler in the current way. When setting the state we need > to be careful not to erase the prior one. > > I renamed the IsDrainingForReset() to IsDrainingForResetOrDestroy(), but I'm > happy to accept any other name. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > File content/common/gpu/media/android_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:651: // Do not post > an error if we are draining for reset. Instead, try > On 2016/04/18 21:12:18, watk wrote: > > Remove "try" since we are unconditionally running it. > > Done. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1020: void > AndroidVideoDecodeAccelerator::RequestDrain( > On 2016/04/18 21:12:18, watk wrote: > > IMO RequestDrain makes it sound like there's a chance the request will be > > rejected. What about DrainCodec? Or BeginDrainingCodec. > > Renamed to StartCodecDrain(). I have to strong preference, can do any if you > still like yours better. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1022: base::Closure > drain_completed_cb) { > On 2016/04/18 21:12:18, watk wrote: > > I'm wondering if it would be clearer to structure it like this: you have a > > OnCodecDrained which gets called whenever we dequeue EOS. It switches on > > drain_type to call Notify{Flush,Reset}Done, ActualDestroy and ResetCodecState > as > > required. It can also reset the drain_type back to NONE. > > > > I can't tell if it's better without writing the code, but maybe you already > > tried? > > Yes, thank you! I tried now, and I think this way is better. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1031: > DecodeBuffer(media::BitstreamBuffer(-1, base::SharedMemoryHandle(), 0)); > On 2016/04/18 21:12:18, watk wrote: > > If there's an EOS already queued for a DRAIN_FOR_FLUSH, for example, and this > is > > DRAIN_FOR_DESTROY, should we queue another EOS? It seems like we may as well > > wait for the one already queued. It probably doesn't matter but worth testing. > > Done. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1040: base::Closure > notification_cb) { > On 2016/04/18 21:12:18, watk wrote: > > I would be inclined to name this consistently with drain_completed_cb even if > it > > is a bit long. Maybe done_cb if you want to make it shorter? > > Done. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1124: base::Closure > notification_cb = media::BindToCurrentLoop( > On 2016/04/18 21:12:18, watk wrote: > > I don't think you need BindToCurrentLoop because this is all single threaded > > right? > > Removed BindToCurrentLoop. The original code always posted the task, I'm not > quite sure why though. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.cc:1142: > On 2016/04/18 21:12:18, watk wrote: > > nit: Remove empty line to keep the LOG and thread DCHECK as out of the way as > > possible. > > Done. > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > File content/common/gpu/media/android_video_decode_accelerator.h (right): > > https://codereview.chromium.org/1862303002/diff/40001/content/common/gpu/medi... > content/common/gpu/media/android_video_decode_accelerator.h:385: // come while > prior is still being processed. > On 2016/04/18 21:12:18, watk wrote: > > It might be worth mentioning that we need to be able to distinguish flush > drains > > from reset/destroy drains because we release the codec buffers immediately for > > the latter case. > > I referred to IsDrainingForresetOrDestroy() where I updated the description. |state_|: when is there ambiguity about the state of the codec after a drain? if we drain while waiting for a key or something? otherwise, it seems like we always end up either in OKAY or the vda is destroyed. either way, lgtm.
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, dalecurtis@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1862303002/#ps160001 (title: "Kept OUTPUT_FORMAT_CHANGED processing while draining, rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862303002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862303002/160001
Message was sent while issue was closed.
Description was changed from ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL attempts to generalize the codec draining mechanism and uses it in 2 additional places: Reset() for Vp8 and Destroy() for Vp8. This CL also release all pending output buffer in Release() and Destroy() for all codecs. BUG=598963 ========== to ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL attempts to generalize the codec draining mechanism and uses it in 2 additional places: Reset() for Vp8 and Destroy() for Vp8. This CL also release all pending output buffer in Release() and Destroy() for all codecs. BUG=598963 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL attempts to generalize the codec draining mechanism and uses it in 2 additional places: Reset() for Vp8 and Destroy() for Vp8. This CL also release all pending output buffer in Release() and Destroy() for all codecs. BUG=598963 ========== to ========== Delay actual flush and release in AVDA Postpone the actual call to MediaCodec.flush() and .release() to let the AVDA drain the MediaCodec. This seems necessary to at least one VP8 file, otherwise .flash() and .release() hangs. The CL attempts to generalize the codec draining mechanism and uses it in 2 additional places: Reset() for Vp8 and Destroy() for Vp8. This CL also release all pending output buffer in Release() and Destroy() for all codecs. BUG=598963 Committed: https://crrev.com/e69b434c2d5c6fd5415a772cda5e5de2f140996c Cr-Commit-Position: refs/heads/master@{#389273} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e69b434c2d5c6fd5415a772cda5e5de2f140996c Cr-Commit-Position: refs/heads/master@{#389273} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
