search

google / ExoPlayer

This project is deprecated and stale. The latest ExoPlayer code is available in https://github.com/androidx/media
https://developer.android.com/media/media3/exoplayer
Apache License 2.0
21.74k stars 6.03k forks source link

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

Closed ronak2121 closed 1 year ago

ronak2121 commented 1 year ago

ExoPlayer Version

2.15.1

Devices that reproduce the issue

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 commented 1 year ago

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 commented 1 year ago

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

ronak2121 commented 1 year ago

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 commented 1 year ago

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

ronak2121 commented 1 year ago

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

hcgil commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

ronak2121 commented 1 year ago

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

ronak2121 commented 1 year ago

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

rohitjoins commented 1 year ago

@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 commented 1 year ago

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

ronak2121 commented 1 year ago

Hi @rohitjoins please see our response above.

rohitjoins commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

Hi @rohitjoins pinging you again on this.

rohitjoins commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

I'm using Google Pixel 4A. Is this issue device and Exoplayer version dependent? I assumed we still have this issue and trying to reproduce it on current version i.e. r2.18.4.

hcgil commented 1 year ago

@rohitjoins On our end we're able to reproduce the issue with the linked asset - just tested on r2.18.4 and still getting that issue, we're able to seek normally but once playback starts the play head is jumping back anywhere from 1 - 10 seconds. To my knowledge we've been able to reproduce this issue on a OnePlus 9 and a Samsung S10+, no knowledge on whether this is reproducible on other devices.

It could be possible that some portion of this issue is device specific. We noticed that other formats with subfragmentsw and specified sync frames (in our case xHE-AAC) should be affected by this as well, but that doesn't seem to be the case on our end.

ronak2121 commented 1 year ago

As far as we're aware; Google Pixels do not support Dolby Atmos. They do not have DD+JOC or AC-4 decoders.

ronak2121 commented 1 year ago

What are our next steps @rohitjoins? Are you getting another device? Do you need Dolby to formally confirm they endorse this fix?

rgalv-Dolby commented 1 year ago

@ronak2121 you are correct, Google Pixels do not support AC-4. @rohitjoins, in order to test this you will need a Dolby enabled device like a Samsung phone/table. Dolby has tested the changes and we can confirm the improved scrubbing behavior with no issues. Here are a few extra streams with audio and video that you can use to test. https://ott.dolby.com/media/ac4_exoplayer_test/master.m3u8 https://ott.dolby.com/media/ac4_exoplayer_test/stream.mpd

ronak2121 commented 1 year ago

@rohitjoins please advise what else you need from us to get our PR merged into an official Exoplayer release.

rohitjoins commented 1 year ago

@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 commented 1 year ago

hi @rohitjoins any update?

rohitjoins commented 1 year ago

@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 commented 1 year ago

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

ronak2121 commented 1 year ago

@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 commented 1 year ago

Hi @rohitjoins checking in on your progress.

ronak2121 commented 1 year ago

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

rohitjoins commented 1 year ago

@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 commented 1 year ago

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

ronak2121 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

ronak2121 commented 1 year ago

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 commented 1 year ago

@rohitjoins?

ronak2121 commented 1 year ago

@rohitjoins please advise what the next steps are.

rohitjoins commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

awesome thanks for the update!

ronak2121 commented 1 year ago

Is this the commit you're referencing @tonihei ? https://github.com/google/ExoPlayer/commit/08c189e768d77e9e2a9665b23fa384e776f9678b#diff-793f10aa618621cccb28e4b688e95913b4027a13a39616c136c20b4695b41f9a

tonihei commented 1 year ago

No, the fix commit is the one linked in the issue above (https://github.com/google/ExoPlayer/commit/cf02581d8fcdc8290d8c85625100a3e90247efde). 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.