Support variable length sgpd boxes in fMPEG#3243
Conversation
| "Variable length description in sgpd found (unsupported)"); | ||
| } | ||
| } else if (sgpdVersion >= 2) { | ||
| long default_length = sgpdVersion >= 1 ? sgpd.readUnsignedInt() : 0; |
There was a problem hiding this comment.
i don't think the >= 1 check here is correct - I think it should be == 1.
Looking at section 8.9.3.2 of ISO 14496-12:2015 it seems default_length is present at v1 only, and the same 4 bytes are default_sample_description_index on v2+:
if (version==1) { unsigned int(32) default_length; }
if (version>=2) {
unsigned int(32) default_sample_description_index;
}
This also matches the previous code.
Same below on the condition gating the description_length skip.
There was a problem hiding this comment.
I have the 2022 and 2024 DIS versions of ISO 14496-12 here, and those both have the following:
aligned(8) class SampleGroupDescriptionBox ()
extends FullBox('sgpd', version, flags){
unsigned int(32) grouping_type;
if (version>=1) { unsigned int(32) default_length; }
if (version>=2) {
unsigned int(32) default_group_description_index;
}
unsigned int(32) entry_count;
int i;
for (i = 1 ; i <= entry_count ; i++){
if (version>=1) {
if (default_length==0) {
unsigned int(32) description_length;
}
}
SampleGroupDescriptionEntry (grouping_type);
// an instance of a class derived from SampleGroupDescriptionEntry
// that is appropriate and permitted for the media type
}
}
I.e. default_length exists for version>=1, and default_group_description_index exists for version>=2. This is also how we write sgpd boxes in our own software.
There was a problem hiding this comment.
Interesting, thanks - aren't these two versions of the spec binary-incompatible with each other? My understanding was that this was generally avoided in these sort of specs. Was this an (unacknowledged?) bug in the 2012 to 2020 versions of the spec?
Given this is quite a fiddly change, and there's a risk that it's quite hard to find this PR comment thread in future when someone is going through the blame layer, I'm going to send a change that pretty much only changes == 1 to >= 1 with an explanation - and then we can rebase this PR on top - hope that's OK.
| } | ||
|
|
||
| @Test | ||
| public void extract_h264WithVariableLengthSgpdBox() throws Exception { |
There was a problem hiding this comment.
Does this test fail for you without the fix? It seems to pass for me
To show this, I checked out this PR, then ran:
$ git checkout HEAD^ -- libraries/extractor/src/main/java/androidx/media3/extractor/mp4/FragmentedMp4Extractor.java
And then ran the tests in this file, and they all passed.
There was a problem hiding this comment.
Hm, I see what is happening; the parseSampleGroups() function scans only for seig grouping types, not roll ones. When it doesn't find any seig grouping type, it does an early return, and doesn't hit the sgpd box parsing code at all.
I had troubles getting any test to work with seig boxes, which is why the sample_fragmented_variable_length_sgpd.mp4 file has another grouping type roll, e.g. MP4Box output:
<SampleGroupBox Size="28" Type="sbgp" Version="0" Flags="0" Specification="p12" Container="stbl traf" grouping_type="roll">
<SampleGroupBoxEntry sample_count="94" group_description_index="1" group_description_in_traf="1"/>
</SampleGroupBox>
<SampleGroupDescriptionBox Size="30" Type="sgpd" Version="1" Flags="0" Specification="p12" Container="stbl traf" grouping_type="roll" default_length="0">
<RollRecoveryEntry roll_distance="-1"/>
</SampleGroupDescriptionBox>I'll try this again with a file with a seig box, but previously I got a deduplicateConsecutiveFormats=false so TrackOutput must receive at least one sampleMetadata() call between format() calls. error on that. I had no idea what to do to work around that other error. Maybe I have to supply some DRM metadata somewhere to make such a test work?
If you get stuck on this again, please can you add the file that causes the error to the PR so I can have a play with it too? |
dimitry-unified-streaming
force-pushed
the
fix-variable-length-sgpd-main-1
branch
from
faa3e25 to
6a8d81f
Compare
|
I replaced the <SampleGroupBox Size="28" Type="sbgp" Version="0" Flags="0" Specification="p12" Container="stbl traf" grouping_type="seig">
<SampleGroupBoxEntry sample_count="48" group_description_index="1" group_description_in_traf="1"/>
</SampleGroupBox>
<SampleGroupDescriptionBox Size="65" Type="sgpd" Version="1" Flags="0" Specification="p12" Container="stbl traf" grouping_type="seig" default_length="0">
<CENCSampleEncryptionGroupEntry IsEncrypted="1" IV_size="0" KID="0x804EA17317113C8B955B35996E5E3D92" constant_IV_size="16" constant_IV="0xDCC29B585406F0F0031C7D01C020C321"/>
</SampleGroupDescriptionBox>However, as I indicated, this file leads to a different exception, which I do not know how to solve or work around: |
|
Thanks - I had a play with the file you provided and concluded that the duplicate format is expected because the |
In the 2012 to 2020 versions of the spec, the binary format was
documented as:
```
if (version==1) { unsigned int(32) default_length; }
if (version>=2) {
unsigned int(32) default_sample_description_index;
}
```
While in the 2022 spec this has changed (in a binary incompatible
way!) to:
```
if (version>=1) { unsigned int(32) default_length; }
if (version>=2) {
unsigned int(32) default_group_description_index;
}
```
The later specs take precedence, so this change updates our parsing
code to follow the 2022 version.
This is a precursor to Issue: #3243 (where the mismatch was
[discovered/discussed](#3243 (comment))).
PiperOrigin-RevId: 929815518
For `sgpd` boxes with version >= 1, `the default_length` field is zero if the length of the following sample group entries is variable. Since the `FragmentedMp4Extractor` only supports one entry, skip the `description_length` field if applicable (e.g. when version >= 1 and `default_length` == 0), instead of throwing an error. For parsing the following CencSampleEncryptionInformationGroupEntry, this should make no difference. Issue: androidx#3177
… an actual seig box.
icbaker
force-pushed
the
fix-variable-length-sgpd-main-1
branch
3 times, most recently
from
1999196 to
5f6c076
Compare
…pdate dump files Also add a release note and fix a local variable name
icbaker
force-pushed
the
fix-variable-length-sgpd-main-1
branch
from
5f6c076 to
fdcea3a
Compare
|
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! |
|
Thanks for the updates and rebasing. I have tested your updates here locally, and they seem to all be successful. |
2 participants
For
sgpdboxes with version >= 1,the default_lengthfield is zero if the length of the following sample group entries is variable.Since the
FragmentedMp4Extractoronly supports one entry, skip thedescription_lengthfield if applicable (e.g. when version >= 1 anddefault_length== 0), instead of throwing an error.For parsing the following CencSampleEncryptionInformationGroupEntry, this should make no difference.
Issue: #3177