Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stuck buffering while tunneling. #6407

Draft
wants to merge 2 commits into
base: dev-v2
Choose a base branch
from

Conversation

stevemayhew
Copy link
Contributor

This pull fixes the issue where the video render does not correctly report ready in tunneled mode. The specific bug is #6366

I can remove the manifest and network security changes (these allow local http testing on my workstation). I've provided a stream that reproduces this on streaming devices with Broadcom SOC chips.

The issue is largely timing related, streams that have very low video frame rate compared to the audio will cause the issue to manifest strait away.

Copy link
Contributor Author

@stevemayhew stevemayhew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clearly a bug (Bug #6366 ) in the tunneling implementation. Can Andrew or someone else review this Thanks.

demos/main/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
@@ -365,6 +368,17 @@ private void initializePlayer() {
RenderersFactory renderersFactory =
((DemoApplication) getApplication()).buildRenderersFactory(preferExtensionDecoders);

boolean enableTunneling = intent.getBooleanExtra(ENABLE_TUNNELED_PLAYBACK, false);
// Get a builder with current parameters then set/clear tunnling based on the intent
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a simple intent extra (-ez enable_tunneled_playback true) that allows you view a URL with tunneling on for platforms that support it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A similar option was implemented in 9af8511

Please rebase.

protected boolean hasOutputReady() {
return hasOutputBuffer();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the crux of the fix, the comments should spell out the intentions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can review and debate a good name...

@@ -1362,7 +1376,7 @@ public boolean isReady() {
return inputFormat != null
&& !waitingForKeys
&& (isSourceReady()
|| hasOutputBuffer()
|| hasOutputReady()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More descriptive of what the check really is.

protected boolean hasOutputReady() {
return tunneling && buffersInCodecCount > 0 || super.hasOutputReady();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the comment says, in tunneling mode once we have queued buffers to the video codec to decode it will render it when the matching audio syncs up.

The bug occurs because the audio track is stopped before the audio matching the video PTS is queued thus stalling the video codec forever.

@andrewlewis andrewlewis self-assigned this Sep 20, 2019
@andrewlewis
Copy link
Collaborator

Had a quick look. This looks good for API 23 onwards where we can update buffersInCodecCount correctly. I'm not certain it works for API 21 though because we don't know when a buffer is rendered so we decrement the counter right after queueing (please let me know if I'm missing something).

@stevemayhew
Copy link
Contributor Author

stevemayhew commented Sep 23, 2019 via email

@stevemayhew
Copy link
Contributor Author

I pulled out the changes not required for the actual bug fix, sorry for polluting the pull request with these.

@andrewlewis andrewlewis assigned krocard and unassigned andrewlewis Nov 7, 2019
@andrewlewis andrewlewis assigned andrewlewis and unassigned krocard Nov 8, 2019
@andrewlewis
Copy link
Collaborator

Thinking about this more, I'm not sure it's safe to assume we get one output buffer per input buffer when using tunneling mode, due to the possibility of the OEM doing frame rate conversion (this wouldn't be valid for non-tunneled playbacks and I think is covered by CTS, but as I understand it one of the reasons that tunneling mode exists is to give flexibility in postprocessing). I will investigate further whether we can avoid using buffersInCodecCount.

@stevemayhew
Copy link
Contributor Author

@andrewlewis We are seeing this issue on boxes when there is low bandwidth that delays the audio (so similar to the issue the Music Choice reproduces).

One of my team members, @dpolyakova is our internal expert on all things Broadcom media stack and has some ideas on this.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

In tunneled playback mode the video renderer is potentially 'ready' with output at anytime there are buffers queued to the codec.   This fix add the more specific boolean `hasOuputReady()` and uses this in the `isReady()` check.

This changes fixes issue google#6366

The call to dequeueOutputBuffer() simply returns TRY_AGAIN always in tunneled mode (this comment is correct: google#1688 (comment) so the super.hasOutputReady() is always false in tunneling mode, even though the codec may have output ready.
…or. This allows player to go into buffering state before video playback starts freezing.
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@dpolyakova dpolyakova force-pushed the stuck-buffering-tunnel-issue branch from 818e5af to 59fd033 Compare April 1, 2020 21:19
@@ -409,7 +415,15 @@ public boolean isReady() {
*/
@Override
protected boolean hasOutputReady() {
return tunneling && buffersInCodecCount > 0 || super.hasOutputReady();
boolean fifoReady = true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are trying to use FIFO length as renderer readiness criteria. If the frame rate is low such as 1fps on music channels the length would still be satisfactory and player wouldn't go into buffering state as long as there is a frame queued in the FIFO. If network bandwidth is low the FIFO length will get close to zero before decoder starts stalling. It's important to note that Broadcom's decoder will stall before rendering last few frames so zero FIFO length cannot be used as a criteria. The 333ms value is somewhat experimental. It was chosen to accommodate at least 10 frames of 30fps stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewlewis @ojw28 Where are you guys at with addressing this issue. I'm preparing to update our code base to what will likely by your 2.11.5 release. We would very much like to not keep merging conflicted changes to the MediaCodecVideoRenderer, for obvious reasons.

Pretty sure we can send you an STB (Broadcom based) that reproduces the issue #6366 I think I included the URL for a section of music choice content that reproduces the issue 100% of the time

We are also see this, as Dasha explained, in other streams where audio is buffered sufficiently ahead of video. As I'm sure you guys are aware, Broadcom codecs often require some special spoon feeding.

Thanks again for looking at this with us, please tell me what I need to do to help the process along.

@dpolyakova
Copy link

@googlebot I consent

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Apr 1, 2020
@stevemayhew stevemayhew marked this pull request as draft February 25, 2022 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants