Bug 181623 - AVSampleBufferDisplayLayer should be flushed when application activates
Summary: AVSampleBufferDisplayLayer should be flushed when application activates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on: 186850
Blocks:
  Show dependency treegraph
 
Reported: 2018-01-12 17:06 PST by Eric Carlson
Modified: 2018-06-20 08:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (39.08 KB, patch)
2018-01-12 17:26 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (37.37 KB, patch)
2018-01-16 09:08 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2018-01-12 17:06:50 PST
AVSampleBufferDisplayLayer sometimes gets into an error state when the web process deactivates, and it must be flushed when application activates again.
Comment 1 Radar WebKit Bug Importer 2018-01-12 17:07:43 PST
<rdar://problem/36487738>
Comment 2 Eric Carlson 2018-01-12 17:26:37 PST
Created attachment 331261 [details]
Patch
Comment 3 Darin Adler 2018-01-13 13:47:32 PST
Comment on attachment 331261 [details]
Patch

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

Can we find a way to test this?

> Source/WebCore/dom/Document.h:1410
> +    void applicationWillResignActive();
> +    void applicationDidEnterBackground(bool);
> +    void applicationWillEnterForeground(bool);
> +    void applicationDidBecomeActive();

Perhaps instead of these we can just expose a single function:

    forEachApplicationStateChangeListener(const Function<void(ApplicationStateChangeListener&)>&);

I think this would cut down the total amount of code. The functions in Page can do more of the work.

> Source/WebCore/dom/Document.h:1895
> +    HashSet<ApplicationStateChangeListener*> m_applicationStateChangeListener;

Should be m_applicationStateChangeListeners, plural.

> Source/WebCore/page/ApplicationStateChangeListener.h:39
> +    virtual void applicationWillResignActive(Document&) { };
> +    virtual void applicationDidBecomeActive(Document&) { };
> +
> +    enum class SuspendState { Suspended, SuspendedUnderLock };
> +    virtual void applicationDidEnterBackground(Document&, SuspendState) { };
> +    virtual void applicationWillEnterForeground(Document&, SuspendState) { };

I don’t think these functions need to receive a Document& argument. I know that in Objective-C delegation patterns we often pass the object, but this is internal to WebKit and this might not be helpful. It’s very easy for DOM nodes to get their document.

The did/will enter foreground/background functions are not used yet. Can we wait to add them until we need them instead of adding them now?

What about the SuspendState? Will clients really need these? And is it better to use argument to communicate this rather than four separate functions?

If we do need to communicate the suspend state and decide we want to use an argument instead of additional separate functions, I suggest we just use a boolean; our rule of thumb is that we use enumerations when callers are going to be passing constants that would otherwise be mysterious. But there are no callers like that.

No need for any of these semicolons after the "{ }".
Comment 4 youenn fablet 2018-01-13 15:02:22 PST
Comment on attachment 331261 [details]
Patch

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

> Source/WebCore/dom/Document.h:1411
> +

All these state changes are probably the same for all documents of a page.
How about moving these methods to the page and having media elements register to the page directly?
Comment 5 Darin Adler 2018-01-13 16:35:15 PST
Comment on attachment 331261 [details]
Patch

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

>> Source/WebCore/dom/Document.h:1411
>> +
> 
> All these state changes are probably the same for all documents of a page.
> How about moving these methods to the page and having media elements register to the page directly?

The great thing about a document is that it always outlasts all the elements in the document. That’s why it’s a great place to register things. The page lifecycle is not such a good fit.

I think it’s OK to register on the document, but I do agree that we should keep the minimum on the document; that’s why I suggested exposing just three functions (add/remove/forEach) on Document.
Comment 6 Eric Carlson 2018-01-16 09:08:38 PST
Created attachment 331391 [details]
Patch
Comment 7 Eric Carlson 2018-01-16 09:13:29 PST
Comment on attachment 331261 [details]
Patch

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

>> Source/WebCore/dom/Document.h:1410
>> +    void applicationDidBecomeActive();
> 
> Perhaps instead of these we can just expose a single function:
> 
>     forEachApplicationStateChangeListener(const Function<void(ApplicationStateChangeListener&)>&);
> 
> I think this would cut down the total amount of code. The functions in Page can do more of the work.

Good idea, changed.

>> Source/WebCore/dom/Document.h:1895
>> +    HashSet<ApplicationStateChangeListener*> m_applicationStateChangeListener;
> 
> Should be m_applicationStateChangeListeners, plural.

Fixed.

>> Source/WebCore/page/ApplicationStateChangeListener.h:39
>> +    virtual void applicationWillEnterForeground(Document&, SuspendState) { };
> 
> I don’t think these functions need to receive a Document& argument. I know that in Objective-C delegation patterns we often pass the object, but this is internal to WebKit and this might not be helpful. It’s very easy for DOM nodes to get their document.
> 
> The did/will enter foreground/background functions are not used yet. Can we wait to add them until we need them instead of adding them now?
> 
> What about the SuspendState? Will clients really need these? And is it better to use argument to communicate this rather than four separate functions?
> 
> If we do need to communicate the suspend state and decide we want to use an argument instead of additional separate functions, I suggest we just use a boolean; our rule of thumb is that we use enumerations when callers are going to be passing constants that would otherwise be mysterious. But there are no callers like that.
> 
> No need for any of these semicolons after the "{ }".

Removed all parameters and semicolons.
Comment 8 WebKit Commit Bot 2018-01-16 11:46:45 PST
Comment on attachment 331391 [details]
Patch

Clearing flags on attachment: 331391

Committed r226990: <https://trac.webkit.org/changeset/226990>
Comment 9 WebKit Commit Bot 2018-01-16 11:46:46 PST
All reviewed patches have been landed.  Closing bug.