Bug 155117 - Set AVURLAssetUsesNoPersistentCacheKey on AVAsset to match caching policy.
Summary: Set AVURLAssetUsesNoPersistentCacheKey on AVAsset to match caching policy.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-07 09:31 PST by Jeremy Jones
Modified: 2016-03-10 20:50 PST (History)
3 users (show)

See Also:


Attachments
Patch (12.66 KB, patch)
2016-03-07 09:48 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (14.71 KB, patch)
2016-03-07 13:32 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (14.86 KB, patch)
2016-03-10 14:08 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (14.95 KB, patch)
2016-03-10 17:40 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2016-03-07 09:31:11 PST
Set AVURLAssetUsesNoPersistentCacheKey on AVAsset to match caching policy.
Comment 1 Jeremy Jones 2016-03-07 09:32:16 PST
rdar://problem/6802240
Comment 2 Jeremy Jones 2016-03-07 09:48:31 PST
Created attachment 273189 [details]
Patch
Comment 3 Jon Lee 2016-03-07 10:13:56 PST
Comment on attachment 273189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273189&action=review

What about WK1?

> Source/WebCore/ChangeLog:8
> +        No new tests because no new funcitonality was added.

functionality*

> Source/WebCore/html/HTMLMediaElement.h:598
> +    bool mediaPlayerUsePersistentCache() override;

Uses* PersistentCache?

> Source/WebCore/page/ChromeClient.h:473
> +    virtual bool isDataStorePersistent() { return true; }

This will probably cause confusion because on its face it sounds like a generic data store, not one specific to media. Can we make it media-specific?

> Source/WebKit2/Shared/WebPageCreationParameters.cpp:58
> +    encoder << isDataStorePersistent;

ditto.

> Source/WebKit2/Shared/WebPageCreationParameters.cpp:138
> +    if (!decoder.decode(parameters.isDataStorePersistent))

ditto.

> Source/WebKit2/Shared/WebPageCreationParameters.h:92
> +    bool isDataStorePersistent;

ditto.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:5170
> +    parameters.isDataStorePersistent = m_websiteDataStore->isPersistent();

ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:1150
> +bool WebChromeClient::isDataStorePersistent()

ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:1152
> +    return m_page->isDataStorePersistent();

ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h:338
> +    bool isDataStorePersistent() override;

ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:368
> +    , m_isDataStorePersistent(parameters.isDataStorePersistent)

ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:932
> +    bool isDataStorePersistent() { return m_isDataStorePersistent; }

ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1447
> +    bool m_isDataStorePersistent;

ditto.
Comment 4 Jeremy Jones 2016-03-07 13:11:04 PST
(In reply to comment #3)
> Comment on attachment 273189 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273189&action=review
> 
> What about WK1?

For WebKit1, caches are disabled by setting [[NSURLCache sharedURLCache] setDiskCapacity:0];

I'll add a WK1 implementation that checks this value.

> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests because no new funcitonality was added.
> 
> functionality*

Done.

> 
> > Source/WebCore/html/HTMLMediaElement.h:598
> > +    bool mediaPlayerUsePersistentCache() override;
> 
> Uses* PersistentCache?

Sure.

> 
> > Source/WebCore/page/ChromeClient.h:473
> > +    virtual bool isDataStorePersistent() { return true; }
> 
> This will probably cause confusion because on its face it sounds like a
> generic data store, not one specific to media. Can we make it media-specific?

mediaUsesPersistentCache, match the MediaPlayer version.

> 
> > Source/WebKit2/Shared/WebPageCreationParameters.cpp:58
> > +    encoder << isDataStorePersistent;
> 
> ditto.
ditto....
Comment 5 Jeremy Jones 2016-03-07 13:32:05 PST
Created attachment 273206 [details]
Patch
Comment 6 Simon Fraser (smfr) 2016-03-07 14:36:47 PST
Comment on attachment 273206 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273206&action=review

> Source/WebCore/ChangeLog:10
> +        This will prevent persistent media caches when webkit is using in memory caching.

in-memory

> Source/WebCore/page/ChromeClient.h:473
> +    virtual bool mediaUsesPersistentCache() { return true; }

Can this be a const method?

> Source/WebCore/platform/graphics/MediaPlayer.h:238
> +    virtual bool mediaPlayerUsesPersistentCache() { return true; }

Can this be const?

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1060
> +bool WebChromeClient::mediaUsesPersistentCache()
> +{
> +    return [[NSURLCache sharedURLCache] diskCapacity] > 0;
> +}

Why make this about media at all? It seems like what you expose to WebCore is "disk cache enabled".

Also, we have our own disk cache in the network process, so I'm not sure the sharedURLCache capacity is at all relevant. Talk to Antti.
Comment 7 Jeremy Jones 2016-03-07 14:51:56 PST
Comment on attachment 273206 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273206&action=review

>> Source/WebCore/ChangeLog:10
>> +        This will prevent persistent media caches when webkit is using in memory caching.
> 
> in-memory

Done.

>> Source/WebCore/page/ChromeClient.h:473
>> +    virtual bool mediaUsesPersistentCache() { return true; }
> 
> Can this be a const method?

Probably. I'll change if possible.

>> Source/WebCore/platform/graphics/MediaPlayer.h:238
>> +    virtual bool mediaPlayerUsesPersistentCache() { return true; }
> 
> Can this be const?

Probably. I'll change if possible.

>> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1060
>> +}
> 
> Why make this about media at all? It seems like what you expose to WebCore is "disk cache enabled".
> 
> Also, we have our own disk cache in the network process, so I'm not sure the sharedURLCache capacity is at all relevant. Talk to Antti.

My previous patch was not media specific. I received feedback that I should make it media specific.

This is the WK1 implementation.
Comment 8 Jeremy Jones 2016-03-10 14:08:05 PST
Created attachment 273620 [details]
Patch
Comment 9 Jeremy Jones 2016-03-10 14:15:25 PST
(In reply to comment #6)
> Comment on attachment 273206 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273206&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        This will prevent persistent media caches when webkit is using in memory caching.
> 
> in-memory

Done.

> 
> > Source/WebCore/page/ChromeClient.h:473
> > +    virtual bool mediaUsesPersistentCache() { return true; }
> 
> Can this be a const method?

Yes. Done.

> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:238
> > +    virtual bool mediaPlayerUsesPersistentCache() { return true; }
> 
> Can this be const?

Yes. Done.

> 
> > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1060
> > +bool WebChromeClient::mediaUsesPersistentCache()
> > +{
> > +    return [[NSURLCache sharedURLCache] diskCapacity] > 0;
> > +}
> 
> Why make this about media at all? It seems like what you expose to WebCore
> is "disk cache enabled".

This is about configuring MediaPlayer i.e. AVFoundation to use a persistent cache or not. I don't want this to be used more generally for other caches. So, I've renamed these to "mediaShouldUsePersistentCache" to make it more explicit that you ask this to find out if media should use a cache. It is not telling you if media uses a cache.

WebKit 1 has no cache model for no disk cache. So, this implementation is not a requirement. It is nice to have, since there is a develop menu option to disable cache, which sets -diskCapacity to 0.

> 
> Also, we have our own disk cache in the network process, so I'm not sure the
> sharedURLCache capacity is at all relevant. Talk to Antti.

Not relevant. This implementation is for WK1. The WK2 implementation gets is values from the persistentDataStore.
Comment 10 Simon Fraser (smfr) 2016-03-10 17:34:48 PST
Comment on attachment 273620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273620&action=review

> Source/WebCore/ChangeLog:5
> +

Radar.
Comment 11 Jeremy Jones 2016-03-10 17:40:05 PST
Created attachment 273660 [details]
Patch
Comment 12 Jer Noble 2016-03-10 20:49:57 PST
Comment on attachment 273660 [details]
Patch

Clearing flags on attachment: 273660

Committed r197990: <http://trac.webkit.org/changeset/197990>
Comment 13 Jer Noble 2016-03-10 20:50:01 PST
All reviewed patches have been landed.  Closing bug.