Closed Bug 1388660 Opened 2 years ago Closed 2 years ago

Sound icon appears on tab for embedded youtube videos without sound (www.bbva.es)

Categories

(Firefox :: Tabbed Browser, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: alejandro, Assigned: alwu)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

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.
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)
Component: General → Tabbed Browser
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)
Ehsan, any idea what's happening here?
Blocks: 486262
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)
We need baku here, but he's on vacation and has blocked NI and all.
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)
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 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 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 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+
Wonderful, thank you!

I think it would be nice if we also added a test for this.
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 on attachment 8896868 [details]
Bug 1388660 - part2 : add test.

https://reviewboard.mozilla.org/r/168156/#review174022

Thank you!
Attachment #8896868 - Flags: review?(ehsan) → review+
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
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
https://hg.mozilla.org/mozilla-central/rev/91caacf94f75
https://hg.mozilla.org/mozilla-central/rev/2652d716dd4d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
uplift reminder.
Flags: needinfo?(alwu)
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?
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 on attachment 8896868 [details]
Bug 1388660 - part2 : add test.

[Triage Comment]
Attachment #8896868 - Flags: approval-mozilla-beta+
(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-
Depends on: 1403395
You need to log in before you can comment on or make changes to this bug.