shaka-project / shaka-packager

A media packaging and development framework for VOD and Live DASH and HLS applications, supporting Common Encryption for Widevine and other DRM Systems.
https://shaka-project.github.io/shaka-packager/
Other
1.9k stars 496 forks source link

feat: add startwithSAP/subsegmentstartswithSAP for audio tracks #1346

Closed cosmin closed 4 months ago

cosmin commented 4 months ago

Replaces #1055

This pull request adds startwithSAP/subsegmentstartswithSAP for aac, ac3, ec3 and ac4 audio tracks according to LIVE or VOD profile.

Closes #364

cosmin commented 4 months ago

Puzzled by how that test is failing only on Windows release static.

joeyparrish commented 4 months ago

Puzzled by how that test is failing only on Windows release static.

I see an "actual" MPD containing: <!--Generated with https://github.com/shaka-project/shaka-packager version 00fc734-release-->, but that line is missing from "expected".

Perhaps the mechanism for stripping that is failing on a particular build.

joeyparrish commented 4 months ago

I'm not certain, but it looks like OnDemandMpdBuilderTest is missing a call to SetPackagerVersionForTesting(). This writes to a global, so the test order may matter. If an earlier test called it, then OnDemandMpdBuilderTest may be benefiting from the earlier value. And the test order may have to do with the order of how things are linked, which may explain how a different build configuration could impact this.

joeyparrish commented 4 months ago

I'm going to test this theory about the failed test by running individual suites in separate invocations using a filter on the command-line. If I'm right, I should be able to trigger a failure on any platform and build config using a filter. This can also help hunt down other missing calls to SetPackagerVersionForTesting().

joeyparrish commented 4 months ago

I was wrong. First, I overlooked this line: xml_compare.cc:128] Attributes of element AdaptationSet do not match.

Second, xml_compare.cc seems not to care about comments or about the order of attributes.

The actual difference is:

Expected:

...
<AdaptationSet contentType="video" maxWidth="720" maxHeight="480" maxFrameRate="10/1" id="0">
...

Actual:

...
<AdaptationSet contentType="video" maxWidth="720" maxHeight="480" subsegmentStartsWithSAP="1" maxFrameRate="10/1" id="0">
...

That was very hard to spot, but actual has subsegmentStartsWithSAP="1", and expected does not.

joeyparrish commented 4 months ago

One of the failing tests is OnDemandMpdBuilderTest.TwoVideosWithDifferentResolutions, which compares output against kFileNameExpectedMpdOutputVideo1And2 which is video_media_info1and2_expected_mpd_output.txt. That file does not mention subsegmentStartsWithSAP... so how is the test passing?

I'll look into xml_compare more deeply.

joeyparrish commented 4 months ago

I added some logs to the comparison and just ran the one test on Linux. When it passes, I see that the actual MPD is missing subsegmentStartsWithSAP:

<AdaptationSet contentType="video" maxWidth="720" maxHeight="480" maxFrameRate="10/1" id="0">

This leads me to think we have an uninitialized value involved.

joeyparrish commented 4 months ago

packager/mpd/base/adaptation_set.h doesn't initialize the new fields you added. In a release build, where memory is generally not sanitized for you on allocation, they happen to have a starting value that is non-zero.

joeyparrish commented 4 months ago

The test should be fixed now.