1223696 - The MediaElement will not be played after removeTrack() and addTrack() back.

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: test_getUserMedia_stopBuffering_addTrackRemoveTrack.html

After remove the VideoTrack and added it back, the MediaElement will not be played. Please see the attached test case.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1223696 - Add mochitest. r?

Bug 1223696 - Add mochitest. r?

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1223696 - Make canvas captureStream helper resilient to exceptions when there's no video. r?

Bug 1223696 - Make canvas captureStream helper resilient to exceptions when there's no video. r?

Comment 3

[Andreas Pehrson [:pehrsons]]

roc, do you care to continue debugging this?

With the patches from mozreview it's pretty clear what's going wrong.

I can see that ImageContainer::SetCurrentImageInternal is being called with the new image every time the canvas gets drawn to, but ImageContainer::NotifyCompositeInternal never gets called. 

ImageContainer::mFrameIDsNotYetComposited just keeps growing.

This is too deep in the gfx stack for me, I don't know where to look :-)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can, but would you mind updating your patch to apply on top of bug 1219711 having landed?

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8685948 [details]
MozReview Request: Bug 1223696 - Add mochitest. r?

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24929/diff/1-2/

Comment 6

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8685950 [details]
MozReview Request: Bug 1223696 - Make canvas captureStream helper resilient to exceptions when there's no video. r?

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24931/diff/1-2/

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #3)
> I can see that ImageContainer::SetCurrentImageInternal is being called with
> the new image every time the canvas gets drawn to, but
> ImageContainer::NotifyCompositeInternal never gets called. 
> 
> ImageContainer::mFrameIDsNotYetComposited just keeps growing.

Video frames are not being composited because the video element is not in the DOM.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

HTMLMediaElement::MetadataLoaded has this:
  // If this element had a video track, but consists only of an audio track now,
  // delete the VideoFrameContainer. This happens when the src is changed to an
  // audio only file.
  // Else update its dimensions.
  if (!aInfo->HasVideo()) {
    ResetState();
  } else {
    mWatchManager.ManualNotify(&HTMLMediaElement::UpdateReadyStateInternal);
  }

We're hitting that, which disconnects the VideoFrameContainer/ImageContainer the stream is supposed to play into.

When nsLayoutUtils::SurfaceFromElement is called later, to draw video frames into the canvas, we construct a new VideoFrameContainer with a different ImageContainer, which does not receive frames.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Disconnecting the VideoFrameContainer like this goes back to bug 945475.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bug 804875 is also involved.

I don't think that resetting state in MetadataLoaded here is necessary, at least not anymore. If we changed the 'src' as the comment says, then ResetState should have happened when we started loading the new resource.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

https://hg.mozilla.org/mozilla-central/file/4294bf91174b71ed7440dc491dac5d15394ec227/dom/html/HTMLMediaElement.cpp#l865

We already do that in HTMLMediaElement::DoLoad().

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

jwwang: right.

So if I fix that, getPixel's drawImage() seems to work fine. But CanvasRenderingContext2D::GetImageData is not called, which seems totally inexplicable.

Comment 13

[Andreas Pehrson [:pehrsons]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> CanvasRenderingContext2D::GetImageData is not called, which seems totally
> inexplicable.

There are no "timeupdate" events?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, it's because the mochitest calls waitForPixel incorrectly. It should be calling waitForPixelColor.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1223696 - Make canvas captureStream helper resilient to exceptions when there's no video. r=roc

Bug 1223696 - Make canvas captureStream helper resilient to exceptions when there's no video. r=roc

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1223696. Don't destroy VideoFrameContainer when we reach MetadataLoaded without a video track. r=jwwang

Bug 1223696. Don't destroy VideoFrameContainer when we reach MetadataLoaded without a video track. r=jwwang

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8688224 [details]
MozReview Request: Bug 1223696 - Make canvas captureStream helper resilient to exceptions when there's no video. r=roc

https://reviewboard.mozilla.org/r/25311/#review22817

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8688225 [details]
MozReview Request: Bug 1223696. Don't destroy VideoFrameContainer when we reach MetadataLoaded without a video track. r=jwwang

https://reviewboard.mozilla.org/r/25313/#review22823

::: dom/html/HTMLMediaElement.cpp
(Diff revision 1)
> -  if (!aInfo->HasVideo()) {

Won't this regress bug 945475?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think so. From that bug:

> The correct root cause is as follows:
> 1. 'src' is changed from a video to an audio file in the test
> 2. VideoFrameContainer::Reset => |mIntrinsicSize| = (-1,-1)
> 3. MediaDecoderStateMachine::RenderVideoFrame => VideoFrameContainer::SetCurrentFrame => |mIntrinsicSizeChanged| = true
> 4. MediaDecoder::MetadataLoaded => MediaDecoder::Invalidate => VideoFrameContainer::InvalidateWithFlags => HTMLMediaElement::UpdateMediaSize

On trunk, between steps 1 and 3, HTMLMediaElement::DoLoad will have called HTMLMediaElement::ResetState, so the VideoFrameContainer will already have been disconnected.

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Makes sense to me. I wonder why I had to call |mVideoFrameContainer->ForgetElement()| in MetadataLoaded() in bug 945475.

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8688225 [details]
MozReview Request: Bug 1223696. Don't destroy VideoFrameContainer when we reach MetadataLoaded without a video track. r=jwwang

https://reviewboard.mozilla.org/r/25313/#review22825

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

(In reply to JW Wang [:jwwang] from comment #20)
> Makes sense to me. I wonder why I had to call
> |mVideoFrameContainer->ForgetElement()| in MetadataLoaded() in bug 945475.

Ok, I recall something. I wanted |mVideoFrameContainer->ForgetElement()| always to be called before |mVideoFrameContainer = nullptr| for consistency.

The point is we want to save some memory by deleting the video frame container when finding out that there is no video tracks in metadata. This bug kinda defeat the purpose however I think this is a good thing because it simplify the logic of the media element.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/162ded2e49c8bbecc48c8943faa36506b297d34e
Bug 1223696. Don't destroy VideoFrameContainer when we reach MetadataLoaded without a video track. r=jwwang

Comment 24

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=17673854&repo=mozilla-inbound

Comment 25

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e722799fa71f

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Urgh, I forgot to land the "Make canvas captureStream helper resilient" patch.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/329a4087f6d77aee8f757359124ee895f264877b
Bug 1223696 - Make canvas captureStream helper resilient to exceptions when there's no video. r=roc

https://hg.mozilla.org/integration/mozilla-inbound/rev/820e0ae7a42482aaefce430593ee66818ec1efdc
Bug 1223696. Don't destroy VideoFrameContainer when we reach MetadataLoaded without a video track. r=jwwang

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/329a4087f6d7
https://hg.mozilla.org/mozilla-central/rev/820e0ae7a424

Comment 29

[Eric Shepherd [:sheppy]]

Added a note to Firefox 45 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/45#DOM_HTML_DOM