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

Seeking into AC4 audio only content with FMP4 subfragments creates discontinuities #11000

Closed
1 task done
ronak2121 opened this issue Feb 18, 2023 · 44 comments
Closed
1 task done
Assignees

Comments

@ronak2121
Copy link

ronak2121 commented Feb 18, 2023

ExoPlayer Version

2.15.1

Devices that reproduce the issue

  • Samsung S11 with Android 11

Devices that do not reproduce the issue

Google Pixels as they do not support decoding the Dolby AC-4 codec

Reproducible in the demo app?

Yes

Reproduction steps

  1. Play attached media
  2. Seek the player forward
  3. Watch playhead snap to the beginning of the segment instead of seeking to specified point

Expected result

Player seeks to IFrame as close to specified point as possible, parsing subfragments within the segment accordingly.

Actual result

Player detects discontinuity and snaps to the closest Iframe at beginning of segment

Media

Will email clear AC4 media separately

Bug Report

@ronak2121
Copy link
Author

Just to note that if we set the fragment sizes to be the same as the segments this doesn’t happen.

it’s only when fragments are smaller that Exoplayer doesn’t take subfragments into account when finding the appropriate IFrame.

@ronak2121 ronak2121 changed the title Seeking into AC4 content with FMP4 subfragments creates discontinuities Seeking into AC4 audio only content with FMP4 subfragments creates discontinuities Feb 18, 2023
@ronak2121
Copy link
Author

ronak2121 commented Feb 20, 2023

Also to note, this works just fine for audio codecs that don’t use IFrames such as AAC, DD+JOC.

@ronak2121
Copy link
Author

ronak2121 commented Feb 21, 2023

Here's what we've traced so far:

  1. FragmentedMP4Extractor seems to only seek into the current fragment at the start of the segment regardless of codec. If the desired position is past that fragment, it doesn't do anything.
  2. SampleQueue's seeking logic seems to kick in to seek into the extracted audio buffer. It seeks into the fragments themselves, looking for the closest keyFrame.
  3. The logic to decide to commit the sample or not seems to be the key. If we force AC4 to behave like all frames are iframes here; it works fine.

@ronak2121
Copy link
Author

FYI that we've emailed our media and ADB log dump as requested

@ronak2121
Copy link
Author

ronak2121 commented Feb 21, 2023

We’re working on a PR that should resolve this issue. My coworker (@hcgil) will post it within the next day or so.

@hcgil
Copy link

hcgil commented Feb 22, 2023

Just put up a PR with a rough solution to the problem we're encountering. More info is given above as well as in the pr description - from the looks of it we're starting playback at the end of the first fragment because we cannot guarantee the presence of sync frames later on. This PR should resolve this by keeping a record of which samples we've encountered since the last sync frame, and committing them (and any successive frames) once we reach our start time, to hopefully decrease the magnitude of discrepancies when seeking.

@ronak2121
Copy link
Author

ronak2121 commented Mar 1, 2023

Hi @rohitjoins we learned that xHE-AAC May also impacted by this issue. Have you had a chance to look at our PR? We're working with Dolby on this to ensure they are aligned with our suggested solution, but we're looking for your guidance as well.

@rohitjoins
Copy link
Contributor

@ronak2121 I haven't had the time to look into this yet. You can expect a reply from me by next week. Thanks :)

@ronak2121
Copy link
Author

@rohitjoins FYI that we’ve received confirmation from Dolby that this fix is the correct one. Please advise.

@ronak2121
Copy link
Author

ronak2121 commented Mar 14, 2023

Hi @rohitjoins any update? This issue is blocking a critical upcoming launch for us. Can you please look into this?

@rohitjoins
Copy link
Contributor

@ronak2121 Sorry for the late reply. I only see the bug report attached to the email you have sent to us and not the media. Can you please send the media? And also confirm here when it is done. Thanks.

@hcgil
Copy link

hcgil commented Mar 14, 2023

Just sent a link to the relevant testing asset as a reply to the initial bug report.

@ronak2121
Copy link
Author

Hi @rohitjoins please see our response above.

@rohitjoins
Copy link
Contributor

@hcgil On playing the testing asset on Exoplayer demo app, I can hear nothing. Unable to play the media on VLC as well. Also I'm able to scrub the player forward without any problems.

@hcgil
Copy link

hcgil commented Mar 17, 2023

@rohitjoins There's a chance something might have happened to the asset; we've just regenerated it and it appears to be working now. We've had some trouble in the past playing AC-4 assets on emulator/on a personal computer (I can't get the assets to play back in VLC), but they appear to be working on Android devices which support Dolby Atmos playback

@ronak2121
Copy link
Author

Hi @rohitjoins pinging you again on this.

@rohitjoins
Copy link
Contributor

@hcgil Tried playing the testing asset again and was able to seek around the duration freely. Can you please provide an asset which can be used to reproduce this issue?

@ronak2121
Copy link
Author

ronak2121 commented Mar 21, 2023

Are you using the same setup as us @rohitjoins ? We can reproduce this issue with that same asset with your test app. Which device are you using?

@rohitjoins
Copy link
Contributor

@ronak2121 Thank you for all the info. I'll find a device which supports AC-4 and merge this PR as soon as possible.

@ronak2121
Copy link
Author

hi @rohitjoins any update?

@rohitjoins
Copy link
Contributor

rohitjoins commented Mar 28, 2023

@ronak2121, I can confirm that I can replicate the problem, and I can observe the disparity in the seek behaviour with and without the fix you provided. While the solution enhances the seeking experience, I don't think it's necessary to modify the generic SampleQueue. As a result, I'm investigating the problem on my end and performing a thorough root cause analysis. I will keep you informed of any developments as I progress.

@rgalv-Dolby
Copy link

@rohitjoins if you have any questioins about the ac-4 stream itself and how it uses i-frames please let me know.

@ronak2121
Copy link
Author

@rohitjoins was wondering if you had an update on this? We can confirm that seeking with xHE-AAC is not a problem if we configure things the way it's broken with AC-4. That doesn't make sense to us in terms of the fix @hcgil uploaded.

@ronak2121
Copy link
Author

Hi @rohitjoins checking in on your progress.

@ronak2121
Copy link
Author

Hi @rohitjoins can you please let us know the next steps here?

@rohitjoins
Copy link
Contributor

@ronak2121 I am sorry that I was unable to work on this issue. I will do my best to prioritise it before our next release which is still a couple of months away.

@ronak2121
Copy link
Author

Can you please let us know what you think is the true fix if it’s not our proposal? @rohitjoins?

@ronak2121
Copy link
Author

@rohitjoins can you please help unblock us?

If you don't have the time to follow up, can you please tell us what you feel is the better fix? We can take on the work to implement it so we get unblocked faster.

@rohitjoins
Copy link
Contributor

@ronak2121 Thank you for your follow-up. As I mentioned earlier, we will target a fix for this issue in our next release which should be next month. Please let me know if there is a way to unblock before that?

I have not performed a thorough root cause analysis to know what a better fix should be yet. I understand that this issue is critical for you, and I am doing my best to prioritise it as much as possible. Thanks for understanding.

@ronak2121
Copy link
Author

Sounds good; Are we still on track to merge this for this month's release? @rohitjoins

@ronak2121
Copy link
Author

ronak2121 commented May 19, 2023

Hi @rohitjoins I saw the Exoplayer release came out 2 days ago, but this wasn't included. Can you please help? Who can I escalate to at Google to get prioritization?

@ronak2121
Copy link
Author

@rohitjoins?

@ronak2121
Copy link
Author

@rohitjoins please advise what the next steps are.

@rohitjoins
Copy link
Contributor

Just quickly wanted to let you know that these were bug fix releases. We are trying our best to accommodate this fix in our next normal release. Will keep you posted.

@tonihei
Copy link
Collaborator

tonihei commented Jun 7, 2023

We have a fix for this issue, but it's quite risky to submit it as it touches code that is used in many different scenarios and we like to let it sit for a bit with internal apps to ensure it doesn't break anything.

For more context: The underlying reason for what you are seeing is that the AC4 decoder on this device produces decoded output samples with timestamps that are different from the input sample timestamps. This is technically allowed, but we have some remaining code in ExoPlayer that relies on matching timestamps (because historically, this used to be always true). Our plan is to remove this dependency on matching timestamps, but as I said in the first paragraph, it touches an area where we've seen many variations of different behaviors and we need to be careful not to break another use case by fixing this one. We will likely push a change to GitHub fairly soon, so you are free to pick this up by depending on the source or wait until the next release (1.2.0).

@ronak2121
Copy link
Author

awesome thanks for the update!

@rohitjoins rohitjoins assigned tonihei and unassigned rohitjoins Jun 13, 2023
@ronak2121
Copy link
Author

Is this the commit you're referencing @tonihei ? 08c189e#diff-793f10aa618621cccb28e4b688e95913b4027a13a39616c136c20b4695b41f9a

icbaker pushed a commit to androidx/media that referenced this issue Jun 15, 2023
We currently do the same check on the input timestamps and
expect the output timestamps to match. Some codecs produce
samples with modified timestamps and the logic is a lot safer
when the comparison with the start time is done on the output
side of the codec.

Issue: google/ExoPlayer#11000
PiperOrigin-RevId: 540228209
icbaker pushed a commit that referenced this issue Jun 15, 2023
We currently do the same check on the input timestamps and
expect the output timestamps to match. Some codecs produce
samples with modified timestamps and the logic is a lot safer
when the comparison with the start time is done on the output
side of the codec.

Issue: #11000
PiperOrigin-RevId: 540228209
@tonihei
Copy link
Collaborator

tonihei commented Jun 15, 2023

No, the fix commit is the one linked in the issue above (cf02581). I can also close the issue for now since the issue has been fixed, but as noted already it won't be released until Media3 1.2.0.

@tonihei tonihei closed this as completed Jun 15, 2023
tof-tof pushed a commit to androidx/media that referenced this issue Jun 26, 2023
*** Original commit ***

Mark output sample as decode-only based on start time

We currently do the same check on the input timestamps and
expect the output timestamps to match. Some codecs produce
samples with modified timestamps and the logic is a lot safer
when the comparison with the start time is done on the output
side of the codec.

Issue: google/ExoPlayer#11000

***

PiperOrigin-RevId: 543379665
tof-tof pushed a commit that referenced this issue Jun 26, 2023
*** Original commit ***

Mark output sample as decode-only based on start time

We currently do the same check on the input timestamps and
expect the output timestamps to match. Some codecs produce
samples with modified timestamps and the logic is a lot safer
when the comparison with the start time is done on the output
side of the codec.

Issue: #11000

***

PiperOrigin-RevId: 543379665
icbaker pushed a commit that referenced this issue Jul 21, 2023
*** Original commit ***

Rollback of cf02581

*** Original commit ***

Mark output sample as decode-only based on start time

We currently do the same check on the input timestamps and
expect the output timestamps to match. Some codecs produce
samples with modified timestamps and the logic is a lot safer
when the comparison with the start time is done on the output
side of the codec.

Issue: #11000

***

***

PiperOrigin-RevId: 549019403
icbaker pushed a commit to androidx/media that referenced this issue Jul 21, 2023
*** Original commit ***

Rollback of b69b332

*** Original commit ***

Mark output sample as decode-only based on start time

We currently do the same check on the input timestamps and
expect the output timestamps to match. Some codecs produce
samples with modified timestamps and the logic is a lot safer
when the comparison with the start time is done on the output
side of the codec.

Issue: google/ExoPlayer#11000

***

***

PiperOrigin-RevId: 549019403
@google google locked and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants