Closed
Bug 1419627
Opened 3 years ago
Closed 3 years ago
Migrate mp4parse-rust to cbindgen
Categories
(Core :: Audio/Video: Playback, enhancement, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: kinetik, Assigned: kinetik)
Details
Attachments
(2 files)
rillian and nox have migrated mp4parse-rust to cbindgen: https://github.com/mozilla/mp4parse-rust/pull/127 This requires changes on the Gecko side as the resulting mp4parse.h is slightly different to the old moz-cheddar version.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•3 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6769ad101603b91b94bda65cb9c2e63972415b5e
Assignee | ||
Comment 4•3 years ago
|
||
Note before landing: media/mp4parse-rust/update-rust.sh needs to be updated to the appropriate VER once PR 127 is merged.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Comment 5•3 years ago
|
||
mozreview-review |
Comment on attachment 8930777 [details] Bug 1419627 - Update mp4parse-rust callers in cheddar->cbindgen migration. https://reviewboard.mozilla.org/r/201856/#review207184 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/media/mp4/MP4Metadata.cpp:43 (Diff revision 1) > > protected: > - UniquePtr<mp4parse_byte_data> mIndice; > + UniquePtr<Mp4parseByteData> mIndice; > }; > > -IndiceWrapperRust::IndiceWrapperRust(mp4parse_byte_data& aRustIndice) > +IndiceWrapperRust::IndiceWrapperRust(Mp4parseByteData& aRustIndice) Error: Bad implicit conversion constructor for 'indicewrapperrust' [clang-tidy: mozilla-implicit-constructor] ::: dom/media/mp4/MP4Metadata.cpp:43 (Diff revision 1) > > protected: > - UniquePtr<mp4parse_byte_data> mIndice; > + UniquePtr<Mp4parseByteData> mIndice; > }; > > -IndiceWrapperRust::IndiceWrapperRust(mp4parse_byte_data& aRustIndice) > +IndiceWrapperRust::IndiceWrapperRust(Mp4parseByteData& aRustIndice) Error: Bad implicit conversion constructor for 'indicewrapperrust' [clang-tidy: mozilla-implicit-constructor]
Assignee | ||
Comment 6•3 years ago
|
||
I'll deal with those defects (if they still exist) when I rebase on top of bug 1418868 before landing.
Comment 7•3 years ago
|
||
mozreview-review |
Comment on attachment 8930777 [details] Bug 1419627 - Update mp4parse-rust callers in cheddar->cbindgen migration. https://reviewboard.mozilla.org/r/201856/#review207512 Looks good. Thanks for taking this on! ::: dom/media/mp4/DecoderData.h:16 (Diff revision 1) > #include "mozilla/Types.h" > #include "mozilla/Vector.h" > #include "nsString.h" > #include "nsTArray.h" > #include "nsString.h" > +#include "mp4parse.h" Extra parse time of the include should be much for C headers, and it's nicer to not have to duplicate forward declaration for generated code. Seems right to me.
Attachment #8930777 -
Flags: review?(giles) → review+
Comment 8•3 years ago
|
||
mozreview-review |
Comment on attachment 8930778 [details] Bug 1419627 - Update mp4parse-rust to cbindgen version. https://reviewboard.mozilla.org/r/201858/#review207514
Attachment #8930778 -
Flags: review?(giles) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•3 years ago
|
||
mozreview-review |
Comment on attachment 8930777 [details] Bug 1419627 - Update mp4parse-rust callers in cheddar->cbindgen migration. https://reviewboard.mozilla.org/r/201856/#review207630 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: dom/media/mp4/MP4Metadata.cpp:28 (Diff revision 2) > using mozilla::media::TimeUnit; > > namespace mozilla { > LazyLogModule gMP4MetadataLog("MP4Metadata"); > > -IndiceWrapper::IndiceWrapper(mp4parse_byte_data& aIndice) > +IndiceWrapper::IndiceWrapper(Mp4parseByteData& aIndice) Error: Bad implicit conversion constructor for 'indicewrapper' [clang-tidy: mozilla-implicit-constructor] ::: dom/media/mp4/MP4Metadata.cpp:28 (Diff revision 2) > using mozilla::media::TimeUnit; > > namespace mozilla { > LazyLogModule gMP4MetadataLog("MP4Metadata"); > > -IndiceWrapper::IndiceWrapper(mp4parse_byte_data& aIndice) > +IndiceWrapper::IndiceWrapper(Mp4parseByteData& aIndice) Error: Bad implicit conversion constructor for 'indicewrapper' [clang-tidy: mozilla-implicit-constructor]
Assignee | ||
Comment 12•3 years ago
|
||
(In reply to Static Analysis Bot [:clangbot] from comment #11) > Error: Bad implicit conversion constructor for 'indicewrapper' [clang-tidy: > mozilla-implicit-constructor] This just seems wrong. Firstly, it's reporting the same thing twice. Secondly, IndiceWrapper's ctor is already explicit in the declaration in MP4Metadata.h. "Linux x64 opt static-analysis-linux64-st-an/opt (S)" was green on my last try push, so I'm just going to ignore this.
Assignee | ||
Comment 13•3 years ago
|
||
mozreview-review |
Comment on attachment 8930777 [details] Bug 1419627 - Update mp4parse-rust callers in cheddar->cbindgen migration. https://reviewboard.mozilla.org/r/201856/#review207182 ::: dom/media/mp4/DecoderData.h (Diff revision 1) > > -extern "C" { > -typedef struct mp4parse_track_info mp4parse_track_info; > -typedef struct mp4parse_track_audio_info mp4parse_track_audio_info; > -typedef struct mp4parse_track_video_info mp4parse_track_video_info; > -} FYI: The structs are anonymous in the new version of mp4parse.h and I couldn't find a nice way to forward declare them, so I gave up and included mp4parse.h.
Attachment #8930777 -
Flags: review+
Comment 14•3 years ago
|
||
Pushed by mgregan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7beea3063907 Update mp4parse-rust callers in cheddar->cbindgen migration. r=rillian https://hg.mozilla.org/integration/autoland/rev/987eb1eb9b47 Update mp4parse-rust to cbindgen version. r=rillian
Updated•3 years ago
|
Priority: -- → P3
Comment 15•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7beea3063907 https://hg.mozilla.org/mozilla-central/rev/987eb1eb9b47
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•