Closed Bug 1238420 Opened 4 years ago Closed 4 years ago

Update rust mp4parse to v0.1.6

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

Attachments

(5 files, 1 obsolete file)

Import the current version, which will be tagged 0.1.6.
Note that 0.1.6 has not been tagged yet.  Attachment 0 is currently an import of HEAD.
Attachment #8706213 - Flags: review?(giles)
Attachment #8706214 - Flags: review?(vladan.bugzilla)
Attachment #8706214 - Flags: review?(giles)
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Bump the byteorder crate to the latest release, matching our upstream Cargo.toml.
Attachment #8706511 - Flags: review?(kinetik)
I merged your changes from yesterday and published v0.1.6. Re-run of the update script to pull the newer code.
Attachment #8706212 - Attachment is obsolete: true
Attachment #8706512 - Flags: review?(kinetik)
Comment on attachment 8706213 [details] [diff] [review]
Update mp4parse-rust invocations in MP4Metadata to match CAPI changes.  r=rillian

Review of attachment 8706213 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for hooking up the new code!
Attachment #8706213 - Flags: review?(giles) → review+
Comment on attachment 8706214 [details] [diff] [review]
Report mp4parse-rust errors via Telemetry.  r=rillian,vladan

Review of attachment 8706214 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

::: media/libstagefright/binding/MP4Metadata.cpp
@@ +144,5 @@
>    Telemetry::Accumulate(Telemetry::MEDIA_RUST_MP4PARSE_SUCCESS,
>                          rust_mp4parse_success == 0);
> +  if (rust_mp4parse_success < 0) {
> +    Telemetry::Accumulate(Telemetry::MEDIA_RUST_MP4PARSE_ERROR_CODE,
> +                          -rust_mp4parse_success);

Right, I was wondering if we could submit negative values. I'll make the error returns positive for 0.1.7.
Attachment #8706214 - Flags: review?(giles) → review+
Priority: -- → P2
Comment on attachment 8706511 [details] [diff] [review]
Update byteorder to 0.4.2

Review of attachment 8706511 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/update-rust.sh
@@ +23,4 @@
>  
>  git clone https://github.com/BurntSushi/byteorder _upstream/byteorder
>  pushd _upstream/byteorder
> +git checkout 0.4.2

Patch description says 0.4.3 but the script says 0.4.2?
Attachment #8706511 - Flags: review?(kinetik) → review+
Attachment #8706512 - Flags: review?(kinetik) → review+
Attachment #8706524 - Flags: review?(kinetik) → review+
(In reply to Matthew Gregan [:kinetik] from comment #12)

> Patch description says 0.4.3 but the script says 0.4.2?

Oops, sorry. 0.4.2 is the correct version.

Looks ok on try, so this should be ready to go assuming data collection review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fd28e3105bd
Attachment #8706511 - Attachment description: Update byteorder to 0.4.3 → Update byteorder to 0.4.2
Comment on attachment 8706214 [details] [diff] [review]
Report mp4parse-rust errors via Telemetry.  r=rillian,vladan

Review of attachment 8706214 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +6069,5 @@
>      "kind": "boolean",
>      "description": "(Bug 1220885) Whether the rust mp4 demuxer successfully parsed a stream segment.",
>      "cpp_guard": "MOZ_RUST_MP4PARSE"
>    },
> +  "MEDIA_RUST_MP4PARSE_ERROR_CODE": {

add an alert_emails field so that you get automated notifications when the histogram is about to expire or the distribution of error codes changes (e.g. if one error code starts spiking). False alarms from this system are extremely rare

@@ +6072,5 @@
>    },
> +  "MEDIA_RUST_MP4PARSE_ERROR_CODE": {
> +    "expires_in_version": "50",
> +    "kind": "enumerated",
> +    "n_values": 32,

is there any chance you'll have additional error codes in the future? if so, you should make n_values larger

if the error code is just 5 bits (i.e. 2^5 = 32), then 32 is fine :)

@@ +6076,5 @@
> +    "n_values": 32,
> +    "bug_numbers": [1238420],
> +    "description": "The error code reported when an MP4 parse attempt has failed.",
> +    "cpp_guard": "MOZ_RUST_MP4PARSE"
> +  },

Note that this histogram declaration will only collect Telemetry from the Release channel on an opt-in basis (roughly 1% of Release population has opted into Telemetry)
Attachment #8706214 - Flags: review?(vladan.bugzilla) → review+
Thanks for the review.

(In reply to (PTO Jan 14-15) Vladan Djeric (:vladan) -- please needinfo from comment #14)
> add an alert_emails field so that you get automated notifications when the
> histogram is about to expire or the distribution of error codes changes
> (e.g. if one error code starts spiking). False alarms from this system are
> extremely rare

Done.

> is there any chance you'll have additional error codes in the future? if so,
> you should make n_values larger
> 
> if the error code is just 5 bits (i.e. 2^5 = 32), then 32 is fine :)

It's technically an int32_t, but we've only got 5 error codes so far.  I tried to leave enough space in the histogram for anything we might add in the future.
Blocks: 1161350
No longer blocks: vp9-in-mp4
You need to log in before you can comment on or make changes to this bug.