Add support for AMR-NB and AMR-WB.

[dreamaniajp]

Hi,
Since current mp4parse-rust doesn't handle samr, sawb and damr boxes, I tried to add some codes to handle it.
Could you help to review this?

Thanks a lot!

[baumanj]

Thanks for the submission!

In general this repo has been focused on the needs of Firefox, but we also want it to be a generally useful rust crate since mp4 parsing is fairly involved. Unfortunately, we don't have the expertise to review all the potentially desirable standards and we want to minimize the impact on our primary consumer (Firefox) by minimizing the amount of code which isn't necessary for that application.

After discussing with the other maintainers, the compromise we came up with is that we're happy to accept changes like this if they're behind a feature flag. I created https://github.com/mozilla/mp4parse-rust/tree/pr-277 based on this change to show what I think that would look like, but if you think there's a better name for the feature or other things you'd like to do differently than I did there, please feel free. It's just a suggestion for the approach we have in mind.

Let me know if you have any questions and thanks again.

[baumanj]

Given the macro context, it's fine to add these entries without any sort of feature gating, but a comment that these are only used for the 3gpp feature-specific code would be helpful

[dreamaniajp]

Sure, I'll add comment to indicate that these code only used for the 3gpp feature-specific code.
Do I need to file a new PR for this commit?

[baumanj]

Do I need to file a new PR for this commit?

Your PR inspired me to make #279 for use cases like these. Now that it's merged, these entries can be conveniently put behind the feature gate like so:

Suggested change

	AMRNBSampleEntry 0x7361_6d72, // "samr" - AMR narrow-band
	AMRWBSampleEntry 0x7361_7762, // "sawb" - AMR wide-band
	AMRSpecificBox 0x6461_6d72, // "damr"
	#[cfg(feature = "3gpp")]
	AMRNBSampleEntry 0x7361_6d72, // "samr" - AMR narrow-band
	#[cfg(feature = "3gpp")]
	AMRWBSampleEntry 0x7361_7762, // "sawb" - AMR wide-band
	#[cfg(feature = "3gpp")]
	AMRSpecificBox 0x6461_6d72, // "damr"

Please make that change and there's no need for any additional comments. The intent is pretty clear, I think.

You don't need to make a new PR. Please rebase so you can get the changes from #279, then add the other changes I've asked for (in whatever way you like) and update your dreamaniajp:master branch so I can review in this PR.

[dreamaniajp]

Ok, I'll rebase and add the feature gate, thanks!

Update: I saw that you already had the commit about this, thanks!

[dreamaniajp]

Thanks for the submission!

In general this repo has been focused on the needs of Firefox, but we also want it to be a generally useful rust crate since mp4 parsing is fairly involved. Unfortunately, we don't have the expertise to review all the potentially desirable standards and we want to minimize the impact on our primary consumer (Firefox) by minimizing the amount of code which isn't necessary for that application.

After discussing with the other maintainers, the compromise we came up with is that we're happy to accept changes like this if they're behind a feature flag. I created https://github.com/mozilla/mp4parse-rust/tree/pr-277 based on this change to show what I think that would look like, but if you think there's a better name for the feature or other things you'd like to do differently than I did there, please feel free. It's just a suggestion for the approach we have in mind.

Let me know if you have any questions and thanks again.

Hi baumanj, thanks for the reply!

Do you mean the changes on pr-277 branch will not be used by Firefox (I mean https://github.com/mozilla/gecko-dev/tree/master/third_party/rust/mp4parse and https://github.com/mozilla/gecko-dev/tree/master/third_party/rust/mp4parse_capi)?
Will pr-277 branch also be maintained when master branch updating?

For people who have projects base on gecko-dev and also need the features on pr-277 branch , it's a shame that the merging effort of mp4parse-rust could not be reduced if they would like to keep merging the latest version of gecko-dev.
I know this might not be the main concern from Firefox's point of view, but do you have any suggestion for this scenario?

Thanks again!

[baumanj]

Do you mean the changes on pr-277 branch will not be used by Firefox (I mean https://github.com/mozilla/gecko-dev/tree/master/third_party/rust/mp4parse and https://github.com/mozilla/gecko-dev/tree/master/third_party/rust/mp4parse_capi)?
Will pr-277 branch also be maintained when master branch updating?

I created the pr-277 branch merely as an example of one way to put the code in your PR behind a feature so that we can accept it into the main development branch of this repo without having to include code in Firefox which isn't used. Does that make sense?

However, you're the author of this code and I didn't want to assume the way I did it was exactly what you'd want, so feel free to take whatever you like from the pr-277 branch, or do it some other way that makes sense to you. Also, feel free to use a different name for the feature if there's one that you find more appropriate than 3gpp. The only requirement we have is that the code is behind some feature since we don't want to include it in Firefox, which doesn't need this functionality right now.

For people who have projects base on gecko-dev and also need the features on pr-277 branch , it's a shame that the merging effort of mp4parse-rust could not be reduced if they would like to keep merging the latest version of gecko-dev.
I know this might not be the main concern from Firefox's point of view, but do you have any suggestion for this scenario?

The intention is to merge your PR into the main development branch of this repo and eventually the published crate. That way, if someone wants to use it in a fork of geko-dev (or any other rust project) they can enable the 3gpp feature (or whatever you want to call it if you have a name you like better) in the Cargo.toml which adds the mp4parse or mp4parse_capi dependencies.

For gecko-dev in particular, it would be necessary to make some additions to this switch statement to handle the new MP4PARSE_CODEC_AMRNB and MP4PARSE_CODEC_AMRWB cases, but otherwise I think it would just be a matter of turning on the feature in the the mp4parse_capi dependency for gecko.

I tested this out and realized we'll want to have a C/C++ define corresponding to the rust feature for the sake of the generated FFI headers. I've updated the pr-277 branch to show where the change needs to be for cbindgen to generate the appropriate headers, but as I mentioned before, if you want to call the feature something else, feel free.

[baumanj]

Do I need to file a new PR for this commit?

Your PR inspired me to make #279 for use cases like these. Now that it's merged, these entries can be conveniently put behind the feature gate like so:

Suggested change

	AMRNBSampleEntry 0x7361_6d72, // "samr" - AMR narrow-band
	AMRWBSampleEntry 0x7361_7762, // "sawb" - AMR wide-band
	AMRSpecificBox 0x6461_6d72, // "damr"
	#[cfg(feature = "3gpp")]
	AMRNBSampleEntry 0x7361_6d72, // "samr" - AMR narrow-band
	#[cfg(feature = "3gpp")]
	AMRWBSampleEntry 0x7361_7762, // "sawb" - AMR wide-band
	#[cfg(feature = "3gpp")]
	AMRSpecificBox 0x6461_6d72, // "damr"

Please make that change and there's no need for any additional comments. The intent is pretty clear, I think.

You don't need to make a new PR. Please rebase so you can get the changes from #279, then add the other changes I've asked for (in whatever way you like) and update your dreamaniajp:master branch so I can review in this PR.

[baumanj]

Are you still intending to move forward with this @dreamaniajp ? It's waiting on you at this point

[dreamaniajp]

Do you mean the changes on pr-277 branch will not be used by Firefox (I mean https://github.com/mozilla/gecko-dev/tree/master/third_party/rust/mp4parse and https://github.com/mozilla/gecko-dev/tree/master/third_party/rust/mp4parse_capi)?
Will pr-277 branch also be maintained when master branch updating?

I created the pr-277 branch merely as an example of one way to put the code in your PR behind a feature so that we can accept it into the main development branch of this repo without having to include code in Firefox which isn't used. Does that make sense?

However, you're the author of this code and I didn't want to assume the way I did it was exactly what you'd want, so feel free to take whatever you like from the pr-277 branch, or do it some other way that makes sense to you. Also, feel free to use a different name for the feature if there's one that you find more appropriate than 3gpp. The only requirement we have is that the code is behind some feature since we don't want to include it in Firefox, which doesn't need this functionality right now.

For people who have projects base on gecko-dev and also need the features on pr-277 branch , it's a shame that the merging effort of mp4parse-rust could not be reduced if they would like to keep merging the latest version of gecko-dev.
I know this might not be the main concern from Firefox's point of view, but do you have any suggestion for this scenario?

The intention is to merge your PR into the main development branch of this repo and eventually the published crate. That way, if someone wants to use it in a fork of geko-dev (or any other rust project) they can enable the 3gpp feature (or whatever you want to call it if you have a name you like better) in the Cargo.toml which adds the mp4parse or mp4parse_capi dependencies.

For gecko-dev in particular, it would be necessary to make some additions to this switch statement to handle the new MP4PARSE_CODEC_AMRNB and MP4PARSE_CODEC_AMRWB cases, but otherwise I think it would just be a matter of turning on the feature in the the mp4parse_capi dependency for gecko.

I tested this out and realized we'll want to have a C/C++ define corresponding to the rust feature for the sake of the generated FFI headers. I've updated the pr-277 branch to show where the change needs to be for cbindgen to generate the appropriate headers, but as I mentioned before, if you want to call the feature something else, feel free.

Really thanks for such detailed explanation!
Yes, I understand and agree the intention of pr-277 branch.
And I have no concern about this approach.
Thanks a again!

I'll continue to complete this PR, thanks.

[dreamaniajp]

Are you still intending to move forward with this @dreamaniajp ? It's waiting on you at this point

Sorry, I missed previous reminder from github.
I saw the commits you summit on pr-277.
And yes that's all I would like to have, thanks.

I don't have further change to do but only have a question about cargo test:
Does features section in Cargo.toml affect cargo test?
I tried to execute cargo test under mp4parse, it needs cargo test --features "3gpp" to have the "3gpp" feature enabled.

Beside this, I think this PR could be finished and closed.

Thanks.

[baumanj]

I don't have further change to do but only have a question about cargo test:
Does features section in Cargo.toml affect cargo test?
I tried to execute cargo test under mp4parse, it needs cargo test --features "3gpp" to have the "3gpp" feature enabled.

Yes, if you want to test your code you'll either need to run cargo test --features 3gpp from the mp4parse crate, or cargo test --all-features from the workspace root. We won't be running these tests on this repo's CI since we have limited resources and must dedicate them to covering the features Firefox relies on, but you're welcome to set them up on your own CI wherever you like.

Beside this, I think this PR could be finished and closed.

Ok, will do. I'll just merge the pr-277 branch since it's based on your initial work and maintains your authorship there.