Closed Bug 1223696 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: ctai, Assigned: roc)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files, 2 obsolete files)

After remove the VideoTrack and added it back, the MediaElement will not be played. Please see the attached test case.
No longer blocks: 910249, 985265, 1208316, 1070216
No longer depends on: 1166140, 1170112, 1170958
Bug 1223696 - Make canvas captureStream helper resilient to exceptions when there's no video. r?
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 :-)
Flags: needinfo?(roc)
I can, but would you mind updating your patch to apply on top of bug 1219711 having landed?
Flags: needinfo?(roc)
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 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/
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
Assignee: nobody → roc
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.
Disconnecting the VideoFrameContainer like this goes back to bug 945475.
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.
jwwang: right.

So if I fix that, getPixel's drawImage() seems to work fine. But CanvasRenderingContext2D::GetImageData is not called, which seems totally inexplicable.
(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?
No, it's because the mochitest calls waitForPixel incorrectly. It should be calling waitForPixelColor.
Bug 1223696 - Make canvas captureStream helper resilient to exceptions when there's no video. r=roc
Attachment #8688224 - Flags: review?(roc)
Bug 1223696. Don't destroy VideoFrameContainer when we reach MetadataLoaded without a video track. r=jwwang
Attachment #8688225 - Flags: review?(jwwang)
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
Attachment #8688224 - Flags: review?(roc) → review+
Attachment #8688225 - Flags: review?(jwwang)
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?
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.
Flags: needinfo?(jwwang)
Makes sense to me. I wonder why I had to call |mVideoFrameContainer->ForgetElement()| in MetadataLoaded() in bug 945475.
Flags: needinfo?(jwwang)
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
(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.
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
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=17673854&repo=mozilla-inbound
Flags: needinfo?(roc)
Urgh, I forgot to land the "Make canvas captureStream helper resilient" patch.
Flags: needinfo?(roc)
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
https://hg.mozilla.org/mozilla-central/rev/329a4087f6d7
https://hg.mozilla.org/mozilla-central/rev/820e0ae7a424
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.