Closed
Bug 1238420
Opened 9 years ago
Closed 9 years ago
Update rust mp4parse to v0.1.6
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: kinetik, Assigned: kinetik)
References
Details
Attachments
(5 files, 1 obsolete file)
(deleted),
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rillian
:
review+
vladan
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
Import the current version, which will be tagged 0.1.6.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Note that 0.1.6 has not been tagged yet. Attachment 0 is currently an import of HEAD.
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8706213 -
Flags: review?(giles)
Assignee | ||
Updated•9 years ago
|
Attachment #8706214 -
Flags: review?(vladan.bugzilla)
Attachment #8706214 -
Flags: review?(giles)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Comment 6•9 years ago
|
||
Bump the byteorder crate to the latest release, matching our upstream Cargo.toml.
Attachment #8706511 -
Flags: review?(kinetik)
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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 9•9 years ago
|
||
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+
Comment 10•9 years ago
|
||
Attachment #8706524 -
Flags: review?(kinetik)
Comment 11•9 years ago
|
||
Updated•9 years ago
|
Priority: -- → P2
Assignee | ||
Comment 12•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Attachment #8706512 -
Flags: review?(kinetik) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8706524 -
Flags: review?(kinetik) → review+
Comment 13•9 years ago
|
||
(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
Updated•9 years ago
|
Attachment #8706511 -
Attachment description: Update byteorder to 0.4.3 → Update byteorder to 0.4.2
Comment 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
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.
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/075d8bd8b9a50a712e2ab0400eea762238164a6f
Bug 1238420 - Update mp4parse-rust to 0.1.6. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0192610a0e7aee0286e4b45c40fd4b3c357f0cd
Bug 1238420 - Update byteorder to v0.4.2. r=kinetik
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca581e5420d1cffe8a8e68cb302647fa131553c
Bug 1238420 - Update MP4Rust gtest for CAPI changes. r=kinetik.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd565cb21a224657b2211efaea485ec8df7819c
Bug 1238420 - Update mp4parse-rust invocations in MP4Metadata to match CAPI changes. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/33b4a7fd0c53d0034fbc00306fd460207e1ca094
Bug 1238420 - Report mp4parse-rust errors via Telemetry. r=rillian,vladan
Updated•9 years ago
|
Blocks: vp9-in-mp4
Assignee | ||
Updated•9 years ago
|
Comment 17•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/075d8bd8b9a5
https://hg.mozilla.org/mozilla-central/rev/b0192610a0e7
https://hg.mozilla.org/mozilla-central/rev/1ca581e5420d
https://hg.mozilla.org/mozilla-central/rev/6dd565cb21a2
https://hg.mozilla.org/mozilla-central/rev/33b4a7fd0c53
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•