Bug 73743

Skipped in r101927

It seems that these tests are no longer available. They were removed in r102300: <http://trac.webkit.org/changeset/102300> Can you recommend other ways to test the SubResourceLoader issue?

Well then let's wait for the original feature to land again from bug 72178 (Not sure why it was closed actually... Seems like Aaron wanted to rework the patch).

Adding Carlos, perhaps we can retitle this bug wrt the work you're doing on the http source element?

(In reply to comment #4) > Adding Carlos, perhaps we can retitle this bug wrt the work you're doing on the http source element? My plan is to use CachedLoader, that could also be used for loading other URL types that are not HTTP family like blob. I have a patch already but it depends on bug #115804, to make sure we can still use our GST buffer directly. CacheLoader uses Subresource loader so there's no need to rename the bug.

Created attachment 202810 [details] Patch This patch depends on bug #115804, otherwise we will assert in debug builds because when using soup the buffer won't be the one allocated by us.

Created attachment 205022 [details] Updated patch Rebased to apply on current git master and added error check in notifyFinished.

Comment on attachment 205022 [details] Updated patch Attachment 205022 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/850562

Created attachment 205025 [details] Try to fix efl build Also added the new file to the GTK+ cmake file too.

Comment on attachment 205025 [details] Try to fix efl build View in context: https://bugs.webkit.org/attachment.cgi?id=205025&action=review > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1027 > + if (resource->loadFailedOrCanceled()) { Ok, what about errorOccured()? Shouldn't it be checked too?

(In reply to comment #10) > (From update of attachment 205025 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205025&action=review > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1027 > > + if (resource->loadFailedOrCanceled()) { > > Ok, what about errorOccured()? Shouldn't it be checked too? This should be covered already, error handling of cached resources is a bit confusing, I think. errorOccurred() checks id status is LoadError or DecodeError. DecodeError is only used by Font and Image subresources, so we only need to care about load errors. When a subresource fails to load, setResourceError is called (so loadFailedOrCanceled() is true) and then LoadError is set (so errorOccured() is true). When a subresource is cancelled, willCancel calls setResourceError and didCancel calls didCancelLoad that sets the status to LoadError. What we could do is get the actual ResourceError to provide some more information like current code does. I'll update the patch.

Created attachment 205066 [details] Updated patch Improved error handling.

Created attachment 205876 [details] Rebased patch

Comment on attachment 205876 [details] Rebased patch LGTM, thanks!

Created attachment 206869 [details] Patch for landing I've moved the soup specific code from CachedRawResource.cpp to its own file in loader/soup

Comment on attachment 206869 [details] Patch for landing ok

Committed r152778: <http://trac.webkit.org/changeset/152778>

SubresourceLoader and CachedResource(*) are not platform-specific dumping grounds. ResourceHandle(Client) was, in fact, a valid place for this intensely GStreamer specific code. Please rework this quickly or roll it out until it can be revisited.

Note - I'm not saying you shouldn't be using SubresourceLoader, which is the entire premise of this bug. I'm saying you need to find a way to do it without leaking in platform specific details.

The platform specific code is indeed part of the ResourceHandle, the soup implementation allows the client to provide its own read buffer, which is used by the gstreamer backend to avoid buffer copies while loading media, but it's not a gstreamer feature. The same way the mac port has platform specific support for loading the data in a CFArrayRef. Except for the headers, all other platform specific code is in platform-specific files. I don't see how the gstreamer loader can provide the read buffer without using the CachedRawResource client. I agree that ideally, there shouldn't be platform ifdefs in those files. Another possibility is adding getOrCreateReadBuffer unconditionally, but unimplemented in other platforms, since the method itself doesn't have any platform specific types.

(In reply to comment #20) > The platform specific code is indeed part of the ResourceHandle, the soup implementation allows the client to provide its own read buffer, which is used by the gstreamer backend to avoid buffer copies while loading media, but it's not a gstreamer feature. But you just said that it's a gstreamer feature. Each media backend tends to do things radically differently from each other, but we've successfully kept the implementation details of ours out of the general purpose, cross-platform loader code. > The same way the mac port has platform specific support for loading the data in a CFArrayRef. Which is an unfortunate case and one that we hope to rectify going forward. But also, note that this callback is *only* in SubresourceLoader, and it was not necessary to pipe it through to the entire CachedResource/CachedRawResource/CachedRawResourceClient stack of classes. They are absolutely *not* a place for platform loading details. It's wrong wrong wrong. > Except for the headers, all other platform specific code is in platform-specific files. I don't see how the gstreamer loader can provide the read buffer without using the CachedRawResource client. I agree that ideally, there shouldn't be platform ifdefs in those files. Another possibility is adding getOrCreateReadBuffer unconditionally, but unimplemented in other platforms, since the method itself doesn't have any platform specific types. Let me ask this - What advantage is gained by this bug? The bug is titled "use SubresourceLoader", but why is that important? Didn't things work just fine when this code was in ResourceHandle? Why is this move important to even be contemplating?

(In reply to comment #21) > (In reply to comment #20) > > The platform specific code is indeed part of the ResourceHandle, the soup implementation allows the client to provide its own read buffer, which is used by the gstreamer backend to avoid buffer copies while loading media, but it's not a gstreamer feature. > > But you just said that it's a gstreamer feature. It's a ResourceHandleSoup feature used by the streamer media backend. > Each media backend tends to do things radically differently from each other, but we've successfully kept the implementation details of ours out of the general purpose, cross-platform loader code. > > > The same way the mac port has platform specific support for loading the data in a CFArrayRef. > > Which is an unfortunate case and one that we hope to rectify going forward. > > But also, note that this callback is *only* in SubresourceLoader, and it was not necessary to pipe it through to the entire CachedResource/CachedRawResource/CachedRawResourceClient stack of classes. > > They are absolutely *not* a place for platform loading details. It's wrong wrong wrong. That was just an example, there's also a mac platform ifdef in CachedResource, tryReplaceEncodedData, that is only used by the network process. But anyway, as I said, I agree with you that ideally there shouldn't be platform ifdefs there. And that's why I also proposed to simply remove the #ifdefs, and leave it unimplemented in platforms that don't need it, or even add a feature flag. I think letting the client provide the read buffer is useful feature in any case. > > Except for the headers, all other platform specific code is in platform-specific files. I don't see how the gstreamer loader can provide the read buffer without using the CachedRawResource client. I agree that ideally, there shouldn't be platform ifdefs in those files. Another possibility is adding getOrCreateReadBuffer unconditionally, but unimplemented in other platforms, since the method itself doesn't have any platform specific types. > > Let me ask this - What advantage is gained by this bug? The bug is titled "use SubresourceLoader", but why is that important? Didn't things work just fine when this code was in ResourceHandle? Why is this move important to even be contemplating? Phil can probably answer it better than me. I started to work on this to reduce the amount of layering violations in gstreamer media player. Using CachedResourceLoader is still a layering violation, but also used by mac port, so it would be a common point to look for a solution valid for both ports. It gives us more control over mixed content and security related stuff, so It should allow us to unskip some layout tests (which I forgot to do it). It also allows to track the progress of the media loaded in the inspector like any other resource. And also allows us to easily support other non HTTP sources like data or blob urls, see bug #102586.

(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #20) > > > The platform specific code is indeed part of the ResourceHandle, the soup implementation allows the client to provide its own read buffer, which is used by the gstreamer backend to avoid buffer copies while loading media, but it's not a gstreamer feature. > > > > But you just said that it's a gstreamer feature. > > It's a ResourceHandleSoup feature used by the streamer media backend. Right. Again, not something that adds to the argument that this belongs at a higher level than that platform specific networking layer. > > > Each media backend tends to do things radically differently from each other, but we've successfully kept the implementation details of ours out of the general purpose, cross-platform loader code. > > > > > The same way the mac port has platform specific support for loading the data in a CFArrayRef. > > > > Which is an unfortunate case and one that we hope to rectify going forward. > > > > But also, note that this callback is *only* in SubresourceLoader, and it was not necessary to pipe it through to the entire CachedResource/CachedRawResource/CachedRawResourceClient stack of classes. > > > > They are absolutely *not* a place for platform loading details. It's wrong wrong wrong. > > That was just an example, there's also a mac platform ifdef in CachedResource, tryReplaceEncodedData, that is only used by the network process. Actually, there is literally nothing platform dependent about that, and we have it working in generic WebKit1 implementations experimentally. That will be upstreamed sometime soon. The only reason it was #if PLATFORM(MAC)'ed was to keep from having to give empty implementations to other platforms. This new thing that was added throughout the entire SubresourceLoader and memory cache machinery is not something that is generically usable, nor does it belong in CachedResource* code at all. > But anyway, as I said, I agree with you that ideally there shouldn't be platform ifdefs there. And that's why I also proposed to simply remove the #ifdefs, and leave it unimplemented in platforms that don't need it, or even add a feature flag. I think letting the client provide the read buffer is useful feature in any case. Perhaps implementing a feature where WebCore provides a buffer to the networking layer is useful. This is the wrong form to do it in. We would not implement it using raw pointers and it would not be implemented in CachedResource* code. > > > Except for the headers, all other platform specific code is in platform-specific files. I don't see how the gstreamer loader can provide the read buffer without using the CachedRawResource client. I agree that ideally, there shouldn't be platform ifdefs in those files. Another possibility is adding getOrCreateReadBuffer unconditionally, but unimplemented in other platforms, since the method itself doesn't have any platform specific types. > > > > Let me ask this - What advantage is gained by this bug? The bug is titled "use SubresourceLoader", but why is that important? Didn't things work just fine when this code was in ResourceHandle? Why is this move important to even be contemplating? > > Phil can probably answer it better than me. I started to work on this to reduce the amount of layering violations in gstreamer media player. Using CachedResourceLoader is still a layering violation, but also used by mac port, so it would be a common point to look for a solution valid for both ports. Can you tell me how using CachedResourceLoader for the gstreamer media backend is a layering violation for the Mac port? (I know that's not what you meant, but I actually have no idea what you mean here, so I'd like clarification) > It gives us more control over mixed content and security related stuff, so It should allow us to unskip some layout tests (which I forgot to do it). > It also allows to track the progress of the media loaded in the inspector like any other resource. > And also allows us to easily support other non HTTP sources like data or blob urls, see bug #102586. Thanks. Those sound like good reasons to do this. I wouldn't have had to ask if they were spelled out in the bug. But good reasons to do this are still not a reason for doing it incorrectly.

(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > (In reply to comment #20) > > > > The platform specific code is indeed part of the ResourceHandle, the soup implementation allows the client to provide its own read buffer, which is used by the gstreamer backend to avoid buffer copies while loading media, but it's not a gstreamer feature. > > > > > > But you just said that it's a gstreamer feature. > > > > It's a ResourceHandleSoup feature used by the streamer media backend. > > Right. Again, not something that adds to the argument that this belongs at a higher level than that platform specific networking layer. > > > > > Each media backend tends to do things radically differently from each other, but we've successfully kept the implementation details of ours out of the general purpose, cross-platform loader code. > > > > > > > The same way the mac port has platform specific support for loading the data in a CFArrayRef. > > > > > > Which is an unfortunate case and one that we hope to rectify going forward. > > > > > > But also, note that this callback is *only* in SubresourceLoader, and it was not necessary to pipe it through to the entire CachedResource/CachedRawResource/CachedRawResourceClient stack of classes. > > > > > > They are absolutely *not* a place for platform loading details. It's wrong wrong wrong. > > > > That was just an example, there's also a mac platform ifdef in CachedResource, tryReplaceEncodedData, that is only used by the network process. > > Actually, there is literally nothing platform dependent about that, and we have it working in generic WebKit1 implementations experimentally. That will be upstreamed sometime soon. The only reason it was #if PLATFORM(MAC)'ed was to keep from having to give empty implementations to other platforms. Ok, so it's the same example as getOrCreateReadBuffer, there's nothing specific to soup, I simply added the ifdefs to avoid empty implementations in other ports. > This new thing that was added throughout the entire SubresourceLoader and memory cache machinery is not something that is generically usable, nor does it belong in CachedResource* code at all. Why can't it be used by other ports? Because the network platform handles the read buffer and doesn't allow API users to provide their own? > > But anyway, as I said, I agree with you that ideally there shouldn't be platform ifdefs there. And that's why I also proposed to simply remove the #ifdefs, and leave it unimplemented in platforms that don't need it, or even add a feature flag. I think letting the client provide the read buffer is useful feature in any case. > > Perhaps implementing a feature where WebCore provides a buffer to the networking layer is useful. This is the wrong form to do it in. We would not implement it using raw pointers and it would not be implemented in CachedResource* code. Fair enough. > > > > Except for the headers, all other platform specific code is in platform-specific files. I don't see how the gstreamer loader can provide the read buffer without using the CachedRawResource client. I agree that ideally, there shouldn't be platform ifdefs in those files. Another possibility is adding getOrCreateReadBuffer unconditionally, but unimplemented in other platforms, since the method itself doesn't have any platform specific types. > > > > > > Let me ask this - What advantage is gained by this bug? The bug is titled "use SubresourceLoader", but why is that important? Didn't things work just fine when this code was in ResourceHandle? Why is this move important to even be contemplating? > > > > Phil can probably answer it better than me. I started to work on this to reduce the amount of layering violations in gstreamer media player. Using CachedResourceLoader is still a layering violation, but also used by mac port, so it would be a common point to look for a solution valid for both ports. > > Can you tell me how using CachedResourceLoader for the gstreamer media backend is a layering violation for the Mac port? (I know that's not what you meant, but I actually have no idea what you mean here, so I'd like clarification) Because the media player is in platform and loader code is not. See bugs #21354 and #21562. > > It gives us more control over mixed content and security related stuff, so It should allow us to unskip some layout tests (which I forgot to do it). > > It also allows to track the progress of the media loaded in the inspector like any other resource. > > And also allows us to easily support other non HTTP sources like data or blob urls, see bug #102586. > > Thanks. Those sound like good reasons to do this. I wouldn't have had to ask if they were spelled out in the bug. See bug description and comment #5. > But good reasons to do this are still not a reason for doing it incorrectly. I agree. I'll think of better alternatives, one that come to my mind in this moment would be to add a special loader for media, similar to the plugins stream loader used by PluginStream. I'm adding Eric to the CC since maybe he had other ideas.

(In reply to comment #24) > > Can you tell me how using CachedResourceLoader for the gstreamer media backend is a layering violation for the Mac port? (I know that's not what you meant, but I actually have no idea what you mean here, so I'd like clarification) > > Because the media player is in platform and loader code is not. See bugs #21354 and #21562. And used by mac port as well in WebCoreAVFResourceLoader, sorry that I misread the question.

(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > (In reply to comment #21) > > > That was just an example, there's also a mac platform ifdef in CachedResource, tryReplaceEncodedData, that is only used by the network process. > > > > Actually, there is literally nothing platform dependent about that, and we have it working in generic WebKit1 implementations experimentally. That will be upstreamed sometime soon. The only reason it was #if PLATFORM(MAC)'ed was to keep from having to give empty implementations to other platforms. > > Ok, so it's the same example as getOrCreateReadBuffer, there's nothing specific to soup, I simply added the ifdefs to avoid empty implementations in other ports. No. It's actually a generic feature of any CachedResource. What this patch crammed into multiple CachedResource* classes was a something that doesn't belong in the memory cache at all. And - if it did - it wouldn't be implemented with raw pointers like this. > > > This new thing that was added throughout the entire SubresourceLoader and memory cache machinery is not something that is generically usable, nor does it belong in CachedResource* code at all. > > Why can't it be used by other ports? Because the network platform handles the read buffer and doesn't allow API users to provide their own? Because other ports want the right design: - Loader code (the interface between networking and the notion of per-page subresources) does not belong in the memory cache. - We try to avoid raw pointers for buffers whenever humanly possible. That this method is a raw pointer + buffer seems indicative of it's SOUP-specificness. > > Can you tell me how using CachedResourceLoader for the gstreamer media backend is a layering violation for the Mac port? (I know that's not what you meant, but I actually have no idea what you mean here, so I'd like clarification) > > Because the media player is in platform and loader code is not. See bugs #21354 and #21562. And we're not going to untangle that mess by moving platform specific bits out of the platform directory and in to loader code > > > It gives us more control over mixed content and security related stuff, so It should allow us to unskip some layout tests (which I forgot to do it). > > > It also allows to track the progress of the media loaded in the inspector like any other resource. > > > And also allows us to easily support other non HTTP sources like data or blob urls, see bug #102586. > > > > Thanks. Those sound like good reasons to do this. I wouldn't have had to ask if they were spelled out in the bug. > > See bug description and comment #5. Some of what you mention is there. Sorry for missing that I think a lot of people tend to look at bug titles before anything else. Maybe the title should be less cryptic and more descriptive. > > But good reasons to do this are still not a reason for doing it incorrectly. > > I agree. I'll think of better alternatives, one that come to my mind in this moment would be to add a special loader for media, similar to the plugins stream loader used by PluginStream. I'm adding Eric to the CC since maybe he had other ideas. Let's add Jer also.

He was already CC'ed Great!

It seems the patch here also solved the bug in 109350 probably because it got rid of the RefPtr<Frame> that WebKitWebSource held, which caused a circular reference.

I am working on an updated patch for bug #115352 and stumbled upon an issue related to this bug. The issue is that the webkit source element can be created by any code using gst_element_make_from_uri with the uris supported by the webkit source element (currently http/https uris). This means that the source element may exist without a player involved (e.g. dash gstreamer plugin loading fragments in different threads), in which case we cannot get the CachedResourceLoader from priv->player which would not be set. I will try to create a CachedResourceLoader/CachedResource manually (when priv->player is not set) and see how it goes, otherwise I believe we will have to have 2 code branches (one using CachedResourceLoader as is now and another one using ResourceHandle as it was before). Please let me know what you think.

Ok, I tried creating a CachedResourceLoader manually (inside the webkit source element when there is no player involved) with CachedResourceLoader::create(0) but when calling requestRawResource() it always returns null. I nailed it down to ResourceLoader::init() failing when called from SubresourceLoader::init() called from CachedResourceLoader::requestResource(). Is there a way to create a CachedResourceLoader without a Document/frame involved? If not we should either have 2 code branches in the webkit source as I stated on comment #29 or revert this patch and always use ResourceHandle as it was before. Please let me know what you think.

I think the WebKit source element is to load URIs inside WebKit, to load HTTP/HTTPS URIs from any other application not using WebKit I think there's another implementation based only on libsoup.

Yes there is a souphttpsrc element in gst-plugins-good but the issue is that in WebKit we kinda neutralize it by configuring our http element to a higher rank. So code in gst plugins calling gst_element_make_from_uri("http") will indeed get a webkitwebsrc element :) We intend to fix that in bug 108088 . Andre, perhaps you can try the patch from that bug? It's not ready to land yet but it should help you.

(In reply to comment #32) > Yes there is a souphttpsrc element in gst-plugins-good but the issue is that in WebKit we kinda neutralize it by configuring our http element to a higher rank. > > So code in gst plugins calling gst_element_make_from_uri("http") will indeed get a webkitwebsrc element :) We intend to fix that in bug 108088 . Andre, perhaps you can try the patch from that bug? It's not ready to land yet but it should help you. As discussed on IRC, the proposed solution for bug 108088 would break the use case for DASH/MSS and others where a gst plugin creates new source elements internally to download data. I pushed an updated patch for bug 115352 that attempts to solve the issues I raised on comment #29 by using 2 code branches, one using a CachedResourceLoader when the player is available, and another one using a ResourceHandle when there is no player involved.

Anything left on this bug? I guess all issues are now handled now that the patch on bug 115352 also landed.

This can be closed I think.