Bug 131902 – [Mac] don't ask for AVAssetTrack properties before they are available

• Status: REOPENED
• Component: Media

[Eric Carlson]

Like other AVFoundation object properties, asking for AVAssetTrack properties before they have are available will block until the data is available. AVAssetTrack implements statusOfValueForKey:error, so we should use that to ask it to load the properties we know we use, and not ask for them until they are available.

[Eric Carlson]

<rdar://16505076>

[Eric Carlson]

Created attachment 229804 [details]
Proposed patch

[Jer Noble]

Comment on attachment 229804 [details]
Proposed patch

nice! r=me.

[WebKit Commit Bot]

Comment on attachment 229804 [details]
Proposed patch

Clearing flags on attachment: 229804

Committed r167658: <http://trac.webkit.org/changeset/167658>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This caused many crashes on tests:

0   libobjc.A.dylib               	0x00007fff93636250 objc_msgSend + 16
1   com.apple.WebCore             	0x000000010e3d1344 ___ZN7WebCore34MediaPlayerPrivateAVFoundationObjC20beginLoadingMetadataEv_block_invoke97 + 52 (MediaPlayerPrivateAVFoundationObjC.mm:803)
2   libdispatch.dylib             	0x00007fff9715cf01 _dispatch_call_block_and_release + 15
3   libdispatch.dylib             	0x00007fff971590b6 _dispatch_client_callout + 8
4   libdispatch.dylib             	0x00007fff9715e0c8 _dispatch_main_queue_callback_4CF + 275
5   com.apple.CoreFoundation      	0x00007fff902b6b4c __CFRunLoopRun + 1644
6   com.apple.CoreFoundation      	0x00007fff902b60e2 CFRunLoopRunSpecific + 290
7   DumpRenderTree                	0x000000010aa9c335 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 4853 (DumpRenderTree.mm:1822)

[WebKit Commit Bot]

Re-opened since this is blocked by bug 131993

[Eric Carlson]

Created attachment 230017 [details]
Updated patch

[Brent Fulgham]

Comment on attachment 230017 [details]
Updated patch

r=me

[Darin Adler]

Comment on attachment 230017 [details]
Updated patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:809
> +    dispatch_group_notify(metadataLoadingGroup, dispatch_get_main_queue(), ^{
> +        callOnMainThread([weakThis, metadataLoadingGroup] {

I’m a little puzzled about the combination of main queue and main thread here. Won’t something dispatched on the main queue already be on the main thread?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:813
> +            dispatch_release(metadataLoadingGroup);

What guarantees that this runs after the completion handler? It looks like we could release metadataLoadingGroup before we are done with it. Maybe that’s impossible because of the enter/leave calls above?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1167
> +    return m_cachedTotalBytes;

Can this value ever total 0? If so, then we’ll keep recomputing it over and over again. Maybe the validity should be tracked separately rather than using "0" as a magic value? Or maybe the magic value could be -1?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2465
> +NSArray* assetTrackMetadataKeyNames()
> +{
> +    static NSArray* keys;
> +    if (!keys)
> +        keys = [[NSArray alloc] initWithObjects:@"totalSampleDataLength", @"mediaType", nil];
> +
> +    return keys;
> +}

No need for the if statement. Depending on the minimum compiler version guaranteed this could be either:

    NSArray *assetTrackMetadataKeyNames()
    {
        static NSArray* keys = [[NSArray alloc] initWithObjects:@"totalSampleDataLength", @"mediaType", nil];
        return keys;
    }

or:

    NSArray *assetTrackMetadataKeyNames()
    {
        return @[@"totalSampleDataLength", @"mediaType"];
    }

[Jer Noble]

(In reply to comment #10)
> (From update of attachment 230017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230017&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:809
> > +    dispatch_group_notify(metadataLoadingGroup, dispatch_get_main_queue(), ^{
> > +        callOnMainThread([weakThis, metadataLoadingGroup] {
> 
> I’m a little puzzled about the combination of main queue and main thread here. Won’t something dispatched on the main queue already be on the main thread?

(Since this was my idea, I'll answer Darin's question.)

On iOS, the main queue isn't on the web thread, and the (misnamed) callOnMainThread() will actually dispatch to the web thread, not the main thread.

This became a problem because WeakPtr::get() will assert if called from a different thread than its creation thread.

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:813
> > +            dispatch_release(metadataLoadingGroup);
> 
> What guarantees that this runs after the completion handler? It looks like we could release metadataLoadingGroup before we are done with it. Maybe that’s impossible because of the enter/leave calls above?

This is a good point.  This notify handler will get called after the dispatch_group_leave() on line 805, not after all the dispatch_group_leave()s on line 799.  The one on line 805 should be moved inside the callOnMainThread() block.

[Eric Carlson]

(In reply to comment #10)
> (From update of attachment 230017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230017&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1167
> > +    return m_cachedTotalBytes;
> 
> Can this value ever total 0? If so, then we’ll keep recomputing it over and over again. Maybe the validity should be tracked separately rather than using "0" as a magic value? Or maybe the magic value could be -1?
> 
  The total size should never be 0 once metaDataAvailable() returns true. 

> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2465
> > +NSArray* assetTrackMetadataKeyNames()
> > +{
> > +    static NSArray* keys;
> > +    if (!keys)
> > +        keys = [[NSArray alloc] initWithObjects:@"totalSampleDataLength", @"mediaType", nil];
> > +
> > +    return keys;
> > +}
> 
> No need for the if statement. Depending on the minimum compiler version guaranteed this could be either:
> 

  Fixed.

[Eric Carlson]

Created attachment 230104 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 230104 [details]
Patch for landing.

Clearing flags on attachment: 230104

Committed r167776: <http://trac.webkit.org/changeset/167776>