Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2000833003:
Don't reset the codec state for a flush; this kills the frames. (Closed)
|
Created:
4 years, 7 months ago by DaleCurtis Modified:
4 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_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. |
DescriptionDon't reset the codec state for a flush; this kills the frames.
Causing a reset after flush completes will invalidate all the
vended frames, don't do this. Instead only reset when a reset
should actually occur.
Instead, since this reset is required on JellyBean devices, we
issue the reset upon receipt of the next Decode() call. This
allows the more frequent EOS cases to not trash the last frame
on these devices will config changes will work as before.
A followup CL will move the texture matrix to the shared state
so that we don't accidentally apply the wrong matrix after the
reset case.
BUG=613608
TEST=no more random blue frames.
Committed: https://crrev.com/eb4a8d89c1c1c3ed4b74e2afdee7ccd8dd6558bc
Cr-Commit-Position: refs/heads/master@{#395995}
Patch Set 1 #Patch Set 2 : Defer reset. #
Total comments: 5
Patch Set 3 : Fix matrix and flush. #
Total comments: 4
Patch Set 4 : Fix comment. Add dcheck. #
Dependent Patchsets: Messages
Total messages: 29 (9 generated)
dalecurtis@chromium.org changed reviewers: + liberato@chromium.org, timav@chromium.org
watk@chromium.org changed reviewers: + watk@chromium.org
Just for the record, this was added for MSE config change EOS's https://codereview.chromium.org/1560983002 I'm not sure what the problem with not doing it is though. I have a feeling it was that playback hitches or something.
On 2016/05/20 20:56:30, watk wrote: > Just for the record, this was added for MSE config change EOS's > https://codereview.chromium.org/1560983002 > > I'm not sure what the problem with not doing it is though. I have a feeling it > was that playback hitches or something. i'm confused now. i remember that we weren't getting resolution changes, but i also remember that (later) we discovered that we didn't need adaptive resolution at all,and MediaCodec got it right. i think we need to try the mse test clips chris was using on the original CL, to see if this regresses.
Description was changed from ========== Don't reset the codec state for a flush; this kills the frames. Causing a reset after flush completes will invalidate all the vended frames, don't do this. Instead only reset when a reset should actually occur. A followup CL will move the texture matrix to the shared state so that we don't accidentally apply the wrong matrix after the reset case. BUG=613608 TEST=no more random blue frames. ========== to ========== Don't reset the codec state for a flush; this kills the frames. Causing a reset after flush completes will invalidate all the vended frames, don't do this. Instead only reset when a reset should actually occur. A followup CL will move the texture matrix to the shared state so that we don't accidentally apply the wrong matrix after the reset case. BUG=613608 TEST=no more random blue frames. ==========
liberato@chromium.org changed reviewers: + chcunningham@chromium.org
On 2016/05/20 21:03:12, liberato wrote: > On 2016/05/20 20:56:30, watk wrote: > > Just for the record, this was added for MSE config change EOS's > > https://codereview.chromium.org/1560983002 > > > > I'm not sure what the problem with not doing it is though. I have a feeling it > > was that playback hitches or something. > > i'm confused now. i remember that we weren't getting resolution changes, but i > also remember that (later) we discovered that we didn't need adaptive resolution > at all,and MediaCodec got it right. > > i think we need to try the mse test clips chris was using on the original CL, to > see if this regresses. As far as I understand MediaCodec can properly process the resolution change only starting with K (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...) so as long as we support JellyBean we need to (1) stop enqueueing frames, (2) drain the codec and (3) reconfigure, which means calling ConfigureMediaCodec(A)synchronously(). So I'd like to ask: (1) Do we still have MediaCodec recreated for resolution change in MSE for JellyBean? (2) After the drain is completed the flush should be a no-op, what are these "vended frames"?
Description was changed from ========== Don't reset the codec state for a flush; this kills the frames. Causing a reset after flush completes will invalidate all the vended frames, don't do this. Instead only reset when a reset should actually occur. A followup CL will move the texture matrix to the shared state so that we don't accidentally apply the wrong matrix after the reset case. BUG=613608 TEST=no more random blue frames. ========== to ========== Don't reset the codec state for a flush; this kills the frames. Causing a reset after flush completes will invalidate all the vended frames, don't do this. Instead only reset when a reset should actually occur. Instead, since this reset is required on JellyBean devices, we issue the reset upon receipt of the next Decode() call. This allows the more frequent EOS cases to not trash the last frame on these devices will config changes will work as before. A followup CL will move the texture matrix to the shared state so that we don't accidentally apply the wrong matrix after the reset case. BUG=613608 TEST=no more random blue frames. ==========
Good catch Tima, I had forgotten about that. Instead I've made it such that flush type drains will defer the reset on API < 18, and do nothing on newer devices.
And oh, vended frames refers to the frames output just prior to the drain completing. When CodecChanged() is called, it unbacks all these frames, causing playback issues.
> i'm confused now. i remember that we weren't getting resolution changes, but > i also remember that (later) we discovered that we didn't need adaptive > resolution at all,and MediaCodec got it right. I think you're memory is correct - we weren't getting resolution changes. IIRC this was initially due to the RETURN_ON_FAILURE you see on line 390 here: https://codereview.chromium.org/1560983002/diff/60001/content/common/gpu/medi... Calling ResetCodecState avoided that failure because the OUTPUT_FORMAT_CHANGED would then be emitted after we had dismissed picture buffers. Obviously ResetCodecState also does a flush (or full teardown for api < 18). I don't recall exactly how bad things were if you failed to do this flush, but I would have expected maybe you may not get OUTPUT_FORMAT_CHANGED or you may get some error. I don't think I was part of the discussion when you later discovered you didn't need adaptive resolution at all - can you give more details? You mean you don't call MediaCodec.flush and things work fine? How far back (api levels) does this work? Its surprising since the android docs state a flush is required. > i think we need to try the mse test clips chris was using on the original CL, > to see if this regresses. run-webkit-tests isn't working with GN at the moment. :D Please do! Build the layout tests with GN and run: mediasource-config-change-mp4-v-framesize.html. The run-webkit-tests script is busted, so use the "manual" steps in this doc http://go//spitzer-layout-tests-doc. Additionally, test the black frames repro described in http://crbug.com/587994 Be sure to test the sdk boundaries 17/18/19 (not sure which of these actually require the reset - see my comments below). > > As far as I understand MediaCodec can properly process the resolution change > only starting with K > (https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...) My read of the linked code isn't that K+ automatically supports adaptive, but rather that K+ *might* support adaptive. Should we be calling this codecSupportsAdapativePlayback(...) instead of just checking the API level in AVDA? > so as long as we support JellyBean we need to (1) stop enqueueing frames, (2) > drain the codec > and (3) reconfigure, which means calling ConfigureMediaCodec(A)synchronously(). > > So I'd like to ask: > (1) Do we still have MediaCodec recreated for resolution change in MSE for > JellyBean? > (2) After the drain is completed the flush should be a no-op, what are these > "vended frames"?
https://codereview.chromium.org/2000833003/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2000833003/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1193: const bool codec_requires_reset_for_config_changes = It used to be that api >= 18 -> quick flush api < 18 -> new media codec Tima's comments seem to be that KitKat (19) is the starting point of good adaptive support, so I think you want this check to be < 19? As it is, you will not do the reset for JBMR2. If I have this right, you still need to separately check >= 18 to decide when to do a quick flush vs make a whole new media codec.
Also, Frank/Dale, I'm a bit confused about how the killed frames causes a blue frame? When I implemented the existing logic, Frank and I discussed how resetting MediaCodec would kill the frames, but IIRC we felt this was OK because the front buffer of the SurfaceTexture would still display fine (freeze). What am I missing?
On 2016/05/21 00:30:29, chcunningham wrote: > Also, Frank/Dale, I'm a bit confused about how the killed frames causes a blue > frame? When I implemented the existing logic, Frank and I discussed how > resetting MediaCodec would kill the frames, but IIRC we felt this was OK because > the front buffer of the SurfaceTexture would still display fine (freeze). What > am I missing? Yes, exactly. Based on recent Dale's comment I'm guessing that ResetCodecState(), among other things, calls strategy_->CodecChanged() which probably discards already dequeued but not yet released, i.e. rendered, frames. Please confirm or refute that, it is important to know.
https://codereview.chromium.org/2000833003/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2000833003/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:907: ResetCodecState(base::Closure()); Did you mean ConfigureMediaCodecAsynchronously() ? What else from ResetCodecState() would have to be done here? Set codec_needs_reset_ back to false? https://codereview.chromium.org/2000833003/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:997: void AndroidVideoDecodeAccelerator::Flush() { Is this flush() exists solely for the purpose to cope with dynamic resolution change? If so, then MediaCodec.flush() would not be needed after the drain for Android >= K, indeed, and do we even need to drain? In other words, we either need to drain because we need to recreate decoder after that, or we can NotifyFlushDone() immediately. Does it sound right?
On 2016/05/21 00:30:29, chcunningham wrote: > Also, Frank/Dale, I'm a bit confused about how the killed frames causes a blue > frame? When I implemented the existing logic, Frank and I discussed how > resetting MediaCodec would kill the frames, but IIRC we felt this was OK because > the front buffer of the SurfaceTexture would still display fine (freeze). What > am I missing? sandersd figured out that the blue frame comes from a bad texture matrix. the video had four frames. the last two hadn't been drawn yet when the flush happens. since the texture matrix is stored per codec image, these never got a chance to get the texture matrix from the SurfaceTexture when they finally were drawn. they instead used the default. that would be fine, except the default turns out to be broken, and was causing the draw to use just the first row of the frame (all blue background) for the entire video frame. if the default was what it was intended to be, then we wouldn't have noticed. which reminds me -- dale, would you mind fixing the default texture matrix as part of this CL? the matrix we were getting on the first two frames is what the default is supposed to be. it just needs [13]=1, i think, though you might want to verify that against the logs you got today. not as part of this CL, but we're going to move the texture matrix to the shared state instead of making it per image.
The beauty of the web-platform tests effort is you don't need to the layout test runner anymore to run them :) http://w3c-test.org/media-source/mediasource-config-change-mp4-v-framesize.html https://codereview.chromium.org/2000833003/diff/20001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2000833003/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:907: ResetCodecState(base::Closure()); On 2016/05/21 at 06:40:31, Tima Vaisburd wrote: > Did you mean ConfigureMediaCodecAsynchronously() ? What else from ResetCodecState() would have to be done here? > > Set codec_needs_reset_ back to false? ResetCodecState() does all the necessary things: Configure() and clears |codec_needs_reset_|. https://codereview.chromium.org/2000833003/diff/20001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1193: const bool codec_requires_reset_for_config_changes = On 2016/05/21 at 00:25:33, chcunningham wrote: > It used to be that > > api >= 18 -> quick flush > api < 18 -> new media codec > > Tima's comments seem to be that KitKat (19) is the starting point of good adaptive support, so I think you want this check to be < 19? As it is, you will not do the reset for JBMR2. > > If I have this right, you still need to separately check >= 18 to decide when to do a quick flush vs make a whole new media codec. Actually, this needs to always reset since once we queue the EOS for flush there's no turning back. In the future we can try to figure out when to flush vs when to reset and how adaptive playback might be better supported.
lgtm with a comment https://codereview.chromium.org/2000833003/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2000833003/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:904: // If we previously deferred a codec restart, take care of it now. This can Do you want to DCHECK that drain_type_ == DRAIN_TYPE_NONE ?
lgtm. nice. -fl
https://codereview.chromium.org/2000833003/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2000833003/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1192: if (drain_type_ == DRAIN_FOR_FLUSH && !did_codec_error_happen) { Can we do this even if there's no drain going on? Then this would also fix the unnecessary codec creation we do for Suspends for fullscreen https://bugs.chromium.org/p/chromium/issues/detail?id=599288
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2000833003/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2000833003/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:904: // If we previously deferred a codec restart, take care of it now. This can On 2016/05/24 at 19:06:26, Tima Vaisburd wrote: > Do you want to DCHECK that drain_type_ == DRAIN_TYPE_NONE ? Done. https://codereview.chromium.org/2000833003/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1192: if (drain_type_ == DRAIN_FOR_FLUSH && !did_codec_error_happen) { On 2016/05/24 at 19:47:26, watk wrote: > Can we do this even if there's no drain going on? Then this would also fix the unnecessary codec creation we do for Suspends for fullscreen https://bugs.chromium.org/p/chromium/issues/detail?id=599288 We probably can, but doing so exposed a few issues with how we're handling reset right now. Namely that ERROR state is non-terminal and ResetCodecState can escape out of it. Probably this should be fixed first, so I'm going to defer this change to a followup CL.
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/2000833003/#ps80001 (title: "Fix comment. Add dcheck.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2000833003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2000833003/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Don't reset the codec state for a flush; this kills the frames. Causing a reset after flush completes will invalidate all the vended frames, don't do this. Instead only reset when a reset should actually occur. Instead, since this reset is required on JellyBean devices, we issue the reset upon receipt of the next Decode() call. This allows the more frequent EOS cases to not trash the last frame on these devices will config changes will work as before. A followup CL will move the texture matrix to the shared state so that we don't accidentally apply the wrong matrix after the reset case. BUG=613608 TEST=no more random blue frames. ========== to ========== Don't reset the codec state for a flush; this kills the frames. Causing a reset after flush completes will invalidate all the vended frames, don't do this. Instead only reset when a reset should actually occur. Instead, since this reset is required on JellyBean devices, we issue the reset upon receipt of the next Decode() call. This allows the more frequent EOS cases to not trash the last frame on these devices will config changes will work as before. A followup CL will move the texture matrix to the shared state so that we don't accidentally apply the wrong matrix after the reset case. BUG=613608 TEST=no more random blue frames. Committed: https://crrev.com/eb4a8d89c1c1c3ed4b74e2afdee7ccd8dd6558bc Cr-Commit-Position: refs/heads/master@{#395995} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/eb4a8d89c1c1c3ed4b74e2afdee7ccd8dd6558bc Cr-Commit-Position: refs/heads/master@{#395995} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
