Fix missing last frame of MPEG-TS content #419

dsparano · 2023-05-25T15:33:17Z

Issue
With TS contents, any codec, the last access unit is never passed on to the input sample queue. The reason is that the end of the sample is currently triggered by the payload_unit_start_indicator (pusi) of the first TS packet of the following access unit, which doesn't exist for the very last access unit. As a result the playback of any TS content always misses the last frame.

For frame accurate applications this seems a non negligible issue.

This problem was already discussed here google/ExoPlayer#7909 and that ticket is still open.

Proposed solution
In our understanding, for a file based case, it is reasonable to assume that the end of stream is a valid end of the last access unit, provided all the accumulated TS packets of the access unit have correct TS header parameters such as transport_error_indicator (=0) and continuity_counter.

This pull request comes from a fix we implemented in our LCEVC-Exo fork. The idea is that on end of stream:

TsExtractor sends an empty data payload with flagged pusi to PesReader;
PesReader recognises the end of input case from the empty payload and calls a modified packetFinished() with boolean endOfInput parameter to the ES reader;
The ES reader calls a new SampleReader method called end() that flushes the accumulated access unit;

tonihei · 2023-05-25T15:45:57Z

Thanks for the contribution! It looks like something went wrong with your PR setup as it includes thousands of commits (mostly by our team). Could you try to rebase/clean this up?

dsparano · 2023-05-25T16:50:36Z

Sorry, had wrongly left the base branch set to release :( should be fixed now.

icbaker · 2023-05-30T16:28:31Z

Please sign the CLA and we can take a look.

dsparano · 2023-05-31T11:00:00Z

Ready for review, sorry for the delay.

icbaker · 2023-06-05T11:25:54Z

I found that lots of tests in the extractor.ts package fail with these changes.

I was able to fix many of them by updating the golden test files by temporarily setting DumpFileAsserts.DUMP_FILE_ACTION to WRITE_TO_LOCAL (and this is expected, since we expect to see more samples at the end of playback), but there are still a lot of failures (not due to golden test file mismatch). Please could you try running these tests and investigate the failures? They are mostly of this form:

Index 0 out of bounds for length 0
java.lang.ArrayIndexOutOfBoundsException: Index 0 out of bounds for length 0
	at androidx.media3.common.util.ParsableByteArray.readUnsignedByte(ParsableByteArray.java:260)
	at androidx.media3.extractor.ts.SectionReader.consume(SectionReader.java:71)
	at androidx.media3.extractor.ts.TsExtractor.read(TsExtractor.java:331)
	at androidx.media3.test.utils.ExtractorAsserts.consumeTestData(ExtractorAsserts.java:529)
	at androidx.media3.test.utils.ExtractorAsserts.consumeTestData(ExtractorAsserts.java:510)
	at androidx.media3.test.utils.ExtractorAsserts.assertOutput(ExtractorAsserts.java:452)
	at androidx.media3.test.utils.ExtractorAsserts.assertBehavior(ExtractorAsserts.java:401)
	at androidx.media3.extractor.ts.TsExtractorTest.sampleWithAit(TsExtractorTest.java:114)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)

Also: I tried to push my commit updating the golden test files, but was unable to. Is it possible that you need to allow permission to me to push commits to this branch? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

We will likely need to push more changes during internal review as well, so please check that access has been granted. Thanks!

dsparano · 2023-06-05T16:59:39Z

@icbaker you should have access now. Let me check the tests.

dsparano · 2023-06-06T15:00:17Z

Tests are now passing for me (after setting WRITE_TO_LOCAL to update the dumps).

icbaker · 2023-06-07T13:51:11Z

I received an invite to collaborate on https://github.com/v-novaltd/androidx-media which I've accepted, but I'm still not able to push commits to this PR. I did some experimentation with #443 and I see the same problem (can't push changes from androidx back to my icbaker personal fork) if I untick the 'allow edits by maintainers' on the PR itself.

I can't see this tickbox on this PR, I think only you can as the creator of the PR. Can you please make sure it's ticked? Thanks!

GitHub docs on this: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Please also take a look at the couple of comments I added in the code itself.

icbaker · 2023-06-07T13:56:59Z

Ah, it's possible you can't tick this box this because v-novaltd is a GitHub organisation rather than an individual user: https://github.com/orgs/community/discussions/5634

And the recommended workaround in https://github.com/orgs/community/discussions/5634#discussioncomment-2155236 is exactly what you've done.

Let me have a play around and see if I can work with the permission you've granted me. Sorry for being a bit unfamiliar here, we've recently changed the way we handle PRs to a process that requires pushing our own commits to the PR before merging it (to avoid 'evil merges') - you can see the full discussion about that here: google/ExoPlayer@e811749#commitcomment-107715150

dsparano · 2023-06-07T15:04:48Z

@icbaker Yes, I cannot see the option 'Allow edits by maintainers' as v-novaltd is a github org.

I'm afraid I don't see your comments yet.

libraries/extractor/src/main/java/androidx/media3/extractor/ts/H264Reader.java

icbaker · 2023-06-05T10:48:06Z

libraries/extractor/src/main/java/androidx/media3/extractor/ts/TsExtractor.java

@@ -323,7 +323,14 @@ public void release() {
    }

    if (!fillBufferWithAtLeastOnePacket(input)) {
-      return RESULT_END_OF_INPUT;
+      if (mode != MODE_HLS) {


Can you add some context about why it's correct to only do this if mode != HLS?

I've seen this PR, as it is, would break HLS mode, because of the presence of the next segment. However I think the logic of the PR should still apply to the very last segment of a VOD HLS, which currently misses the last frame.

Let me investigate a bit further on this.

Of course when I say "would break" I mean without the if statement above

I've added the HLS case as well.

icbaker · 2023-06-07T16:48:52Z

I'm afraid I don't see your comments yet.

Sorry, I didn't realise they were stuck in 'pending' state - they should be published now I think.

dsparano · 2023-06-15T11:03:13Z

Current comments are now addressed.

icbaker · 2023-06-20T11:12:46Z

Thanks for the fixes!

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

…eaders; add end of input flag to readers to flush accumulated data from last PES

dsparano changed the base branch from release to main May 25, 2023 16:02

icbaker self-assigned this May 31, 2023

icbaker added the pending comments label Jun 5, 2023

icbaker reviewed Jun 7, 2023

View reviewed changes

icbaker removed the pending comments label Jun 20, 2023

icbaker added the should merge label Jun 20, 2023

icbaker force-pushed the vnova-104 branch from ac4a3fe to 3369a1d Compare June 20, 2023 11:13

dsparano added 5 commits June 20, 2023 17:08

Send empty pusi flagged data, on end of input, from TS extractor to r…

c6c048f

…eaders; add end of input flag to readers to flush accumulated data from last PES

Fix lib-extractor tests

f0666c6

Add end of input fix to H263 reader

4537291

Add end of input fix to H262 reader

550bc5b

Add end of TS input fix to HLS mode

b553d2c

icbaker force-pushed the vnova-104 branch 2 times, most recently from 18e705a to 3572f90 Compare June 20, 2023 16:12

icbaker added 2 commits June 21, 2023 10:00

Add a release note, fix some nullness and comments

35e179f

Update TS dump files with extra final sample

7cb7e8c

icbaker force-pushed the vnova-104 branch from 3572f90 to 7cb7e8c Compare June 21, 2023 09:00

tof-tof merged commit e665e2a into androidx:main Jun 22, 2023
1 check passed

icbaker mentioned this pull request Jun 29, 2023

Render final frame of MPEG-TS video google/ExoPlayer#7909

Closed

androidx locked and limited conversation to collaborators Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing last frame of MPEG-TS content #419

Fix missing last frame of MPEG-TS content #419

dsparano commented May 25, 2023

tonihei commented May 25, 2023

dsparano commented May 25, 2023

icbaker commented May 30, 2023

dsparano commented May 31, 2023

icbaker commented Jun 5, 2023

dsparano commented Jun 5, 2023

dsparano commented Jun 6, 2023

icbaker commented Jun 7, 2023

icbaker commented Jun 7, 2023

dsparano commented Jun 7, 2023

icbaker Jun 5, 2023

dsparano Jun 8, 2023

dsparano Jun 8, 2023

dsparano Jun 15, 2023

icbaker commented Jun 7, 2023

dsparano commented Jun 15, 2023

icbaker commented Jun 20, 2023

Fix missing last frame of MPEG-TS content #419

Fix missing last frame of MPEG-TS content #419

Conversation

dsparano commented May 25, 2023

tonihei commented May 25, 2023

dsparano commented May 25, 2023

icbaker commented May 30, 2023

dsparano commented May 31, 2023

icbaker commented Jun 5, 2023

dsparano commented Jun 5, 2023

dsparano commented Jun 6, 2023

icbaker commented Jun 7, 2023

icbaker commented Jun 7, 2023

dsparano commented Jun 7, 2023

icbaker Jun 5, 2023

Choose a reason for hiding this comment

dsparano Jun 8, 2023

Choose a reason for hiding this comment

dsparano Jun 8, 2023

Choose a reason for hiding this comment

dsparano Jun 15, 2023

Choose a reason for hiding this comment

icbaker commented Jun 7, 2023

dsparano commented Jun 15, 2023

icbaker commented Jun 20, 2023