Closed
Bug 1388660
Opened 7 years ago
Closed 7 years ago
Sound icon appears on tab for embedded youtube videos without sound (www.bbva.es)
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
RESOLVED
FIXED
Firefox 57
People
(Reporter: alejandro, Assigned: alwu)
References
(Depends on 1 open bug)
Details
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
jwwang
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
ehsan.akhgari
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
On Nightly 57.0a1 (2017-08-08) (64-bit), if a media such as a Youtube video has no sound (or is muted), the audio indicator in the tab is still shown.
Comment 1•7 years ago
|
||
Tested with https://www.youtube.com/watch?v=apqhpClpETA, and for me when I click the mute button, after a few seconds the icon fades out (the delay is deliberate). Can you post more specific steps, or clarify if you see something else, and if you can reproduce on a clean profile? ( https://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-profiles )
Flags: needinfo?(alexrs95)
Updated•7 years ago
|
Component: General → Tabbed Browser
Reporter | ||
Comment 2•7 years ago
|
||
In this page https://www.bbva.es/particulares/index.jsp that has several Youtube videos with no sound, the icon appears. It's true that in the Youtube webpage it works.
Flags: needinfo?(alexrs95)
Comment 3•7 years ago
|
||
Ehsan, any idea what's happening here?
Blocks: 486262
status-firefox55:
--- → affected
Flags: needinfo?(ehsan)
Summary: Sound icon appears on tab for media without sound → Sound icon appears on tab for embedded youtube videos without sound (www.bbva.es)
Comment 4•7 years ago
|
||
We need baku here, but he's on vacation and has blocked NI and all.
Comment 5•7 years ago
|
||
According to the "stats for nerds" on the videos, they seem to be ~ -69dB, so they're silent. Alastor, do you remember what was the bug number for not showing the icon for silent audio streams? This is a dupe of that bug.
Flags: needinfo?(ehsan) → needinfo?(alwu)
Assignee | ||
Comment 6•7 years ago
|
||
That was bug1238906, but I forgot why I didn't use the threshold to filter out the super small value. I'll take this bug, Thanks!
Assignee: nobody → alwu
Flags: needinfo?(alwu)
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8895653 [details] Bug 1388660 - part1 : AudioData::IsAudible() should return false when its data is super small and near to zero. https://reviewboard.mozilla.org/r/166922/#review172050 ::: dom/media/MediaData.cpp:69 (Diff revision 1) > return false; > } > > for (uint32_t frame = 0; frame < mFrames; ++frame) { > for (uint32_t channel = 0; channel < mChannels; ++channel) { > - if (mAudioData[frame * mChannels + channel] != 0) { > + if (std::fabs(mAudioData[frame * mChannels + channel]) > 1e-7) { Replace 1e-7 with a constant and put comments to explain why this value is chosen.
Attachment #8895653 -
Flags: review?(jwwang) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7950ce109754f4e4ed42cb7d16304e6b140c5763
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8895653 [details] Bug 1388660 - part1 : AudioData::IsAudible() should return false when its data is super small and near to zero. https://reviewboard.mozilla.org/r/166922/#review172140 ::: dom/media/MediaData.cpp:35 (Diff revisions 1 - 2) > > const char* AudioData::sTypeName = "audio"; > const char* VideoData::sTypeName = "video"; > > +bool > +IsDataLoudnessHearable(const float& aData) You can't assume the float type. http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/dom/media/AudioSampleFormat.h#43-52 An AudioDataValue could be either a float or an int16_t.
Attachment #8895653 -
Flags: review+ → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8895653 [details] Bug 1388660 - part1 : AudioData::IsAudible() should return false when its data is super small and near to zero. https://reviewboard.mozilla.org/r/166922/#review172188 ::: dom/media/MediaData.cpp:35 (Diff revision 4) > > const char* AudioData::sTypeName = "audio"; > const char* VideoData::sTypeName = "video"; > > +bool > +IsDataLoudnessHearable(const AudioDataValue& aData) Use pass-by-value for scalars.
Attachment #8895653 -
Flags: review?(jwwang) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
Wonderful, thank you! I think it would be nice if we also added a test for this.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8896868 [details] Bug 1388660 - part2 : add test. https://reviewboard.mozilla.org/r/168156/#review173518 Why remove the test for an audio file with a totally silent audio track? I know that with the current implementation the test basically amounts to the same effect, but the test shouldn't assume anything about the implementation, so that if later we change anything in the implementation which adds a bug where we mishandle the case of a totally silent audio track but not an almost silent audio track it would still catch it. How about having a file_almostSilentAudioTrack.html on the side?
Attachment #8896868 -
Flags: review?(ehsan) → review-
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8896868 [details] Bug 1388660 - part2 : add test. https://reviewboard.mozilla.org/r/168156/#review174022 Thank you!
Attachment #8896868 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 23•7 years ago
|
||
hmm...weird, my test would fail [1] on try server but it won't fail on my local. The error said that I can't get the video element. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd2a1478b76a&selectedJob=123475688
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a618d8c9f37d69d8670a3228ba2aeb975ef6fa57&selectedJob=123763929
Comment 27•7 years ago
|
||
Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/91caacf94f75 part1 : AudioData::IsAudible() should return false when its data is super small and near to zero. r=jwwang https://hg.mozilla.org/integration/autoland/rev/2652d716dd4d part2 : add test. r=Ehsan
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/91caacf94f75 https://hg.mozilla.org/mozilla-central/rev/2652d716dd4d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Assignee | ||
Comment 30•7 years ago
|
||
Comment on attachment 8895653 [details] Bug 1388660 - part1 : AudioData::IsAudible() should return false when its data is super small and near to zero. Approval Request Comment [Feature/Bug causing the regression]: showing audio indicator when media is audible [User impact if declined]: showing audio indicator for media with almost silent audio track, it makes user confused [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: No [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: need to uplift both patch, patch 1 is the solution and patch2 is the test case [Is the change risky?]: None [Why is the change risky/not risky?]: Is only change the way we check audible, and it would only take a effect for the video with almost silent audio track. [String changes made/needed]: No
Flags: needinfo?(alwu)
Attachment #8895653 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox56:
--- → affected
Comment 31•7 years ago
|
||
Comment on attachment 8895653 [details] Bug 1388660 - part1 : AudioData::IsAudible() should return false when its data is super small and near to zero. Fix for sound icon error in tabs, let's uplift to beta.
Attachment #8895653 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 32•7 years ago
|
||
Comment on attachment 8896868 [details] Bug 1388660 - part2 : add test. [Triage Comment]
Attachment #8896868 -
Flags: approval-mozilla-beta+
Comment 33•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a76b057966b7 https://hg.mozilla.org/releases/mozilla-beta/rev/7a1e28ed93cf
Flags: in-testsuite+
Comment 34•7 years ago
|
||
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #30) > [Is this code covered by automated tests?]: Yes > [Has the fix been verified in Nightly?]: No > [Needs manual test from QE? If yes, steps to reproduce]: No Setting qe-verify- based on Alastor's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•