Only do audio capturing for MediaElement with audio track

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

Other Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Fork from bug1262053 comment81.

The test case "test_getUserMedia_audioCapture.html" would be broke if we don't capture the MediaElement without audio  track.

The fail reason is we do audio capturing too early before we have metadata, so that we won't generate the correct output stream by [1].

[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#2576
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8786197 - Flags: review?(jwwang)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8786197 [details]
Bug 1298777 - don't need to capture audio for media element without audio track.

https://reviewboard.mozilla.org/r/75158/#review73442

::: dom/html/HTMLMediaElement.cpp:4312
(Diff revision 1)
>  #endif // MOZ_EME
>                   ;
>    mTags = aTags.forget();
>    mLoadedDataFired = false;
>    ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_METADATA);
> +  SetMediaInfo(*aInfo);

It looks like we should decouple AudioCaptureStreamChangeIfNeeded() from SetMediaInfo() because it depends on not only mMediaInfo but also the readyState.
Attachment #8786197 - Flags: review?(jwwang) → review-
Priority: -- → P3
(Assignee)

Comment 3

2 years ago
(In reply to JW Wang [:jwwang] from comment #2)
> It looks like we should decouple AudioCaptureStreamChangeIfNeeded() from
> SetMediaInfo() because it depends on not only mMediaInfo but also the
> readyState.

Since we need to call AudioCaptureStreamChangeIfNeeded() when mMediaInfo changed, I think we still bind them together. Otherwise, we need to call AudioCaptureStreamChangeIfNeeded() everytime after the line modifying the mMediaInfo.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8786197 - Flags: feedback?(pehrson)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8786197 - Flags: feedback?(pehrson)
(Assignee)

Comment 6

2 years ago
The fail of the "test_getUserMedia_audioCapture.html" has been solved in bug1299451, thanks Andreas!
Depends on: 1299451
Comment hidden (mozreview-request)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1299451
(Assignee)

Comment 9

2 years ago
This issue isn't equal to bug1299451 ;)
The main purpose of this bug is to avoid capturing media element without audio.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Ah, my bad sorry. Is it MediaDecoder specific then? Since MediaStream is very dynamic on which tracks are available.
(Assignee)

Comment 11

2 years ago
Yes, the test case is about the MediaDecoder. 

Before you fixed the bug1299451, I got the problem that the captureStreamInternal() would be called before metadata loaded, and the media stream doesn't be set correctly.

But now the problem doesn't happen anymore, so the patch here is very simple, just check the HasAudio() in AudioCaptureStreamChangeIfNeeded() to avoid capturing the media element without audio track.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8786197 [details]
Bug 1298777 - don't need to capture audio for media element without audio track.

https://reviewboard.mozilla.org/r/75158/#review74702
Attachment #8786197 - Flags: review?(jwwang) → review+
(In reply to Alastor Wu [:alwu] from comment #3)
> (In reply to JW Wang [:jwwang] from comment #2)
> > It looks like we should decouple AudioCaptureStreamChangeIfNeeded() from
> > SetMediaInfo() because it depends on not only mMediaInfo but also the
> > readyState.
> 
> Since we need to call AudioCaptureStreamChangeIfNeeded() when mMediaInfo
> changed, I think we still bind them together. Otherwise, we need to call
> AudioCaptureStreamChangeIfNeeded() everytime after the line modifying the
> mMediaInfo.

It looks like the problem of ready state is fixed by bug 1299451 so SetMediaInfo() doesn't need to depend on the ready state anymore.

Comment 15

2 years ago
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8812855b1134
don't need to capture audio for media element without audio track. r=jwwang

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8812855b1134
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.