Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(356)

Issue 363953002: Oilpan: have the media element safely close its MediaSource. (Closed)

Created:
6 years, 5 months ago by sof
Modified:
6 years, 5 months ago
CC:
blink-reviews, blink-reviews-html_chromium.org, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vcarbune.chromium
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Oilpan: have the media element safely close its MediaSource. The media element must notify its attached MediaSource upon being finalized, as the MediaSource must then transition to a closed state and fire the corresponding event. Hence, upon media element finalization, it will conditionally notify its MediaSource (if any) object if the latter survived the GC. If neither survived, an explicit close isn't needed. With that in place, the media element no longer keeps a RefPtr to its MediaSource (with Oilpan enabled.) Consequently, ref-forwarding over HTMLMediaSource is no longer needed nor supported. R=haraken@chromium.org BUG=340522 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177532

Patch Set 1 #

Patch Set 2 : Style tidying #

Total comments: 10

Patch Set 3 : Tweak assert availability + no need for extra finalization of SourceBuffer #

Total comments: 2

Patch Set 4 : Tidy clearing of m_attachedElement #

Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -7 lines) Patch
M Source/core/html/HTMLMediaElement.h View 1 3 chunks +11 lines, -1 line 0 comments Download
M Source/core/html/HTMLMediaElement.cpp View 1 4 chunks +14 lines, -1 line 0 comments Download
M Source/core/html/HTMLMediaSource.h View 3 chunks +6 lines, -1 line 0 comments Download
M Source/modules/mediasource/MediaSource.h View 3 chunks +4 lines, -2 lines 0 comments Download
M Source/modules/mediasource/MediaSource.cpp View 1 2 3 5 chunks +18 lines, -2 lines 0 comments Download
M Source/modules/mediasource/SourceBuffer.cpp View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M Source/modules/mediasource/SourceBufferList.cpp View 1 2 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
sof
Please take a look. As requested, tidy up the relationship between HTMLMediaElement and (HTML)MediaSource with ...
6 years, 5 months ago (2014-07-02 19:47:58 UTC) #1
haraken
https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/MediaSource.cpp File Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/MediaSource.cpp#newcode269 Source/modules/mediasource/MediaSource.cpp:269: m_attachedElement->setCloseMediaSourceWhenFinalizing(); Don't you need to clear m_attachedElement here? https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/MediaSource.cpp#newcode269 ...
6 years, 5 months ago (2014-07-03 03:25:37 UTC) #2
sof
https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/MediaSource.cpp File Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/MediaSource.cpp#newcode269 Source/modules/mediasource/MediaSource.cpp:269: m_attachedElement->setCloseMediaSourceWhenFinalizing(); On 2014/07/03 03:25:37, haraken wrote: > > By ...
6 years, 5 months ago (2014-07-03 06:22:24 UTC) #3
haraken
https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/MediaSource.cpp File Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/MediaSource.cpp#newcode269 Source/modules/mediasource/MediaSource.cpp:269: m_attachedElement->setCloseMediaSourceWhenFinalizing(); On 2014/07/03 06:22:24, sof wrote: > On 2014/07/03 ...
6 years, 5 months ago (2014-07-03 06:40:31 UTC) #4
sof
https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/SourceBuffer.cpp File Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/SourceBuffer.cpp#newcode109 Source/modules/mediasource/SourceBuffer.cpp:109: removedFromMediaSource(); On 2014/07/03 06:40:31, haraken wrote: > On 2014/07/03 ...
6 years, 5 months ago (2014-07-03 06:52:25 UTC) #5
sof
On 2014/07/03 06:52:25, sof wrote: > https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/SourceBuffer.cpp > File Source/modules/mediasource/SourceBuffer.cpp (right): > > https://codereview.chromium.org/363953002/diff/20001/Source/modules/mediasource/SourceBuffer.cpp#newcode109 > ...
6 years, 5 months ago (2014-07-03 08:22:48 UTC) #6
haraken
+acolwell LGTM https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasource/MediaSource.cpp File Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasource/MediaSource.cpp#newcode267 Source/modules/mediasource/MediaSource.cpp:267: // notify its MediaSource during finalization by ...
6 years, 5 months ago (2014-07-03 09:46:05 UTC) #7
sof
https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasource/MediaSource.cpp File Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasource/MediaSource.cpp#newcode267 Source/modules/mediasource/MediaSource.cpp:267: // notify its MediaSource during finalization by calling close(). ...
6 years, 5 months ago (2014-07-03 09:49:00 UTC) #8
sof
On 2014/07/03 09:49:00, sof wrote: > https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasource/MediaSource.cpp > File Source/modules/mediasource/MediaSource.cpp (right): > > https://codereview.chromium.org/363953002/diff/40001/Source/modules/mediasource/MediaSource.cpp#newcode267 > ...
6 years, 5 months ago (2014-07-03 09:59:18 UTC) #9
sof
Sending this one along; will ofc followup should there be later arriving feedback.
6 years, 5 months ago (2014-07-04 09:37:59 UTC) #10
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 5 months ago (2014-07-04 09:38:02 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/363953002/60001
6 years, 5 months ago (2014-07-04 09:38:37 UTC) #12
commit-bot: I haz the power
6 years, 5 months ago (2014-07-04 10:23:45 UTC) #13
Message was sent while issue was closed.
Change committed as 177532

Powered by Google App Engine
This is Rietveld 408576698