Feature/dtsx codec updates #9163
Conversation
|
@kim-vde Do you have any comments on this PR? |
library/core/src/main/java/com/google/android/exoplayer2/mediacodec/MediaCodecInfo.java
Outdated
Show resolved
Hide resolved
|
I've just merged the change in 8e29e76. Our tooling didn't work properly so the issue is not marked as so. I will close it. |
|
The tooling didn't mark the PR as closed because @ProtoScott reverted the commit head on the 22 after @kim-vde merged the PR in our staging branch. |
That change was reverted based on feedback from @kim-vde. This change was intended. |
|
Just to clarify, we did merge the right thing here (i.e., we did not forget to include the revert). You can verify this by looking at 8e29e76. The PR not being marked as merged does not imply that we didn't pick up a commit that we should have. In most cases it's just an (avoidable) process problem with the way we merge the internal commit. |
|
Technically the revert commit was not merged. The merge commit 8e29e76 has 2 parent, one is the dev-v2 branch head before the merge (72cf9c3), the second is the 3rd commit of the PR: 83d2c39. If the revert commit (3003fc9) had been merged, it would be the parent of the merge commit. This can be confirm with: $ git branch --contains 83d2c39a363ab76bc7415f30b47aa7aff4e55a23
* dev-v2
$ git branch --contains 3003fc91fc8c65d7caf6bfefcd8e27c301c0c1e3
error: no such commit 3003fc91fc8c65d7caf6bfefcd8e27c301c0c1e3
$ tig 8e29e76b517157decf3a5993cbc32ffc759173ca
2021-07-23 14:12 +0100 kim-vde M─┐ Merge pull request #9163 from ProtoScott:feature/dtsx_codec_updates
2021-07-08 11:17 +0100 Scott │ o Updated MediaCodecInfo with correct channel adjustments for DTS codecs
2021-07-08 11:11 +0100 Scott │ o Updated mimetypes to support DTSX and to correct assign dtse to DTS Express
2021-07-08 11:07 +0100 Scott │ o Updates to mp4 atom extractor/parser for dtsx support.As you can see the revert commit never made it in the dev-v2 branch. Nevertheless, the content of the revert did make it in the merge commit. If we recreate the merge commit, without any modification, and diff it with the actual merge (8e29e76): $ git checkout 8e29e76b517157decf3a5993cbc32ffc759173ca^1
$ git merge 8e29e76b517157decf3a5993cbc32ffc759173ca^2
$ git diff HEAD 8e29e76b517157decf3a5993cbc32ffc759173ca
--- a/RELEASENOTES.md
+++ b/RELEASENOTES.md
@@ -103,6 +103,9 @@
+* Extractors:
+ * Add support for DTS-UHD in MP4
+ ([#9163](https://github.com/google/ExoPlayer/issues/9163).
--- a/library/common/src/main/java/com/google/android/exoplayer2/util/MimeTypes.java
+++ b/library/common/src/main/java/com/google/android/exoplayer2/util/MimeTypes.java
- public static final String AUDIO_DTS_X = BASE_TYPE_AUDIO + "/vnd.dts.uhd";
+ public static final String AUDIO_DTS_UHD = BASE_TYPE_AUDIO + "/vnd.dts.uhd";
@@ -385,10 +385,10 @@ public final class MimeTypes {
- } else if (codec.startsWith("dtsx")) {
- return MimeTypes.AUDIO_DTS_X;
} else if (codec.startsWith("dtsh") || codec.startsWith("dtsl")) {
return MimeTypes.AUDIO_DTS_HD;
+ } else if (codec.startsWith("dtsx")) {
+ return MimeTypes.AUDIO_DTS_UHD;
--- a/library/common/src/test/java/com/google/android/exoplayer2/util/MimeTypesTest.java
+++ b/library/common/src/test/java/com/google/android/exoplayer2/util/MimeTypesTest.java
@@ -137,9 +137,10 @@ public final class MimeTypesTest {
- assertThat(MimeTypes.getMediaMimeType("dtse")).isEqualTo(MimeTypes.AUDIO_DTS);
+ assertThat(MimeTypes.getMediaMimeType("dtse")).isEqualTo(MimeTypes.AUDIO_DTS_EXPRESS);
assertThat(MimeTypes.getMediaMimeType("dtsh")).isEqualTo(MimeTypes.AUDIO_DTS_HD);
assertThat(MimeTypes.getMediaMimeType("dtsl")).isEqualTo(MimeTypes.AUDIO_DTS_HD);
+ assertThat(MimeTypes.getMediaMimeType("dtsx")).isEqualTo(MimeTypes.AUDIO_DTS_UHD);
--- a/library/core/src/main/java/com/google/android/exoplayer2/mediacodec/MediaCodecInfo.java
+++ b/library/core/src/main/java/com/google/android/exoplayer2/mediacodec/MediaCodecInfo.java
@@ -624,10 +624,6 @@ public final class MediaCodecInfo {
- || MimeTypes.AUDIO_DTS.equals(mimeType)
- || MimeTypes.AUDIO_DTS_HD.equals(mimeType)
- || MimeTypes.AUDIO_DTS_EXPRESS.equals(mimeType)
- || MimeTypes.AUDIO_DTS_X.equals(mimeType)
--- a/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp4/AtomParsers.java
+++ b/library/extractor/src/main/java/com/google/android/exoplayer2/extractor/mp4/AtomParsers.java
@@ -1373,7 +1373,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType;
- mimeType = MimeTypes.AUDIO_DTS_X;
+ mimeType = MimeTypes.AUDIO_DTS_UHD;So the revert commit was never merged, but @kim-vde made the same change (and others) after importing the PR.
I don't believe this to be correct. Anytime the PR is not marked as merged, it means the commits merged were not the exact ones from the PR. Though as here, we might have done the same manual change in the merge commit.
I think we should double check any time this occurs, which should be pretty rare. |
|
It makes sense to me that the PR has not been automatically marked as merged, because, as @krocard noted, the last commit has not been technically merged. The question is more, what's the best way to deal with this situation? Should we for example update the commit ID in the change description so that the PR is automatically marked as merged when we push the change? |
|
I think the main thing we want to avoid is that the PR author finds a bug, updates the PR, we miss the fix and erroneously manually close the PR as merged. Let's discuss about the best way to avoid this situation in our next team meeting. |
Updates to the ExoPlayer mp4 atom extractor/parser for DTS:X support.
Updates to the ExoPlayer MimeTypes to support DTS:X.
Updated MediaCodecInfo with correct channel adjustments for DTS codecs.