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

Dts mpeg2ts update #275

Closed
wants to merge 15 commits into from
Closed

Conversation

rahulnmohan
Copy link
Contributor

@rahulnmohan rahulnmohan commented Mar 22, 2023

Added below mentioned features.

  • Support for extracting DTS LBR(DTS Express) and DTS UHD Profile 2(DTS:X) descriptor ID from PSI PMT
  • The DTSReader class is updated for extracting a DTS LBR frame.
  • Newly added DTSUhdReader class for extracting DTS UHD frame.
  • The DTSUtil class is updated to parse the DTS LBR or DTS UHD frame and report the format information.

Similar changes are done for the ExoPlayer.
Pull request for the ExoPlayer updates: google/ExoPlayer#11076
Feature request for the ExoPlayer: google/ExoPlayer#11075

@rohitjoins
Copy link
Contributor

@rahulnmohan Do you think we can add some tests for this change?

@rahulnmohan
Copy link
Contributor Author

@rahulnmohan Do you think we can add some tests for this change?

@rohitjoins I will check how to add test cases for the proposed changes.

@rahulnmohan
Copy link
Contributor Author

@rohitjoins
Is there any documentation available for adding new tests to the framework?

I have added test entries for sampleWithDts* in TsExtractorTest class and copied sample files into libraries\test_data\src\test\assets\media\ts directory.
While running the test, seeing the below errors.

Dump file not found. To update the dump file, change DumpFileAsserts#DUMP_FILE_ACTION to WRITE_TO_LOCAL (for Robolectric tests) or WRITE_TO_DEVICE (for instrumentation tests) and re-run the test. java.io.IOException: Dump file not found. To update the dump file, change DumpFileAsserts#DUMP_FILE_ACTION to WRITE_TO_LOCAL (for Robolectric tests) or WRITE_TO_DEVICE (for instrumentation tests) and re-run the test. at androidx.media3.test.utils.DumpFileAsserts.assertOutput(DumpFileAsserts.java:163) at androidx.media3.test.utils.DumpFileAsserts.assertOutput(DumpFileAsserts.java:95) at androidx.media3.test.utils.ExtractorAsserts.assertOutput(ExtractorAsserts.java:454) at androidx.media3.test.utils.ExtractorAsserts.assertBehavior(ExtractorAsserts.java:398) at androidx.media3.test.utils.ExtractorAsserts.assertBehavior(ExtractorAsserts.java:354) at androidx.media3.extractor.ts.TsExtractorTest.sampleWithDts(TsExtractorTest.java:119) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) 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) at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:591) at org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$0(SandboxTestRunner.java:274) at org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:88) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:829) Caused by: java.io.FileNotFoundException: extractordumps/ts/sample_dts.ts.0.dump at org.robolectric.shadows.ShadowArscAssetManager10.nativeOpenAsset(ShadowArscAssetManager10.java:747) at android.content.res.AssetManager.nativeOpenAsset(AssetManager.java) at android.content.res.AssetManager.open(AssetManager.java:874) at android.content.res.AssetManager.open(AssetManager.java:851) at androidx.media3.test.utils.TestUtil.getInputStream(TestUtil.java:171) at androidx.media3.test.utils.TestUtil.getByteArray(TestUtil.java:166) at androidx.media3.test.utils.TestUtil.getString(TestUtil.java:176) at androidx.media3.test.utils.DumpFileAsserts.assertOutput(DumpFileAsserts.java:161) at androidx.media3.test.utils.DumpFileAsserts.assertOutput(DumpFileAsserts.java:95) at androidx.media3.test.utils.ExtractorAsserts.assertOutput(ExtractorAsserts.java:454) at androidx.media3.test.utils.ExtractorAsserts.assertBehavior(ExtractorAsserts.java:398) at androidx.media3.test.utils.ExtractorAsserts.assertBehavior(ExtractorAsserts.java:354) at androidx.media3.extractor.ts.TsExtractorTest.sampleWithDts(TsExtractorTest.java:119) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

@rohitjoins
Copy link
Contributor

@rahulnmohan The error is complaining about not finding the dump file for the sample test file you added. As mentioned in the error log, you'll need to temporarily modify this line in DumpFileAsserts from COMPARE_WITH_EXISTING to WRITE_TO_LOCAL and run the tests which will add/create the dump files with data for this new file. You can revert back this change once the new dump file is created. Dump file is data against which the test verifies the sample file when tests are run.

@rahulnmohan
Copy link
Contributor Author

@rohitjoins Thanks for the prompt reply. I have added test cases for the TS extractor updates. Would you please review it?

@rohitjoins
Copy link
Contributor

@rahulnmohan Thanks for adding the tests and sample files. One more request would be to minimise the size of the sample files(possibly down to single number digit of samples) and still maintain test coverage for them. We try to keep the test media duration reasonably short, so tests run faster and to avoid having a huge directory of binaries that everyone cloning the project externally has to check out.

@rahulnmohan
Copy link
Contributor Author

@rohitjoins Thanks for reviewing the changes.
Currently, I have submitted the same test streams which we added in the Android CTS. I will try for getting shorter streams. Thanks.

@rohitjoins
Copy link
Contributor

@rahulnmohan One alternative suggestion could be to consider safely truncating the streams that were included in the tests.

@rahulnmohan
Copy link
Contributor Author

@rohitjoins I have updated the DTS test vectors. Would you please check if the file size is shorter enough to mainline?

@rahulnmohan
Copy link
Contributor Author

Hi @rohitjoins, please let me know if any further action is required from my side to merge these changes to the main branch. Thanks.

@rohitjoins
Copy link
Contributor

Hi @rahulnmohan,

No at the moment. I have sent a PR internally for this change and is in the process of getting it reviewed. Will let you know if there is some major doubt/confusion over anything. Otherwise, should merge this soon. Thanks.

@rahulnmohan
Copy link
Contributor Author

Hi @rohitjoins ,
a gentle reminder of this pull request.
Would it be possible to share a tentative timeline to have these changes merged into the main branch?
Thank you.

@rohitjoins
Copy link
Contributor

@rahulnmohan

Please refrain from pushing any more changes as it will complicate the internal review. I would try my best to include this change into our next release i.e. 1.2.0. Thanks.

Copy link
Contributor

@rohitjoins rohitjoins left a comment

Choose a reason for hiding this comment

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

Sorry for replying on this after so long.

Copy link
Contributor

@rohitjoins rohitjoins left a comment

Choose a reason for hiding this comment

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

Hi @rahulmohan,

Please see three new comments added to resolve the final few things which is unclear to us to merge this PR.

…UHD header size. Update comments in the time stamp extraction code.
@google-oss-bot

This comment was marked as off-topic.

@rahulnmohan
Copy link
Contributor Author

Hi @rohitjoins
Please let me know if I need to further clarify anything in this thread.

@rohitjoins
Copy link
Contributor

Hi @rahulnmohan,

Yes, please look at the comment in L445 in DtsUtil.Java. I have also tagged you there to have your attention.

@rahulnmohan
Copy link
Contributor Author

@rohitjoins, Please see my reply here #275 (comment)

@rohitjoins
Copy link
Contributor

@rahulnmohan, can you please see the comment and reply.

Copy link
Contributor

@rohitjoins rohitjoins left a comment

Choose a reason for hiding this comment

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

Also added three other comments:
comment1
comment2
comment3

Please resolve these last few comments to make further progress in the merge process of this PR.

break;
case STATE_FINDING_EXTSS_HEADER_SIZE:
// Read enough bytes to parse the header size information.
if (continueRead(data, headerScratchBytes.getData(), /* targetLength= */ 10)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we intend to read 10 bytes here or just 6 as bytesRead is set to 4 when skipToNextSync is called?

May be we need to reset bytesRead (to zero?) each time we change state and start reading a new 'part' of the file with a different targetLength?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rohitjoins, To parse the ExtSS header size, 7 bytes is enough. The 'continueRead()' function handles the size of any extra bytes read before.
int bytesToRead = min(source.bytesLeft(), targetLength - bytesRead);

…umber of bytes(7 bytes) to extract the ExtSS header size.
copybara-service bot pushed a commit that referenced this pull request Jan 16, 2024
Imported from GitHub PR #275

Added below mentioned features.

- Support for extracting DTS LBR(DTS Express) and DTS UHD Profile 2(DTS:X) descriptor ID from PSI PMT
- The DTSReader class is updated for extracting a DTS LBR.
- Newly added DtsUhdReader class for extracting DTS UHD frame.
- The DTSUtil class is updated to parse the DTS LBR or DTS UHD frame and report the format information.

Feature request for ExoPlayer: google/ExoPlayer#11075
Merge 21efa08 into 104cfc3

COPYBARA_INTEGRATE_REVIEW=#275 from rahulnmohan:dts-mpeg2ts-update 21efa08
PiperOrigin-RevId: 598854998
@rohitjoins
Copy link
Contributor

@rahulnmohan, the PR was merged as 27c021f to the main branch.

Will be released with 1.3.0.

@rohitjoins rohitjoins closed this Jan 16, 2024
@rohitjoins rohitjoins reopened this Jan 16, 2024
@rohitjoins rohitjoins closed this Jan 16, 2024
copybara-service bot pushed a commit to google/ExoPlayer that referenced this pull request Jan 16, 2024
…TS:X Profile2

Imported from GitHub PR androidx/media#275

Added below mentioned features.

- Support for extracting DTS LBR(DTS Express) and DTS UHD Profile 2(DTS:X) descriptor ID from PSI PMT
- The DTSReader class is updated for extracting a DTS LBR.
- Newly added DtsUhdReader class for extracting DTS UHD frame.
- The DTSUtil class is updated to parse the DTS LBR or DTS UHD frame and report the format information.

Feature request for ExoPlayer: #11075
Merge 21efa0810db31550d6b215639f9ca2af6a32139a into 104cfc3

COPYBARA_INTEGRATE_REVIEW=androidx/media#275 from rahulnmohan:dts-mpeg2ts-update 21efa0810db31550d6b215639f9ca2af6a32139a
PiperOrigin-RevId: 598854998
@rahulnmohan
Copy link
Contributor Author

@rohitjoins, Thank you very much for the great help and the extended review.

@androidx androidx locked and limited conversation to collaborators Mar 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants