-
Notifications
You must be signed in to change notification settings - Fork 460
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
Dts mpeg2ts update #275
Conversation
@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. |
@rohitjoins I have added test entries for sampleWithDts* in TsExtractorTest class and copied sample files into libraries\test_data\src\test\assets\media\ts directory.
|
@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 |
@rohitjoins Thanks for the prompt reply. I have added test cases for the TS extractor updates. Would you please review it? |
@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. |
@rohitjoins Thanks for reviewing the changes. |
@rahulnmohan One alternative suggestion could be to consider safely truncating the streams that were included in the tests. |
@rohitjoins I have updated the DTS test vectors. Would you please check if the file size is shorter enough to mainline? |
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/ts/DtsUhdReader.java
Outdated
Show resolved
Hide resolved
Hi @rohitjoins, please let me know if any further action is required from my side to merge these changes to the main branch. Thanks. |
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. |
libraries/extractor/src/main/java/androidx/media3/extractor/ts/DtsUhdReader.java
Outdated
Show resolved
Hide resolved
Hi @rohitjoins , |
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. |
There was a problem hiding this 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.
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/ts/DtsUhdReader.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/ts/DtsUhdReader.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/ts/DtsUhdReader.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/ts/DtsUhdReader.java
Outdated
Show resolved
Hide resolved
ed9c241
to
4c173c7
Compare
There was a problem hiding this 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.
libraries/extractor/src/main/java/androidx/media3/extractor/ts/DtsReader.java
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/DtsUtil.java
Show resolved
Hide resolved
…UHD header size. Update comments in the time stamp extraction code.
This comment was marked as off-topic.
This comment was marked as off-topic.
Hi @rohitjoins |
Hi @rahulnmohan, Yes, please look at the comment in L445 in |
@rohitjoins, Please see my reply here #275 (comment) |
@rahulnmohan, can you please see the comment and reply. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
break; | ||
case STATE_FINDING_EXTSS_HEADER_SIZE: | ||
// Read enough bytes to parse the header size information. | ||
if (continueRead(data, headerScratchBytes.getData(), /* targetLength= */ 10)) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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);
libraries/extractor/src/main/java/androidx/media3/extractor/ts/DtsReader.java
Show resolved
Hide resolved
…umber of bytes(7 bytes) to extract the ExtSS header size.
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
@rahulnmohan, the PR was merged as 27c021f to the main branch. Will be released with 1.3.0. |
…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
@rohitjoins, Thank you very much for the great help and the extended review. |
Added below mentioned features.
Similar changes are done for the ExoPlayer.
Pull request for the ExoPlayer updates: google/ExoPlayer#11076
Feature request for the ExoPlayer: google/ExoPlayer#11075