Bug 135164 - [MSE][Mac] In SourceBufferPrivateAVFObjC::abort(), support reseting parser to the last appended initialization segment.
Summary: [MSE][Mac] In SourceBufferPrivateAVFObjC::abort(), support reseting parser to...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on: 135163
Blocks:
  Show dependency treegraph
 
Reported: 2014-07-22 10:57 PDT by Jer Noble
Modified: 2016-09-28 07:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.88 KB, patch)
2016-09-27 16:46 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (22.99 KB, patch)
2016-09-27 17:01 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (24.45 KB, patch)
2016-09-27 17:51 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (26.02 KB, patch)
2016-09-27 18:01 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-07-22 10:57:26 PDT
[MSE][Mac] In SourceBufferPrivateAVFObjC::abort(), support reseting parser to the last appended initialization segment.
Comment 1 Jer Noble 2016-09-27 16:46:02 PDT
Created attachment 290022 [details]
Patch
Comment 2 Jer Noble 2016-09-27 16:49:23 PDT
rdar://problem/28438637
Comment 3 Eric Carlson 2016-09-27 16:57:59 PDT
Comment on attachment 290022 [details]
Patch

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

Looks like you forgot a few files :-)

> Source/WebCore/ChangeLog:17
> +        and will block until the the previous append() operation completes.

Nit: the the

> Source/WebCore/ChangeLog:20
> +        SourceBufferPrivateAVFObjC to be reset after an abort(), so make that ivar a @property. Rather than passing a

Nit: "a @property" -> "an @property"
Comment 4 Jer Noble 2016-09-27 17:01:13 PDT
Created attachment 290026 [details]
Patch
Comment 5 Eric Carlson 2016-09-27 17:10:15 PDT
Comment on attachment 290026 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        and will block until the the previous append() operation completes.

Nit: "the the"

> Source/WebCore/ChangeLog:20
> +        SourceBufferPrivateAVFObjC to be reset after an abort(), so make that ivar a @property. Rather than passing a

Nit: "a @property" -> "an @property"

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:675
> +    dispatch_group_wait(m_isAppendingGroup.get(), DISPATCH_TIME_FOREVER);

Is it really safe to wait forever?
Comment 6 Jer Noble 2016-09-27 17:48:13 PDT
(In reply to comment #5)
> Comment on attachment 290026 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290026&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        and will block until the the previous append() operation completes.
> 
> Nit: "the the"

Ok.

> > Source/WebCore/ChangeLog:20
> > +        SourceBufferPrivateAVFObjC to be reset after an abort(), so make that ivar a @property. Rather than passing a
> 
> Nit: "a @property" -> "an @property"

Ok.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:675
> > +    dispatch_group_wait(m_isAppendingGroup.get(), DISPATCH_TIME_FOREVER);
> 
> Is it really safe to wait forever?

Now that I think of it, no, it's not.  The parsing thread may be blocked on the main thread providing it a AVStreamSession for EME-backed media elements. Apart from that, however, it should be fine.
Comment 7 Jer Noble 2016-09-27 17:51:10 PDT
Created attachment 290034 [details]
Patch for landing
Comment 8 Jer Noble 2016-09-27 18:01:29 PDT
Created attachment 290040 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2016-09-28 07:55:10 PDT
Comment on attachment 290040 [details]
Patch for landing

Clearing flags on attachment: 290040

Committed r206518: <http://trac.webkit.org/changeset/206518>