Add support for MPEG-4 Part 2 video codec (ISO 14496-2). #296
Conversation
|
Thanks for the submission. Just to let you know, I'm not going to be able to start reviewing this until next week (Apr 19) at the earliest, but I'll definitely get to it as soon as I can. |
|
I've started looking at this, but I'm not going to be finished until later this week. One thing that would help is to know what your desired use case for the code is. Are you looking to consume video/mp4v-es content via the |
Hi, Many thanks. |
|
It's going to take me a little while to get to this yet. Just wanted to let you know I haven't forgotten. |
Thanks @baumanj ! |
|
Hi @baumanj , Sorry for bothering you. Thanks a lot! |
|
Hey @dreamaniajp. Apologies for the delay, but this is a critical time for my main work. It would be easier to approve this PR if it were written in a way that doesn't affect the existing functionality when the The current patch needs to have conflicts resolved (and probably be rebased). I'd suggest doing that and seeing if it's possible to write your change in a way that doesn't modify the existing functionality. We definitely want this crate to useful for purposes other than Firefox, but in terms of resource allocation, we have to prioritize that use case. If this is the approach you feel makes most sense, perhaps you can use a fork for the time being until I have the opportunity to give this the review it needs. I want to set clear expectations and let you know that it'll still be at least a little while until I can look at this closely enough to determine whether we can accept it in its current form. |
dreamaniajp
force-pushed
the
master
branch
3 times, most recently
from
574f2a5 to
b1e2e1a
Compare
|
Hi @baumanj , Thanks a lot. |
mp4parse/src/lib.rs
Outdated
| esds.audio_codec = match object_profile { | ||
| 0x40 | 0x41 => CodecType::AAC, | ||
| 0x69 | 0x6B => CodecType::MP3, | ||
| _ => CodecType::Unknown, | ||
| }; | ||
|
|
||
| esds.video_codec = match object_profile { | ||
| 0x20..=0x24 => CodecType::MP4V, | ||
| _ => CodecType::Unknown, | ||
| }; | ||
|
|
There was a problem hiding this comment.
This is a logic change to the underlying code which will affect the behavior with the "mp4v" feature disabled. Putting it behind a feature flag (and making L3393–3398 conditional on #[cfg(not(feature = "mp4v"))] obscures the code too much, so I don't think we want to accept this kind of change.
It may well be a good one, but unfortunately we don't have time to invest in the necessary vetting for a change that isn't necessary for us.
There was a problem hiding this comment.
Hi @baumanj ,
I moved this back to the original place to keep the original call flow and committed the change again.
Thanks.
mp4parse/src/lib.rs
Outdated
| esds.audio_codec = match object_profile { | ||
| 0x40 | 0x41 => CodecType::AAC, | ||
| 0x69 | 0x6B => CodecType::MP3, | ||
| _ => CodecType::Unknown, | ||
| }; |
There was a problem hiding this comment.
Is it necessary to move this? Doing so potentially changes the behavior even then the "mp4v" feature is not enabled since it mutates esds before the call to find_descriptor which takes it as a parameter
There was a problem hiding this comment.
Hi @baumanj ,
No, we don't need to move this actually.
I moved this back to the original place to keep the original call flow and committed the change again.
Thanks.
|
Thanks all! |
Hi, This PR tries to fix the MP4V (ISO 14496-2) support.
Could you help to review it?
Many thanks!