Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
302093011:
Oilpan: move the MediaPlayer and MediaPlayerClient objects to the heap. (Closed)
|
Created:
6 years, 6 months ago by sof Modified:
6 years, 6 months ago CC:
blink-reviews, jamesr, blink-reviews-html_chromium.org, Rik, pdr., philipj_slow, gasubic, fs, eric.carlson_apple.com, jbroman, danakj, feature-media-reviews_chromium.org, dglazkov+blink, nessy, krit, Stephen Chennney, vcarbune.chromium, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionOilpan: move the MediaPlayer and MediaPlayerClient objects to the heap.
Move the implementation of the MediaPlayer objects to the heap, allowing
it to be kept as a Member of its client object, the media element.
The media element now first calls detach() on the media player when
clearing out its reference. This is so that the media player object can
explicitly and synchronously notify its dependent objects of the player
object going away. In the case of Oilpan it will be swept out and
destructed at the next GC.
R=
BUG=378229
Patch Set 1 #Patch Set 2 : Move DummyBase from WebCore to WTF #
Total comments: 19
Patch Set 3 : Reset WebLayer if media element and player object die simult #Patch Set 4 : Explicitly instantiate and export DummyBase<void> #
Total comments: 17
Patch Set 5 : Have MediaController weakly track its media elements #
Total comments: 7
Messages
Total messages: 22 (0 generated)
Please take a look. Media player destruction is a bit ornery in places, but this is now ready & in a shape that I'm content with. (Still trying to get a greener Oilpan tree to run trybots against; please ignore their redness for now.) https://codereview.chromium.org/302093011/diff/20001/Source/platform/graphics... File Source/platform/graphics/media/MediaPlayer.h (right): https://codereview.chromium.org/302093011/diff/20001/Source/platform/graphics... Source/platform/graphics/media/MediaPlayer.h:59: class MediaPlayerClient : public WillBeGarbageCollectedMixin { The use of DummyBase<void> breaks the component build in the non-Oilpan case, see http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu... I _thought_ this was due to DummyBase being provided via WebCore & moved it to WTF instead, but no. I've not yet #if-ed my way around the issue..is there a better way to handle it?
oilpan lgtm. https://codereview.chromium.org/302093011/diff/20001/Source/platform/heap/Han... File Source/platform/heap/Handle.h (right): https://codereview.chromium.org/302093011/diff/20001/Source/platform/heap/Han... Source/platform/heap/Handle.h:45: class DummyBase { I think this needs a PLATFORM_EXPORT to satisfy the component build. https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:91: { The two issues below are not nice, but I think your proposals for follow-up fixes seem reasonable. https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:193: return; Yeah, this is is really not nice, but fixing in a follow-up is ok with me.
https://codereview.chromium.org/302093011/diff/20001/Source/platform/heap/Han... File Source/platform/heap/Handle.h (right): https://codereview.chromium.org/302093011/diff/20001/Source/platform/heap/Han... Source/platform/heap/Handle.h:45: class DummyBase { On 2014/06/02 12:59:07, zerny-chromium wrote: > I think this needs a PLATFORM_EXPORT to satisfy the component build. That might work, but this is a template, so unclear at what instances to export it at.
https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:76: audioSourceProvider()->setClient(0); Instead of adding these lines, if possible, it would be better to move AudioSourceProvider to the heap at the same time. https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:79: m_client.clear(); hmm, I don't still understand why you need this clear. Here the WebMediaPlayerClientImpl is about to die, so we no longer need to take care of Members, don't we? https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:82: m_webMediaPlayer.clear(); You don't need this since the OwnPtr will be automatically destroyed when WebMediaPlayerClientImpl is destructed. https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:108: // avoid that callback. Understood. A follow-up is fine :) https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:502: m_webAudioSourceProvider = 0; This looks weird. Moving AudioSourceProvider to the heap will allow us to remove these lines, but would it be hard? Nit: You need #if ENABLE(WEB_AUDIO).
https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:79: m_client.clear(); On 2014/06/02 15:24:48, haraken wrote: > > hmm, I don't still understand why you need this clear. Here the > WebMediaPlayerClientImpl is about to die, so we no longer need to take care of > Members, don't we? It is needed for the exact same reason as it is needed in detach(). If the client object dies without calling detach() (a possibility with Oilpan), then we have to prevent the WebMediaPlayer from attempting to clear the WebLayer by calling back into the client when destructed.
https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:79: m_client.clear(); On 2014/06/02 15:46:55, sof wrote: > On 2014/06/02 15:24:48, haraken wrote: > > > > hmm, I don't still understand why you need this clear. Here the > > WebMediaPlayerClientImpl is about to die, so we no longer need to take care of > > Members, don't we? > > It is needed for the exact same reason as it is needed in detach(). If the > client object dies without calling detach() (a possibility with Oilpan), then we > have to prevent the WebMediaPlayer from attempting to clear the WebLayer by > calling back into the client when destructed. Makes sense. Then who calls mediaPlayerSetWebLayer(0) when the WebMediaPlayerClient and the HTMLMediaElement die at the same time? It seems we need to call mediaPlayerSetWebLayer(0) even when they die togther, in order to call GraphicsLayer::unregisterContentsLayer(m_webLayer).
https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:79: m_client.clear(); On 2014/06/02 16:01:42, haraken wrote: > On 2014/06/02 15:46:55, sof wrote: > > On 2014/06/02 15:24:48, haraken wrote: > > > > > > hmm, I don't still understand why you need this clear. Here the > > > WebMediaPlayerClientImpl is about to die, so we no longer need to take care > of > > > Members, don't we? > > > > It is needed for the exact same reason as it is needed in detach(). If the > > client object dies without calling detach() (a possibility with Oilpan), then > we > > have to prevent the WebMediaPlayer from attempting to clear the WebLayer by > > calling back into the client when destructed. > > Makes sense. > > Then who calls mediaPlayerSetWebLayer(0) when the WebMediaPlayerClient and the > HTMLMediaElement die at the same time? It seems we need to call > mediaPlayerSetWebLayer(0) even when they die togther, in order to call > GraphicsLayer::unregisterContentsLayer(m_webLayer). When doing the obvious finalization of that pre-existing layer, this lead to stability problems in a number of cases. I will have to re-investigate. https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:82: m_webMediaPlayer.clear(); On 2014/06/02 15:24:48, haraken wrote: > > You don't need this since the OwnPtr will be automatically destroyed when > WebMediaPlayerClientImpl is destructed. I'm just keeping what was here previously. https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:502: m_webAudioSourceProvider = 0; On 2014/06/02 15:24:48, haraken wrote: > > This looks weird. Moving AudioSourceProvider to the heap will allow us to remove > these lines, but would it be hard? > Hmm, I think this CL is complex enough as it is. > Nit: You need #if ENABLE(WEB_AUDIO). Already within such a context.
https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:502: m_webAudioSourceProvider = 0; On 2014/06/02 16:25:08, sof wrote: > On 2014/06/02 15:24:48, haraken wrote: > > > > This looks weird. Moving AudioSourceProvider to the heap will allow us to > remove > > these lines, but would it be hard? > > > > Hmm, I think this CL is complex enough as it is. > > > Nit: You need #if ENABLE(WEB_AUDIO). > > Already within such a context. I'm fine with addressing the issue in a follow-up. A FIXME is fine in this CL.
https://codereview.chromium.org/302093011/diff/20001/Source/platform/heap/Han... File Source/platform/heap/Handle.h (right): https://codereview.chromium.org/302093011/diff/20001/Source/platform/heap/Han... Source/platform/heap/Handle.h:45: class DummyBase { On 2014/06/02 14:25:03, sof wrote: > On 2014/06/02 12:59:07, zerny-chromium wrote: > > I think this needs a PLATFORM_EXPORT to satisfy the component build. > > That might work, but this is a template, so unclear at what instances to export > it at. I've addressed the smaller problem of supporting DummyBase<void> (i.e., WillBeGarbageCollectedMixin) from code residing in other components. https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:79: m_client.clear(); On 2014/06/02 16:25:08, sof wrote: > On 2014/06/02 16:01:42, haraken wrote: > > On 2014/06/02 15:46:55, sof wrote: > > > On 2014/06/02 15:24:48, haraken wrote: > > > > > > > > hmm, I don't still understand why you need this clear. Here the > > > > WebMediaPlayerClientImpl is about to die, so we no longer need to take > care > > of > > > > Members, don't we? > > > > > > It is needed for the exact same reason as it is needed in detach(). If the > > > client object dies without calling detach() (a possibility with Oilpan), > then > > we > > > have to prevent the WebMediaPlayer from attempting to clear the WebLayer by > > > calling back into the client when destructed. > > > > Makes sense. > > > > Then who calls mediaPlayerSetWebLayer(0) when the WebMediaPlayerClient and the > > HTMLMediaElement die at the same time? It seems we need to call > > mediaPlayerSetWebLayer(0) even when they die togther, in order to call > > GraphicsLayer::unregisterContentsLayer(m_webLayer). > > When doing the obvious finalization of that pre-existing layer, this lead to > stability problems in a number of cases. I will have to re-investigate. I cannot reproduce that instability now (but am running it past all bots atm to verify) -- it could be that this case of WebLayer resetting got tangled up with clearing the client reference correctly. Not sure. Anyway, I've re-instated that call to mediaPlayerSetWebLayer(0) in ~HTMLMediaElement in the Oilpan case. Only called if the player hasn't been detached, i.e., the media element and player die at the same time. https://codereview.chromium.org/302093011/diff/20001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:502: m_webAudioSourceProvider = 0; On 2014/06/02 16:32:40, haraken wrote: > On 2014/06/02 16:25:08, sof wrote: > > On 2014/06/02 15:24:48, haraken wrote: > > > > > > This looks weird. Moving AudioSourceProvider to the heap will allow us to > > remove > > > these lines, but would it be hard? > > > > > > > Hmm, I think this CL is complex enough as it is. > > > > > Nit: You need #if ENABLE(WEB_AUDIO). > > > > Already within such a context. > > I'm fine with addressing the issue in a follow-up. A FIXME is fine in this CL. Added a FIXME next to the AudoSourceProviderImpl declaration.
https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:332: if (m_mediaController) { We should convert MediaController into keeping a set of weak references to its media elements. Will do that via this CL. https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:337: closeMediaSource(); This is unsafe as it touches another heap object (implementing the abstract HTMLMediaSource interface.)
Mostly looks good. Let me ask a couple of clarifying questions. https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:347: m_completelyLoaded = true; Just help me understand: I agree that we can remove this code in oilpan builds, but how is the above issue handled in oilpan (i.e., how is it guaranteed that ~MediaPlayer does not trigger userCancelledLoad() when the MediaPlayer and the HTMLMediaElement die together)? https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:370: mediaPlayerSetWebLayer(0); This looks OK in this CL, but I'm curious what happens if you move WebMediaPlayer to the heap in a follow-up. https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3442: clearMediaPlayerAndAudioSourceProviderClient(); I wonder why you had to add this to oilpan builds. https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:76: audioSourceProvider()->setClient(0); I'm a bit confused: Why do we need to explicitly clear the client in oilpan builds, whereas we are not doing the clear in non-oilpan builds? https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:502: m_webAudioSourceProvider = 0; The same question here. I'm curious why we need to introduce this clear in oilpan builds. https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.h:133: OwnPtr<WebMediaPlayer> m_webMediaPlayer; Nit: I think you'll need to move WebMediaPlayer to the heap in a follow-up and I'm curious how things are reorganized then :)
https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.h:133: OwnPtr<WebMediaPlayer> m_webMediaPlayer; On 2014/06/03 05:31:08, haraken wrote: > > Nit: I think you'll need to move WebMediaPlayer to the heap in a follow-up and > I'm curious how things are reorganized then :) I don't understand what that means. We have moved what can be moved on the renderer side by now. blink::WebMediaPlayer is something implemented by the browser process, no?
https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:370: mediaPlayerSetWebLayer(0); On 2014/06/03 05:31:08, haraken wrote: > > This looks OK in this CL, but I'm curious what happens if you move > WebMediaPlayer to the heap in a follow-up. So the above comment doesn't make sense. Revised comment: how about moving the mediaPlayerSetWebLayer(0) call from ~HTMLMediaElement to ~WebMediaPlayerClientImpl, because ~WebMediaPlayerClientImpl owns the m_webMediaPlayer ? https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.h (right): https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.h:133: OwnPtr<WebMediaPlayer> m_webMediaPlayer; On 2014/06/03 05:45:44, sof wrote: > On 2014/06/03 05:31:08, haraken wrote: > > > > Nit: I think you'll need to move WebMediaPlayer to the heap in a follow-up and > > I'm curious how things are reorganized then :) > > I don't understand what that means. We have moved what can be moved on the > renderer side by now. blink::WebMediaPlayer is something implemented by the > browser process, no? oh, you're right; we cannot move blink::WebMediaPlayer to the heap.
https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:370: mediaPlayerSetWebLayer(0); On 2014/06/03 06:00:14, haraken wrote: > On 2014/06/03 05:31:08, haraken wrote: > > > > This looks OK in this CL, but I'm curious what happens if you move > > WebMediaPlayer to the heap in a follow-up. > > So the above comment doesn't make sense. > > Revised comment: how about moving the mediaPlayerSetWebLayer(0) call from > ~HTMLMediaElement to ~WebMediaPlayerClientImpl, because > ~WebMediaPlayerClientImpl owns the m_webMediaPlayer ? The m_webMediaPlayer and WebMediaPlayerClientImpl's client (an HTMLMediaElement) keeps track of the current WebLayer, not WebMediaPlayerClientImpl, so it wouldn't know what object to unregister with GraphicsLayer in ~WebMediaPlayerClientImpl (or detach(), I suppose.) It could intercept the setting of WebLayers via setWebLayer() and keep a separate copy, clearly. The question then becomes: is that worth doing?
On 2014/06/03 06:11:43, sof wrote: > https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... > Source/core/html/HTMLMediaElement.cpp:370: mediaPlayerSetWebLayer(0); > On 2014/06/03 06:00:14, haraken wrote: > > On 2014/06/03 05:31:08, haraken wrote: > > > > > > This looks OK in this CL, but I'm curious what happens if you move > > > WebMediaPlayer to the heap in a follow-up. > > > > So the above comment doesn't make sense. > > > > Revised comment: how about moving the mediaPlayerSetWebLayer(0) call from > > ~HTMLMediaElement to ~WebMediaPlayerClientImpl, because > > ~WebMediaPlayerClientImpl owns the m_webMediaPlayer ? > > The m_webMediaPlayer and WebMediaPlayerClientImpl's client (an HTMLMediaElement) > keeps track of the current WebLayer, not WebMediaPlayerClientImpl, so it > wouldn't know what object to unregister with GraphicsLayer in > ~WebMediaPlayerClientImpl (or detach(), I suppose.) > > It could intercept the setting of WebLayers via setWebLayer() and keep a > separate copy, clearly. The question then becomes: is that worth doing? Thanks, understood; your current approach sounds better.
https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:337: closeMediaSource(); On 2014/06/03 05:16:23, sof wrote: > This is unsafe as it touches another heap object (implementing the abstract > HTMLMediaSource interface.) As the media element currently keeps a RefPtr<> to the media source object, this isn't actually unsafe. We actually have to signal the close operation here, as the MediaSource supports an observable 'sourceclose' event. Hence, this call will be kept and MediaSource will not be altered. For now.
https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3442: clearMediaPlayerAndAudioSourceProviderClient(); On 2014/06/03 05:31:08, haraken wrote: > > I wonder why you had to add this to oilpan builds. We are possibly severing the connection to a pre-existing media player, so it will have to be detached. https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:76: audioSourceProvider()->setClient(0); On 2014/06/03 05:31:08, haraken wrote: > > I'm a bit confused: Why do we need to explicitly clear the client in oilpan > builds, whereas we are not doing the clear in non-oilpan builds? Because we will call clearMediaPlayerAndAudioSourceProviderClient() in the non-Oilpan case, which calls setClient(). In the Oilpan case, should this object die at the same time as the media element, we will have to arrange for setClient(0) to be called on the AudioSourceProvider also. That is the purpose of this call. To avoid performing a repeated setClient(0) call on said provider, we clear out the m_webAudioSourceProvider when the client is cleared. That is the purpose of the added code in setClient(). https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:502: m_webAudioSourceProvider = 0; On 2014/06/03 05:31:08, haraken wrote: > > The same question here. I'm curious why we need to introduce this clear in > oilpan builds. See answer to https://codereview.chromium.org/302093011/diff/60001/Source/web/WebMediaPlaye...
https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/302093011/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:332: if (m_mediaController) { On 2014/06/03 05:16:23, sof wrote: > We should convert MediaController into keeping a set of weak references to its > media elements. Will do that via this CL. Done. https://codereview.chromium.org/302093011/diff/80001/Source/core/html/MediaCo... File Source/core/html/MediaController.h (right): https://codereview.chromium.org/302093011/diff/80001/Source/core/html/MediaCo... Source/core/html/MediaController.h:111: typedef WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > MediaElementSequence; To be able to weakly refer to a registered sequence of media elements, I had to change this to a HeapLinkedHashSet. To keep things uniform, I switched to a LinkedHashSet in the non-Oilpan case (from a Vector.) Acceptable?
LGTM. Let's wait for an approval of acolwell. https://codereview.chromium.org/302093011/diff/80001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/80001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:75: if (audioSourceProvider()) I think this cannot be 0. https://codereview.chromium.org/302093011/diff/80001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:502: m_webAudioSourceProvider = 0; Having this code looks harmless, but I still don't understand why you need this. In non-oilpan builds, m_webAudioSourceProvider is not cleared when a client is unregistered from the AudioSourceProviderImpl, is it?
Sorry, forgot to answer the question earlier. https://codereview.chromium.org/302093011/diff/80001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/302093011/diff/80001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:502: m_webAudioSourceProvider = 0; On 2014/06/03 09:19:47, haraken wrote: > > Having this code looks harmless, but I still don't understand why you need this. > > In non-oilpan builds, m_webAudioSourceProvider is not cleared when a client is > unregistered from the AudioSourceProviderImpl, is it? m_webAudioSourceProvider is something m_webMediaPlayer owns. We cannot keep that reference once we delete m_webMediaPlayer, it's dead. Hence clearing it for both Oilpan and not. It is cleared in non-Oilpan builds also; see what happens when the media element detaches the player.
MediaPlayer and MediaPlayerClient are going away. There are several CLs in flight to remove pieces of it. I don't think we should invest in moving them to the heap. In the not too distant future HTMLMediaElement will be solely using WebMediaPlayer instead. If we need to move some of the code out of the destructor, that is fine and I think this CL should focus on that. I'm fine with the MediaController changes, but I think they should be moved to a different CL since those are a different set of changes. https://codereview.chromium.org/302093011/diff/80001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/302093011/diff/80001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3442: clearMediaPlayerAndAudioSourceProviderClient(); Why is this done here? I believe this needs to occur after the m_audioSourceNode->lock() to avoid the audio thread from accessing the audio source provider during destruction. https://codereview.chromium.org/302093011/diff/80001/Source/platform/graphics... File Source/platform/graphics/media/MediaPlayer.h (right): https://codereview.chromium.org/302093011/diff/80001/Source/platform/graphics... Source/platform/graphics/media/MediaPlayer.h:63: virtual void trace(Visitor*) { } This class is going away. We should not be adding code to it.
Thanks for filling me in on limited future of these objects. If MediaPlayer+MediaPlayerClient is going away and you don't consider it worth moving them to the heap, then I'm not going to invest any more time on this CL. https://codereview.chromium.org/302093011/diff/80001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/302093011/diff/80001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3442: clearMediaPlayerAndAudioSourceProviderClient(); On 2014/06/03 19:23:08, acolwell wrote: > Why is this done here? I believe this needs to occur after the > m_audioSourceNode->lock() to avoid the audio thread from accessing the audio > source provider during destruction. "this" == closeMediaSource() ? clearMediaPlayerAndAudioSourceProviderClient() has its own lock-unlock pairing. It mirrors the ordering for clearMediaPlayer(), where closeMediaSource() is called prior to clear...Client(); and there's no explicit lock there first. IOW, I thought the lock call here was to control/protect the MediaPlayer::create(this) call. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
