-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Ogg fixes #10807
Ogg fixes #10807
Conversation
Do not assume that the comment header is in the same page as the identification header.
Ignore non supported tracks in ogg files and try to find a supported one. Allow to play vorbis files that embed fake video tracks containing the track thumbnail.
Ensure that the proper pages are read and not mixed with other streams.
Thank you for sending the PR. I'll take a look. |
@rohitjoins it lacks new tests and probably some more code refactoring to reduce duplicate code and position handling in the buffers. But wanted to discuss before. (And probably extracting a new C.XXX long value for the -1L as there was none) For the tests I'll need more details about how to create the new files and the corresponding expectations if you expect me to add them. |
@rohitjoins Any news? If this kind of approach is not acceptable in it's current form, is there an easy way to handle this on my side in a module and not by maintaining a fork ? |
@Tolriq I apologise for the delay. Will look into this next week. If you want to speed up the process, please consider raising the same PR in https://github.com/androidx/media. Please raise any future PRs in the media repository as well. Thanks. |
@rohitjoins No problem but as said this was first for discussion and tuning before anything else. I'm in prod with this in a fork, but it requires some improvements to really be merged I suppose. About media 3, I do not have plans to migrate currently as everything it brings are not things I want nor want to use. I really hope exoplayer can still be used as a single library without forced dependencies on the rest. I have my fork of media router and many other custom things that does not fit the "media3" architecture. |
if (VorbisUtil.verifyVorbisHeaderCapturePattern(/* headerType= */ 0x03, scratch, /* quiet= */ true)) { | ||
scratch.setPosition(scratch.getPosition() - 7); | ||
commentHeader = VorbisUtil.readVorbisCommentHeader(scratch); | ||
} |
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.
Based on the spec, the Ogg stream consists of three header packets which are in order.
Do we need to do this also for vorbisIdHeader
? Based on my understanding, I think we skip pages not belonging to vorbis stream in OggPageHeader
and we don't need to do any changes here. WDYT?
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.
Yes the page is skipped before if needed for the vorbisIdHeader so no need for change. If it's not present at that moment it's an error.
Previously it was assumed that the comment was in the same page and directly read, but the spec does not enforce that and I had multiple files during my tests that had the comment on the next page.
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.
We read one packet at a time when calling this method. And as these packets are in order, I expect this to work as intended. Can you please share some examples where the comment header is present in the next page and we are not able to read it based on the current implementation?
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.
The sample file from the OG issue have the case. As explained you mix packet and page. The packets are put in pages. But the pages can be anysize and interleaved specially for those multi tracks.
So you have [Stream1-Page1][Header][Packet1][Stream2-Page1][Header][Packet1][Stream1-Page2][Header][Packet2][Stream2-Page2][Header][Packet2]
Packet 1 and packet 2 are in order and adjoined as the spec define. But the actual bytes are not at all, there's the other stream + the page header between them.
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 @Tolriq,
There are few minor issues and some major in this PR. I would have fixed the minor comments changes if everything else worked as expected.
Also this PR requires extensive testing in my opinion. You can create some tests by extracting headers from the sample multi-stream media files and check if we can read the correct first supported header from it. One other test would be too see that we only read packets for the streamSerialNumber we derived and ignore the rest. And lastly, it would be nice to add some multi-stream ogg media files and check if can be played successfully.
As the issue to support multi stream in Ogg is a low priority for us now, we cannot afford to spend so much time in merging this PR at this moment in time. Please address all the comments and add tests for this to be merged.
@@ -59,6 +61,10 @@ | |||
|
|||
private final ParsableByteArray scratch = new ParsableByteArray(MAX_SEGMENT_COUNT); | |||
|
|||
public OggPageHeader(long streamSerialNumber) { | |||
expectedStreamSerialNumber = streamSerialNumber; | |||
} |
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.
Should we create both parameterised and default constructors here? The default constructor can call the parameterised one with default value for streamSerialNumber. The default value can be C.INDEX_UNSET or create a new constant if wanted.
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.
Javadoc is missing.
@@ -69,6 +69,7 @@ | |||
*/ | |||
public DefaultOggSeeker( | |||
StreamReader streamReader, | |||
long streamSerialNumber, |
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.
We won't need to change the signature of this function if we can make streamSerialNumber as public member of StreamReader class.
public FlacReader(long streamSerialNumber) { | ||
super(streamSerialNumber); | ||
} | ||
|
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.
Javadoc is missing for the constructor.
} | ||
try { | ||
input.skip((int) input.getPeekPosition()); | ||
header.skipToNextPage(input); |
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.
Based on the documentation of Extractor.sniff(ExtractorInput input), we can only modify the reading position when returning true from this method. If false is returned only peek position is modified.
Here we are changing the read position by calling skip on input and may still return false.
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.
This one is the only one a little problematic :(
While it's relatively easy to build a PeekExtractorInput wrapper, I could imagine an attack vector of a on purpose wrong ogg file with hundreds of fake streams to oom ExoPlayer.
It would probably need some limits to be defined to address that, either in the buffer size of the wrapper or in the number of attempted track tested. WTDY?
private final OggPageHeader pageHeader; | ||
public OggPacket(long streamSerialNumber) { | ||
pageHeader = new OggPageHeader(streamSerialNumber); | ||
} |
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.
Javadoc is missing. Also this constructor should be moved after all variables are declared.
public OpusReader(long streamSerialNumber) { | ||
super(streamSerialNumber); | ||
} | ||
|
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.
Javadoc is missing.
public VorbisReader(long streamSerialNumber) { | ||
super(streamSerialNumber); | ||
} | ||
|
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.
Same, Javadoc is missing here.
if (VorbisUtil.verifyVorbisHeaderCapturePattern(/* headerType= */ 0x03, scratch, /* quiet= */ true)) { | ||
scratch.setPosition(scratch.getPosition() - 7); | ||
commentHeader = VorbisUtil.readVorbisCommentHeader(scratch); | ||
} |
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.
We read one packet at a time when calling this method. And as these packets are in order, I expect this to work as intended. Can you please share some examples where the comment header is present in the next page and we are not able to read it based on the current implementation?
@@ -48,7 +48,7 @@ public void skipToNextPage_success() throws Exception { | |||
new byte[] {'O', 'g', 'g', 'S'}, | |||
TestUtil.buildTestData(20, random)), |
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.
This test fails based on the changes. As we set the limit now to EMPTY_PAGE_HEADER_SIZE which of size 27. So we should use a number greater than 27 here.
@rohitjoins For the tests I asked about more details about how to create them I do not know how the files are generated and how to reference them from the test data as it fails when just adding here on Windows. As a reminder this is a PR for #10502 that have the file for test. |
Closing all PRs on this deprecated project. We are now unable to merge PRs from here. If you would like us to consider this change again please file a new PR on the media3 project: https://github.com/androidx/media/pulls |
Improve ogg support by supporting multistream container and selecting the first supported.
Properly support comment headers present in a future page.
Properly only decode a single stream when selected by the extractor.