Closed
Bug 1515658
Opened 5 years ago
Closed 5 years ago
Incorrect audio duration is displayed for a blob audio composed of blob slices
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
VERIFIED
FIXED
mozilla66
People
(Reporter: emilghitta, Assigned: baku)
References
Details
(Keywords: regression)
Attachments
(1 file)
5.89 KB,
patch
|
jya
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
[Affected versions]: Firefox 60.4.0esr (BuildId:20181203164059). Firefox 66.0a1 (BuildId:20181219220049). [Unaffected versions]: Due to bug 1514581 Firefox 64.0 (BuildId:20181206201918). Firefox 65.0b5 (BuildId:20181217180946). [Affected platforms]: Windows 10 64bit macOS 10.14. Ubuntu 16.04 64bit. [Steps to reproduce]: 1. Open https://jsfiddle.net/tzyjr059/ 2. Select a file of audio/wav type. For example, 440Hz 0:05.00 wav found in https://www.mediacollege.com/audio/tone/download/ [Expected result]: The correct audio duration is displayed: 0:00 / 0:05 [Actual result]: The following audio duration is displayed: 13:31:36 / 13:31:36 [Regression range]: This seems to be a regression: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e897e367d3bd489422d86fbdfac54925c18329d2&tochange=3d918ff5d63442d7b88e1b7e9cb03b832bc28fdf I didn't managed to generate a more "precise" pushlog since mozregression keeps skipping builds.
Assignee | ||
Comment 1•5 years ago
|
||
jya, where do we calculate the media size? Could it be that we try to read data synchronously, nsIAsyncInputStream doesn't allow this and we show some default value?
Flags: needinfo?(jyavenard)
Updated•5 years ago
|
Comment 2•5 years ago
|
||
Since this is triaged and has a priority set, marking this fix-optional to remove it from regression triage. Happy to still take a patch in nightly.
Comment 3•5 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #1) > jya, where do we calculate the media size? Could it be that we try to read > data synchronously, nsIAsyncInputStream doesn't allow this and we show some > default value? This is done via MediaResource::GetLength which is a synchronous API. How the length is determine by the underlying medium varies, for http requests it's a combination of Content-Length headers and how much we've read so far. GetLength is supposed to return -1 if the size is unknown, most of the media code then will use the current position as duration or until the last sample is demuxed
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 4•5 years ago
|
||
We know the blobURL's size. We can use it directly for this particular corner case.
Assignee: nobody → amarchesini
Attachment #9034180 -
Flags: review?(jyavenard)
Comment 5•5 years ago
|
||
Comment on attachment 9034180 [details] [diff] [review] blob_media.patch Review of attachment 9034180 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/media/ChannelMediaResource.h @@ +252,5 @@ > MediaCacheStream mCacheStream; > > ChannelSuspendAgent mSuspendAgent; > + > + // The size of the stream if known. Should indicate further. something like: // The size of the stream if known at construction time (such as with blob) as no doubt the next person will wonder why we have so many exceptions related to length @@ +253,5 @@ > > ChannelSuspendAgent mSuspendAgent; > + > + // The size of the stream if known. > + int64_t mKnownStreamLength; this name is confusing IMHO. This is indicate if we know the length at construction time. We already have mFirstReadLength as a workaround for the first length reported by the channel. Can't think of a better name unfortunately. In any case it should be const too.
Attachment #9034180 -
Flags: review?(jyavenard) → review+
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9f8232272e ChannelMediaResource should use the BlobURLs' length when known, r=jya
Comment 7•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9c9f8232272e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment 8•5 years ago
|
||
Please request Beta approval on this when you get a chance.
Flags: qe-verify+
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 9•5 years ago
|
||
Comment on attachment 9034180 [details] [diff] [review] blob_media.patch [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1514581 User impact if declined: Wrong media data length for real OS files when used as blobURLs Is this code covered by automated tests?: No Has the fix been verified in Nightly?: Yes Needs manual test from QE?: Yes If yes, steps to reproduce: See the bug description List of other uplifts needed: none Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): The fix is about using the blob size instead of using -1. It's just about propagating an existing information. String changes made/needed: none
Flags: needinfo?(amarchesini)
Attachment #9034180 -
Flags: approval-mozilla-beta?
Reporter | ||
Comment 10•5 years ago
|
||
This issue is verified fixed using Firefox 66.0a1 (BuildId:20190106214444) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 16.04 64bit. Leaving the qe-verify+ flag until this gets verified in beta as well.
Status: RESOLVED → VERIFIED
Comment 11•5 years ago
|
||
Comment on attachment 9034180 [details] [diff] [review] blob_media.patch [Triage Comment] Fixes a regression caused by bug 1514581 which caused the wrong media data length to be reported in some cases. Approved for 65.0b9.
Attachment #9034180 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 12•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/47a7517154c0
Reporter | ||
Comment 13•5 years ago
|
||
This is verified fixed using Firefox 65.0b9 (BuildId:20190107180200) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 16.04 64bit.
Flags: qe-verify+
Updated•5 years ago
|
Has Regression Range: --- → yes
You need to log in
before you can comment on or make changes to this bug.
Description
•