Closed Bug 1298777 Opened 5 years ago Closed 5 years ago

Only do audio capturing for MediaElement with audio track

Categories

(Core :: Audio/Video: Playback, defect, P3)

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(1 file)

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
Attachment #8786197 - Flags: review?(jwwang)
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-
(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.
Attachment #8786197 - Flags: feedback?(pehrson)
Attachment #8786197 - Flags: feedback?(pehrson)
The fail of the "test_getUserMedia_audioCapture.html" has been solved in bug1299451, thanks Andreas!
Depends on: 1299451
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1299451
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.
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 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.
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
https://hg.mozilla.org/mozilla-central/rev/8812855b1134
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.