Skip to content

Adds support for fragmented mp4 with multiple sidx boxes #2186

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

anthonybajoua
Copy link

@anthonybajoua anthonybajoua commented Feb 25, 2025

Summary:

Addresses Github issue #9373
Implements solution 1 proposed by Google maintainer.

  • Added test. Generated test file using below command:

ffmpeg -i short_1080p_videoonly_lowbitrate.mp4 -c copy -movflags frag_keyframe+delay_moov+empty_moov+dash sample_fragmented_seekable_multi_sidx.mp4 (Above movflags flags actually result in an unplayable file on this player until this fix.)

Algorithm/Solution

  1. Upon encountering multiple sidx atom in track:
  2. For each atom in the track:
    1. If sidx atom then aggregate ChunkIndex into master list, else skip
    2. Merge them all into one large chunk index.
    3. Reset the seek pointer to where it was last
      Retry above on I/O failures.

Copy link

google-cla bot commented Feb 25, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@anthonybajoua anthonybajoua force-pushed the meta/fragmented-mp4-multiple-sidx-seek-squashed branch from 476504f to ef35943 Compare February 25, 2025 18:19
@colinkho
Copy link
Contributor

Hey @tonihei @icbaker , would you guys be able to take a look? This would help us maintain a vanilla fMp4 extractor!

@icbaker
Copy link
Collaborator

icbaker commented Feb 26, 2025

We can take a look once the CLA check is passing.

@oceanjules
Copy link
Contributor

@anthonybajoua Hey, a reminder to complete the CLA instruction if you want us to look at your code :)

@rohitjoins rohitjoins self-assigned this Mar 12, 2025
@anthonybajoua anthonybajoua marked this pull request as ready for review March 25, 2025 21:16
@google-oss-bot

This comment was marked as resolved.

@colinkho
Copy link
Contributor

Seems like the CLA issue is finally resolved. @icbaker Would you be able to take a look?

@Shell1125
Copy link

Details

@rohitjoins rohitjoins force-pushed the meta/fragmented-mp4-multiple-sidx-seek-squashed branch 10 times, most recently from 713b9cd to 2e0d21a Compare April 25, 2025 16:04
@rohitjoins rohitjoins force-pushed the meta/fragmented-mp4-multiple-sidx-seek-squashed branch 3 times, most recently from 8e41a84 to d7534e9 Compare April 29, 2025 15:33
@rohitjoins rohitjoins force-pushed the meta/fragmented-mp4-multiple-sidx-seek-squashed branch from d7534e9 to 8b681a8 Compare April 29, 2025 15:34
@rohitjoins rohitjoins force-pushed the meta/fragmented-mp4-multiple-sidx-seek-squashed branch 2 times, most recently from a0163ee to ca0912d Compare April 29, 2025 16:14
@rohitjoins rohitjoins force-pushed the meta/fragmented-mp4-multiple-sidx-seek-squashed branch from ca0912d to 0d55dd3 Compare May 1, 2025 12:36
}
} finally {
if (seekPositionBeforeSidxProcessing != C.INDEX_UNSET) {
seekPositionBeforeSidxProcessing = C.INDEX_UNSET;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're still investigating the use of finally block here. Extractor should be able to restart from an IOException without having this. Removing this block results in test failures with IO Errors simulation.

Copy link
Author

@anthonybajoua anthonybajoua May 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to the reset of the seek position being set and not cleared during an IO Error.

https://gist.github.com/anthonybajoua/c85a786a30f9876c82849544f848f190

See the above revision for a much cleaner way to achieve this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants