1295906 - Move |mVideoFrameContainer->ClearCurrentFrame()| out of MediaFormatReader::ReleaseResources()

Status: RESOLVED FIXED

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

Description

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

VideoFrameContainer should be a concern of playback (MDSM/VideoSink) instead of a reader/decoder.

I will add a MediaSink::ClearVideoFrames() function to do the job.

By doing so, we allow MDSM to enter dormant state more aggressively without making noticeable effects to the user:

1. Enter dormant state when playback reaches the end to reclaim memory and hardware resources. Note we can't clear video frames which is visible to the user.
2. Enter dormant state when playback is paused for a certain amount of time to reclaim memory and hardware resources. Note we can't clear video frames which is visible to the user.
3. Enter dormant state when the document becomes inactive. Note we can clear video frames which is not visible to the user.

Comment 1

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

Since MDSM also holds a VideoFrameContainer, I will move the call to ClearCurrentFrame() to MediaDecoderStateMachine::FinishShutdown().

Comment 2

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

Bug 1295901 has changed the function name.

Comment 3

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

Attachment: Bug 1295906 - Move |mVideoFrameContainer->ClearCurrentFrame()| out of MediaFormatReader::ReleaseResources().

Review commit: https://reviewboard.mozilla.org/r/72422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72422/

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8782212 [details]
Bug 1295906 - Move |mVideoFrameContainer->ClearCurrentFrame()| out of MediaFormatReader::ReleaseResources().

https://reviewboard.mozilla.org/r/72422/#review70896

Comment 5

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

Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6d2cc94bb92
Move |mVideoFrameContainer->ClearCurrentFrame()| out of MediaFormatReader::ReleaseResources(). r=jya

Comment 7

[Iris Hsiao [:ihsiao]]

Sorry had to back this out for M-e10s crashes on Windows 7 debug, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=2364187&repo=autoland#L3321

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35416a1541ae
Backed out changeset d6d2cc94bb92 for M-e10s crashes on Windows 7 debug

Comment 9

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

It is interesting that MediaFormatReader::Shutdown() clears |mVideoFrameContainer| [1] before calling MediaDecoderReader::Shutdown() which calls ReleaseResources() [2]. Because |mVideoFrameContainer| is already cleared, |mVideoFrameContainer->ClearCurrentFrame()| is never called during the shutdown phase.

It is even more interesting that when we really call |mVideoFrameContainer->ClearCurrentFrame()| during the shutdown phase, it crashes!

The crash (comment 7) address is 0xffffffffe5e5e5f1 which is a UAF. Might need to use some helps from gfx guys.

[1] https://hg.mozilla.org/mozilla-central/file/052656fc513c05da969590ac5934abd67271a897/dom/media/MediaFormatReader.cpp#l141
[2] https://hg.mozilla.org/mozilla-central/file/052656fc513c05da969590ac5934abd67271a897/dom/media/MediaDecoderReader.cpp#l523
[3] https://hg.mozilla.org/mozilla-central/file/052656fc513c05da969590ac5934abd67271a897/dom/media/MediaFormatReader.cpp#l1977

Comment 10

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

https://hg.mozilla.org/try/rev/c63516139d86e9ab14238934aadf4719922edba8

This change still got crashes. I am almost sure there is something wrong in gfx so that calling ClearCurrentFrame() during shutdown causes crash.

Comment 11

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

Attachment: Bug 1295906 - Remove the call to |mVideoFrameContainer->ClearCurrentFrame()| from ReleaseResources().

Review commit: https://reviewboard.mozilla.org/r/73984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73984/

Comment 12

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

Comment on attachment 8784643 [details]
Bug 1295906 - Remove the call to |mVideoFrameContainer->ClearCurrentFrame()| from ReleaseResources().

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

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8784643 [details]
Bug 1295906 - Remove the call to |mVideoFrameContainer->ClearCurrentFrame()| from ReleaseResources().

https://reviewboard.mozilla.org/r/73984/#review71980

Comment 14

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

Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/112c00ceea7c
Remove the call to |mVideoFrameContainer->ClearCurrentFrame()| from ReleaseResources(). r=jya

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/112c00ceea7c