Skip to content

Remove empty_sample_boxes. #89

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

Merged
merged 2 commits into from
Apr 17, 2017

Conversation

alfredoyang
Copy link
Contributor

Fragmented file can be recognized from stsc, stco, and stts. There is no requirement to keep empty_sample_boxes anymore.

@alfredoyang
Copy link
Contributor Author

hmm... Travis CI fails at rust nightly build?

@rillian
Copy link
Contributor

rillian commented Apr 13, 2017

The nightly failure is an issue with rustup and the current nightly packaging. See rust-lang/rustup#1062 We can land with nightly failures until that's resolved.

stco.offsets.is_empty() &&
stts.samples.is_empty() => (*fragmented) = true as u8,
_ => {},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think moving all this code to a single location is a good idea. And your implementation here is marvelously functional, but the lines are also very long. I tried several different forms, but couldn't find a better way to handle the nested Options. Maybe just break the line before the if guard in the match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean this way?

    let mut iter = tracks.iter();
    iter.find(|track| track.track_id == Some(track_id)).map_or(mp4parse_status::BAD_ARG, |track| {
        match (&track.stsc, &track.stco, &track.stts) {
            (&Some(ref stsc), &Some(ref stco), &Some(ref stts))
                if stsc.samples.is_empty() && stco.offsets.is_empty() && stts.samples.is_empty() => (*fragmented) = true as u8,
            _ => {},
        };
        mp4parse_status::OK
    })

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that's better I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, PR updated.

@rillian rillian merged commit afb01d9 into mozilla:master Apr 17, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants