Chia-hung Tai [:ctai :ctai_mozilla :cht] Reporter
	Description • 9 years ago

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

	Andreas Pehrson [:pehrsons]
	Comment 1 • 9 years ago

Bug 1223696 - Add mochitest. r?

	Andreas Pehrson [:pehrsons]
	Comment 2 • 9 years ago

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

	Andreas Pehrson [:pehrsons]
	Comment 3 • 9 years ago

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 :-)

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 4 • 9 years ago

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

	Andreas Pehrson [:pehrsons]
	Comment 5 • 9 years ago

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/

	Andreas Pehrson [:pehrsons]
	Comment 6 • 9 years ago

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/

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 7 • 9 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 8 • 9 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 9 • 9 years ago

Disconnecting the VideoFrameContainer like this goes back to bug 945475.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 10 • 9 years ago

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.

	JW Wang [:jwwang] [:jw_wang]
	Comment 11 • 9 years ago

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

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

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 12 • 9 years ago

jwwang: right.

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

	Andreas Pehrson [:pehrsons]
	Comment 13 • 9 years ago

(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?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 14 • 9 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 15 • 9 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 16 • 9 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 17 • 9 years ago

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

	JW Wang [:jwwang] [:jw_wang]
	Comment 18 • 9 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 19 • 9 years ago

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.

	JW Wang [:jwwang] [:jw_wang]
	Comment 20 • 9 years ago

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

	JW Wang [:jwwang] [:jw_wang]
	Comment 21 • 9 years ago

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

	JW Wang [:jwwang] [:jw_wang]
	Comment 22 • 9 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 23 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 24 • 9 years ago

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

	Pulsebot
	Comment 25 • 9 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 26 • 9 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 27 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 28 • 9 years ago
bugherder

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

	Eric Shepherd [:sheppy]
	Comment 29 • 8 years ago

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