Bug 191759 - [MSE][GStreamer] Refactor AppendPipeline deinitialization
Summary: [MSE][GStreamer] Refactor AppendPipeline deinitialization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alicia Boya García
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-16 11:06 PST by Alicia Boya García
Modified: 2018-11-21 00:49 PST (History)
4 users (show)

See Also:


Attachments
Patch (7.98 KB, patch)
2018-11-16 11:09 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alicia Boya García 2018-11-16 11:06:30 PST
AppendPipeline currently has a method, clearPlayerPrivate(), that the
client code uses to deinitialize most of the AppendPipeline... just
before actually destructing it in the next line of code.

Since at that point there should not be alive RefPtr's pointing to the
AppendPipeline there is no need for this kind of pre-deinitialization
in this usage pattern. Instead, we can just rely on C++ destructors,
cleaning the code a bit and removing the potential for the question
"what if `clearPlayerPrivate() has been called before?`": it has not.

Assertions have been added to ensure that there is only one alive
RefPtr pointing to AppendPipeline, therefore guaranteeing its immediate
destruction.
Comment 1 Alicia Boya García 2018-11-16 11:09:19 PST
Created attachment 355078 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2018-11-18 22:33:17 PST
Patch looks ok to me. Anyway, I'd like Enrique to have a quick look at this as well.
Comment 3 Enrique Ocaña 2018-11-21 00:12:48 PST
The patch looks good, but I suspected about problems coming from other threads still processing appends (it happened in the past).

Alicia's explanations about how the new AbortableTaskQueue would prevent non-main threads from accessing the append pipeline when it's being destroyed convinced me. She also tested player destruction in a 100 iterations loop while appends are still ongoing and didn't detect any problem related to the pipeline destruction.

I think the patch is good to be committed.
Comment 4 WebKit Commit Bot 2018-11-21 00:49:46 PST
Comment on attachment 355078 [details]
Patch

Clearing flags on attachment: 355078

Committed r238412: <https://trac.webkit.org/changeset/238412>
Comment 5 WebKit Commit Bot 2018-11-21 00:49:47 PST
All reviewed patches have been landed.  Closing bug.