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.
<rdar://16505076>
Created attachment 229804 [details] Proposed patch
Comment on attachment 229804 [details] Proposed patch nice! r=me.
Comment on attachment 229804 [details] Proposed patch Clearing flags on attachment: 229804 Committed r167658: <http://trac.webkit.org/changeset/167658>
All reviewed patches have been landed. Closing bug.
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)
Re-opened since this is blocked by bug 131993
Created attachment 230017 [details] Updated patch
Comment on attachment 230017 [details] Updated patch r=me
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"]; }
(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.
(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.
Created attachment 230104 [details] Patch for landing.
Comment on attachment 230104 [details] Patch for landing. Clearing flags on attachment: 230104 Committed r167776: <http://trac.webkit.org/changeset/167776>