Closed Bug 1515658 Opened 1 year ago Closed 1 year ago
Incorrect audio duration is displayed for a blob audio composed of blob slices
[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.
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?
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.
(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
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 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 firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9f8232272e ChannelMediaResource should use the BlobURLs' length when known, r=jya
Please request Beta approval on this when you get a chance.
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
Attachment #9034180 - Flags: approval-mozilla-beta?
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.
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+
You need to log in before you can comment on or make changes to this bug.