Migrate mp4parse-rust to cbindgen

RESOLVED FIXED in Firefox 59

Status

()

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement
Importance:
P3
normal
Status:
RESOLVED FIXED
Opened:
2 years ago
Updated:
2 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Version:
Trunk
Target:
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(2 attachments)

Bug 1419627 - Update mp4parse-rust callers in cheddar->cbindgen migration.
59 bytes, text/x-review-board-request
rillian
: review+
kinetik
: review+
Details
Bug 1419627 - Update mp4parse-rust to cbindgen version.
59 bytes, text/x-review-board-request
rillian
: review+
Details 
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.
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 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]
I'll deal with those defects (if they still exist) when I rebase on top of bug 1418868 before landing.
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 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 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]
(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.
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+
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
https://hg.mozilla.org/mozilla-central/rev/7beea3063907
https://hg.mozilla.org/mozilla-central/rev/987eb1eb9b47
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.