Closed Bug 1062654 Opened 11 years ago Closed 11 years ago

Create h264 decoder based on Apple's Video Decode Acceleration framework

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(8 files, 30 obsolete files)

2.43 KB, patch
Details | Diff | Splinter Review
9.21 KB, patch
Details | Diff | Splinter Review
7.41 KB, patch
Details | Diff | Splinter Review
13.87 KB, patch
Details | Diff | Splinter Review
6.16 KB, patch
Details | Diff | Splinter Review
14.96 KB, patch
Details | Diff | Splinter Review
1.63 KB, patch
Details | Diff | Splinter Review
37.82 KB, patch
Details | Diff | Splinter Review
Currently, the h264 decoder on mac is using the Video Toolbox framework introduced in Mac OS X 10.7. In 10.9, it became possible to enable hardware acceleration, however this currently causes crashes (bug 1062596). The aim of this bug is to track the implementation of a new decoder using the VDA framework (https://developer.apple.com/library/mac/technotes/tn2267/_index.html). VDA is available on certain mac with compatible graphic cards, and available from 10.6.3. The plan would be to try to use the VDA decoder first, and should it fail or not be available with the current hardware. to revert to the current Video Toolbox decoder. There are known issues with VDA with some hardware in 10.7 and earlier with some H264 content (in particular interlaced), which can result in kernel panic. However, those have now mostly all been identified and an exception can be added for those. The use of VDA allows for playback of highly complex h264 content with minimal CPU usage
See Also: → 1062596
Blocks: 799318
Attached patch Implement VDA Decoder (obsolete) — Splinter Review
Preliminary VDA implementation. Work in progress
Depends on: 1059066
Depends on: 1062596
Attached patch Implement Apple VDA decoder (obsolete) — Splinter Review
Implement VDA decoder
Attachment #8485512 - Flags: review?(giles)
Attached patch Integrate and enable VDA decoder (obsolete) — Splinter Review
Integrate VDA decoder. First try VDA then default to VT should a problem occurred
Attachment #8485513 - Flags: review?(giles)
Derive VT decoder from VDA. The VideoToolbox is heavily derived from VDA's API. This simplifies the code and reduce size
Attachment #8485514 - Flags: review?(giles)
Fix Link/Unlink video framework cycles. The framework was in effect always loaded as MP4Reader::IsAppleAvailable called AppleVTDecoder::Link to check if VT was available. Resulting in a link-count of 8 for every load. Also favor VideoToolbox on 10.9 as HW acceleration is now available. Only attempt VDA on earlier systems
Attachment #8485516 - Flags: review?(giles)
Attachment #8485400 - Attachment is obsolete: true
Comment on attachment 8485516 [details] [diff] [review] Fix Link/Unlink Apple video frameworks cycle Review of attachment 8485516 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/apple/AppleDecoderModule.cpp @@ +48,5 @@ > + Gestalt(gestaltSystemVersionMajor, &majorVersion); > + Gestalt(gestaltSystemVersionMinor, &minorVersion); > + Gestalt(gestaltSystemVersionBugFix, &bugFix); > + > + if (majorVersion >= 10 && minorVersion >= 9) { This test is wrong. It should be: if (majorVersion > 10 || (majorVersion >= 10 && minorVersion >= 9)) { @@ +52,5 @@ > + if (majorVersion >= 10 && minorVersion >= 9) { > + // VideoToolbox supports hardware acceleration from 10.9. > + sIsVTHWEnabled = true; > + } > + if (majorVersion >= 10 && minorVersion >= 7) { Same as above.
(In reply to Masatoshi Kimura [:emk] from comment #6) > Comment on attachment 8485516 [details] [diff] [review] > Fix Link/Unlink Apple video frameworks cycle > > Review of attachment 8485516 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/fmp4/apple/AppleDecoderModule.cpp > @@ +48,5 @@ > > + Gestalt(gestaltSystemVersionMajor, &majorVersion); > > + Gestalt(gestaltSystemVersionMinor, &minorVersion); > > + Gestalt(gestaltSystemVersionBugFix, &bugFix); > > + > > + if (majorVersion >= 10 && minorVersion >= 9) { > > This test is wrong. It should be: majorVersion is always 10.
Add support for 2025's Mac OS 11. Assuming VDA would still be supported then
Attachment #8485572 - Flags: review?(giles)
Attachment #8485516 - Attachment is obsolete: true
Attachment #8485516 - Flags: review?(giles)
Reshuffle version tests
Attachment #8485573 - Flags: review?(giles)
Attachment #8485572 - Attachment is obsolete: true
Attachment #8485572 - Flags: review?(giles)
Comment on attachment 8485512 [details] [diff] [review] Implement Apple VDA decoder Review of attachment 8485512 [details] [diff] [review]: ----------------------------------------------------------------- r=me with issues addressed. Thanks! ::: content/media/fmp4/apple/AppleVDADecoder.cpp @@ +74,1 @@ > } mInitialized = false; here too, right? @@ +98,5 @@ > { > mTaskQueue->Flush(); > + OSStatus rv = VDADecoderFlush(mDecoder, 0 /*dont emit*/); > + if (rv != noErr) { > + LOG("AppleVDADecoder::Flush failed waiting for platform decoder."); Presumedly this doesn't happen often. Worth logging the error value as well. @@ +111,3 @@ > { > mTaskQueue->AwaitIdle(); > + OSStatus rv = VDADecoderFlush(mDecoder, 0 /*dont emit*/); For Drain() you _do_ want the flush to emit any in-flight frames, since the reader calls this after it's submitted the last frame to make sure all output is returned. @@ +153,5 @@ > + AppleVDADecoder* decoder = > + static_cast<AppleVDADecoder*>(decompressionOutputRefCon); > + > + AutoCFRelease<CFNumberRef> ptsref = > + (CFNumberRef)CFDictionaryGetValue(frameInfo, CFSTR("FRAME_PTS")); CFSTR() leaks. I suggest AppleUtils::GetCFDict(frameInfo "FRAME_PTS"); @@ +295,5 @@ > + const void* keys[] = { CFSTR("FRAME_PTS"), > + CFSTR("FRAME_DTS"), > + CFSTR("FRAME_DURATION"), > + CFSTR("FRAME_OFFSET"), > + CFSTR("FRAME_KEYFRAME") }; Here you'll have to use AutoCFRelease<CFStringRef>s :( @@ +335,5 @@ > + OSType format = 'avc1'; > + AutoCFRelease<CFNumberRef> avc_height = > + CFNumberCreate(kCFAllocatorDefault, > + kCFNumberSInt32Type, > + &mConfig.display_height); I used NULL instead of kCFAllocatorDefault my my code because it's sorter and 'NULL for default value' is a common C idiom. I don't object to using the constant instead; it is more readable. But if you want to change it, I'd prefer you use NULL here and file a separate bug to change all the code at once. Or fix my code if you're rather have a dependent bug. :) @@ +343,5 @@ > + &mConfig.display_width); > + AutoCFRelease<CFNumberRef> avc_format = > + CFNumberCreate(kCFAllocatorDefault, > + kCFNumberSInt32Type, > + &format); kCMVideoCodecType_H264? @@ +356,5 @@ > + AppleVDALinker::GetPropAVCCData() }; > + const void* decoderValue[] = { avc_height, > + avc_width, > + avc_format, > + avc_data }; Maybe static_assert() that the ArrayLength()s are equal here? It can be hard to count to 4 visually. @@ +363,5 @@ > + decoderKeys, > + decoderValue, > + ArrayLength(decoderKeys), > + &kCFTypeDictionaryKeyCallBacks, > + &kCFTypeDictionaryValueCallBacks); All this could be in a CreateDecoderConfiguration() helper. @@ +381,4 @@ > return NS_ERROR_FAILURE; > } > > + mInitialized = true; What is this protecting? It seems to reflect mDecoder != nullptr. Are you worried about Init() being called multiple times? If it's not, might it be better to MOZ_ASSERT(!mDecoder) to catch the leak? @@ +408,5 @@ > kCVPixelBufferPixelFormatTypeKey, > kCVPixelBufferOpenGLCompatibilityKey }; > const void* outputValues[] = { IOSurfaceProperties, > + PixelFormatTypeNumber, > + kCFBooleanTrue }; Indentation. ::: content/media/fmp4/apple/AppleVDADecoder.h @@ +27,2 @@ > > +class AppleFrameRef { Is there some reason you need the complete type here? It's better to just declare class AppleFrameRef; as an opaque type in the header and keep the implementation in AppleVDADecoder.cpp. Having implementation in the header slows down compilation of any files which include it, so there's only an advantage if isn't something the caller needs to inline. @@ +63,5 @@ > + static already_AddRefed<AppleVDADecoder> CreateVDADecoder( > + const mp4_demuxer::VideoDecoderConfig& aConfig, > + MediaTaskQueue* aVideoTaskQueue, > + MediaDataDecoderCallback* aCallback, > + layers::ImageContainer* aImageContainer); I assume this factory method is used in a subsequent patch? @@ +80,4 @@ > nsresult OutputFrame(CVPixelBufferRef aImage, > + nsAutoPtr<AppleFrameRef> aFrameRef); > + > + protected: Looking ahead to the subclass patch? ::: content/media/fmp4/apple/AppleVDALinker.cpp @@ +109,2 @@ > { > + CFStringRef *address = (CFStringRef*)dlsym(sLink, symbol); CFStringRef* address ::: content/media/fmp4/apple/AppleVDALinker.h @@ +24,5 @@ > static void Unlink(); > + static CFStringRef GetPropWidth(); > + static CFStringRef GetPropHeight(); > + static CFStringRef GetPropSourceFormat(); > + static CFStringRef GetPropAVCCData(); Hmm. We did this for AppleVTLinker::GetPropHWAccel() but now that I see them all together it occurs to me there's no need for getters here. There's no validation to be done, and they're static globals, so no need for a |this| pointer. AppleVDALinker::skPropWidth is just as clear as AppleVDALinker::GetPropWidth(). ::: content/media/fmp4/apple/VideoDecodeAcceleration/VDADecoder.h @@ +20,5 @@ > +typedef uint32_t VDADecodeInfoFlags; > + > +enum { > + kVDADecodeInfo_Asynchronous = 1UL << 0, > + kVDADecodeInfo_FrameDropped = 1UL << 1 Two-space indent. @@ +37,5 @@ > + kVDADecoderHardwareNotSupportedErr = -12470, > + kVDADecoderFormatNotSupportedErr = -12471, > + kVDADecoderConfigurationError = -12472, > + kVDADecoderDecoderFailedErr = -12473, > +}; You don't use these. Unless you need them for a later patch, please remove. @@ +42,5 @@ > + > +typedef struct OpaqueVDADecoder* VDADecoder; > + > +typedef void (*VDADecoderOutputCallback) > + (void *decompressionOutputRefCon, void* decompressionOutputRefCon, @@ +52,5 @@ > +//decoderConfiguration keys > +extern const CFStringRef kVDADecoderConfiguration_Height; // CFNumber > +extern const CFStringRef kVDADecoderConfiguration_Width; // CFNumber > +extern const CFStringRef kVDADecoderConfiguration_SourceFormat; // CFNumber - SInt32, FourCharacterCode for format > +extern const CFStringRef kVDADecoderConfiguration_avcCData; // avc1 decoder configuration data Spaces, not tabs, please. @@ +60,5 @@ > + CFDictionaryRef decoderConfiguration, > + CFDictionaryRef destinationImageBufferAttributes, /* can be NULL */ > + VDADecoderOutputCallback *outputCallback, > + void *decoderOutputCallbackRefcon, > + VDADecoder *decoderOut); Pointer goes with the type, not the argument name. We also don't usually space argument names into columns in mozilla code.
Attachment #8485512 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #10) > Comment on attachment 8485512 [details] [diff] [review] > Implement Apple VDA decoder > > Review of attachment 8485512 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with issues addressed. Thanks! > > ::: content/media/fmp4/apple/AppleVDADecoder.cpp > @@ +74,1 @@ > > } > > mInitialized = false; here too, right? yes. > > @@ +98,5 @@ > > { > > mTaskQueue->Flush(); > > + OSStatus rv = VDADecoderFlush(mDecoder, 0 /*dont emit*/); > > + if (rv != noErr) { > > + LOG("AppleVDADecoder::Flush failed waiting for platform decoder."); > > Presumedly this doesn't happen often. Worth logging the error value as well. This is a copy of the VTDecoder code, should I change both ? > > @@ +111,3 @@ > > { > > mTaskQueue->AwaitIdle(); > > + OSStatus rv = VDADecoderFlush(mDecoder, 0 /*dont emit*/); > > For Drain() you _do_ want the flush to emit any in-flight frames, since the > reader calls this after it's submitted the last frame to make sure all > output is returned. Ah right ... thanks ! > > @@ +153,5 @@ > > + AppleVDADecoder* decoder = > > + static_cast<AppleVDADecoder*>(decompressionOutputRefCon); > > + > > + AutoCFRelease<CFNumberRef> ptsref = > > + (CFNumberRef)CFDictionaryGetValue(frameInfo, CFSTR("FRAME_PTS")); > > CFSTR() leaks. I suggest AppleUtils::GetCFDict(frameInfo "FRAME_PTS"); that's not how I read CFSTR() macro to work: "CFSTR(), not being a "Copy" or "Create" function, does not return a new reference for you. So, you should not release the return value. This is much like constant C or Pascal strings --- when you use "hello world" in a program, you do not free it." AppleUtils::GetCFDict maybe be clearer anyway... > > @@ +295,5 @@ > > + const void* keys[] = { CFSTR("FRAME_PTS"), > > + CFSTR("FRAME_DTS"), > > + CFSTR("FRAME_DURATION"), > > + CFSTR("FRAME_OFFSET"), > > + CFSTR("FRAME_KEYFRAME") }; > > Here you'll have to use AutoCFRelease<CFStringRef>s :( Well, according to CFSTR you don't :) > > @@ +335,5 @@ > > + OSType format = 'avc1'; > > + AutoCFRelease<CFNumberRef> avc_height = > > + CFNumberCreate(kCFAllocatorDefault, > > + kCFNumberSInt32Type, > > + &mConfig.display_height); > > I used NULL instead of kCFAllocatorDefault my my code because it's sorter > and 'NULL for default value' is a common C idiom. I felt the other way round. I felt that NULL wasn't clear even though they are indeed the same. I found myself reading the doc over and over to make sure it was all fine. > > I don't object to using the constant instead; it is more readable. But if > you want to change it, I'd prefer you use NULL here and file a separate bug > to change all the code at once. Or fix my code if you're rather have a > dependent bug. :) I can use NULL first, and then kCFAllocatorDefault in another patch, but I can already see the headaches it's going to cause maintaining both in series... > > @@ +343,5 @@ > > + &mConfig.display_width); > > + AutoCFRelease<CFNumberRef> avc_format = > > + CFNumberCreate(kCFAllocatorDefault, > > + kCFNumberSInt32Type, > > + &format); > > kCMVideoCodecType_H264? no... VDA doesn't rely on CoreMedia (which only exists in 10.7). VDA is purely h264 only. Format here indicate the extradata format (annex b or avcc) > Maybe static_assert() that the ArrayLength()s are equal here? It can be hard > to count to 4 visually. ok, > > @@ +363,5 @@ > > + decoderKeys, > > + decoderValue, > > + ArrayLength(decoderKeys), > > + &kCFTypeDictionaryKeyCallBacks, > > + &kCFTypeDictionaryValueCallBacks); > > All this could be in a CreateDecoderConfiguration() helper. will do. > > @@ +381,4 @@ > > return NS_ERROR_FAILURE; > > } > > > > + mInitialized = true; > > What is this protecting? It seems to reflect mDecoder != nullptr. Originally, I had TryVDADecoder() which would return a boolean. It would create a VDA decoder, then check that it succeeded and return. This was to be used by the AppleDecoderModule to check if VDA was available on the platform. However, I had concern that creating the VDA decoder, and succeeding, to delete it again could have the potential of not working right after as there could be a delay between the time the object is deleted, and then effectively released by the garbage collector. So instead I changed it and made CreateVDADecoder that can return a nullable. The instance returned is to be reused and the Init() has already been called on it. I wanted to separate and still be able to construct the VDADecoder in the same fashion as the VTDecoder (where mDecoder would have been created, but Init() hasn't been called on it yet). This is what mInitialized does. I will document it. > > Are you worried about Init() being called multiple times? If it's not, might > it be better to MOZ_ASSERT(!mDecoder) to catch the leak? There could be a way to using mDecoder value instead... I'll see to that, > ::: content/media/fmp4/apple/AppleVDADecoder.h > @@ +27,2 @@ > > > > +class AppleFrameRef { > > Is there some reason you need the complete type here? > > It's better to just declare > > class AppleFrameRef; > because it will be used by the VTDecoder in the Part3 patch, I can keep the implementation obscured and only declare the prototype. There are two different constructor for that class. It can't be fully opaque > @@ +63,5 @@ > > + static already_AddRefed<AppleVDADecoder> CreateVDADecoder( > > + const mp4_demuxer::VideoDecoderConfig& aConfig, > > + MediaTaskQueue* aVideoTaskQueue, > > + MediaDataDecoderCallback* aCallback, > > + layers::ImageContainer* aImageContainer); > > I assume this factory method is used in a subsequent patch? yes, as soon as I started working on VDA, I realised how similar that APIs were and in fact I've worked right away on having VT being derived from VDA. I split the patches to make it more clear, but it was developed from the ground up as one being derived from the other. > > @@ +80,4 @@ > > nsresult OutputFrame(CVPixelBufferRef aImage, > > + nsAutoPtr<AppleFrameRef> aFrameRef); > > + > > + protected: > > Looking ahead to the subclass patch? yes... do you want me to keep that separated for the time being and move OutputFrame to be a private member instead? To me that's unnecessary step as really, all those patches are to be looked upon as a whole. > > + CFStringRef *address = (CFStringRef*)dlsym(sLink, symbol); > > CFStringRef* address hmm.. actually I had that fixed in the Re-enable HW decoder patch... > Hmm. We did this for AppleVTLinker::GetPropHWAccel() but now that I see them > all together it occurs to me there's no need for getters here. There's no > validation to be done, and they're static globals, so no need for a |this| > pointer. > > AppleVDALinker::skPropWidth is just as clear as > AppleVDALinker::GetPropWidth(). old habits I guess, I always use Getter/Setter normally, regardless on how its actually implemented. I see value in both approaches. Will use the constant then, and create a new patch for the HWAccel constant. > > ::: content/media/fmp4/apple/VideoDecodeAcceleration/VDADecoder.h > @@ +20,5 @@ > > +typedef uint32_t VDADecodeInfoFlags; > > + > > +enum { > > + kVDADecodeInfo_Asynchronous = 1UL << 0, > > + kVDADecodeInfo_FrameDropped = 1UL << 1 > > Two-space indent. ok.. was a direct copy/paste from Apple's private header > > @@ +37,5 @@ > > + kVDADecoderHardwareNotSupportedErr = -12470, > > + kVDADecoderFormatNotSupportedErr = -12471, > > + kVDADecoderConfigurationError = -12472, > > + kVDADecoderDecoderFailedErr = -12473, > > +}; > > You don't use these. Unless you need them for a later patch, please remove. will do > > @@ +42,5 @@ > > + > > +typedef struct OpaqueVDADecoder* VDADecoder; > > + > > +typedef void (*VDADecoderOutputCallback) > > + (void *decompressionOutputRefCon, > > void* decompressionOutputRefCon, > > @@ +52,5 @@ > > +//decoderConfiguration keys > > +extern const CFStringRef kVDADecoderConfiguration_Height; // CFNumber > > +extern const CFStringRef kVDADecoderConfiguration_Width; // CFNumber > > +extern const CFStringRef kVDADecoderConfiguration_SourceFormat; // CFNumber - SInt32, FourCharacterCode for format > > +extern const CFStringRef kVDADecoderConfiguration_avcCData; // avc1 decoder configuration data > > Spaces, not tabs, please. from Apple, I don't need them anyway. They are remnant of an earlier attempt of mine to attempt to resolve them at runtime, which failed miserably > > @@ +60,5 @@ > > + CFDictionaryRef decoderConfiguration, > > + CFDictionaryRef destinationImageBufferAttributes, /* can be NULL */ > > + VDADecoderOutputCallback *outputCallback, > > + void *decoderOutputCallbackRefcon, > > + VDADecoder *decoderOut); > > Pointer goes with the type, not the argument name. We also don't usually > space argument names into columns in mozilla code. yeah, those were the Apple formatting, it's a straight copy/paste from their headers. I will mozilla them :) Thanks for the review.
(In reply to Jean-Yves Avenard [:jya] from comment #11) > > > + LOG("AppleVDADecoder::Flush failed waiting for platform decoder."); > > > > Presumedly this doesn't happen often. Worth logging the error value as well. > > This is a copy of the VTDecoder code, should I change both ? Yes please. Can/should be a separate commit. > that's not how I read CFSTR() macro to work: > "CFSTR(), not being a "Copy" or "Create" function, does not return a new > reference for you. So, you should not release the return value. This is > much like constant C or Pascal strings --- when you use "hello world" > in a program, you do not free it." I'm sure it wraps the constant string, but has to be allocating the wrapper struct, no? "An immutable string, or NULL if there was a problem creating the object...it will remain valid until the program terminates." OTOH, I can't get it to allocate much memory. So maybe you're right. Or it's doing some de-dup in the background? > I can use NULL first, and then kCFAllocatorDefault in another patch, but I > can already see the headaches it's going to cause maintaining both in > series... Why I suggested you fix my code in a separate bug and land that first. > > kCMVideoCodecType_H264? > > no... VDA doesn't rely on CoreMedia (which only exists in 10.7). VDA is > purely h264 only. Ok. > > > + mInitialized = true; > > > > What is this protecting? It seems to reflect mDecoder != nullptr. > > Originally, I had TryVDADecoder() which would return a boolean. It would > create a VDA decoder, then check that it succeeded and return. This was to > be used by the AppleDecoderModule to check if VDA was available on the > platform. > > However, I had concern that creating the VDA decoder, and succeeding, to > delete it again could have the potential of not working right after as there > could be a delay between the time the object is deleted, and then > effectively released by the garbage collector. > > So instead I changed it and made CreateVDADecoder that can return a > nullable. The instance returned is to be reused and the Init() has already > been called on it. > I wanted to separate and still be able to construct the VDADecoder in the > same fashion as the VTDecoder (where mDecoder would have been created, but > Init() hasn't been called on it yet). > > This is what mInitialized does. I will document it. That makes sense. If you document it, it's fine. > To me that's unnecessary step as really, all those patches are to be looked > upon as a whole. Well, it's a trade off. Separate commits can be easier to review, and make the history easier to understand. They're also more work to maintain until your code lands. > old habits I guess, I always use Getter/Setter normally, regardless on how > its actually implemented. > I see value in both approaches. Yes of course. Trivial getters are a good idea when API (or ABI) stability is important. That's not the case here. > ok.. was a direct copy/paste from Apple's private header Keep in mind this isn't the Apple header. We'd prefer to use that directly, but it's not license-compatible. This is Mozilla code describing a compatible interface, not third party code, so Mozilla code style is appropriate.
(In reply to Ralph Giles (:rillian) from comment #12) > (In reply to Jean-Yves Avenard [:jya] from comment #11) > > > > > + LOG("AppleVDADecoder::Flush failed waiting for platform decoder."); > > > > > > Presumedly this doesn't happen often. Worth logging the error value as well. > > > > This is a copy of the VTDecoder code, should I change both ? > > Yes please. Can/should be a separate commit. > > > that's not how I read CFSTR() macro to work: > > "CFSTR(), not being a "Copy" or "Create" function, does not return a new > > reference for you. So, you should not release the return value. This is > > much like constant C or Pascal strings --- when you use "hello world" > > in a program, you do not free it." > > I'm sure it wraps the constant string, but has to be allocating the wrapper > struct, no? "An immutable string, or NULL if there was a problem creating > the object...it will remain valid until the program terminates." where did you read that it could return NULL ? > > OTOH, I can't get it to allocate much memory. So maybe you're right. Or it's > doing some de-dup in the background? googling about it, which seems to be the general answer in regards to questions around CFSTR and leaks, it appears to be identical as doing in Obj-C: @"text". In C you would do CFSTR("text") instead. There's no memory allocated. FWIW, that's how I've always used CFSTR, having said that, I could have been wrong all those times :) Looking at various Apple docs: https://developer.apple.com/library/ios/documentation/CoreFoundation/Conceptual/CFCollections/Articles/creating.html they use CFSTR in the same fashion. They also use CFSTR in the same way as me in LLVM source code: http://llvm.org/klaus/lldb/blob/29575840d868ee82547fd5f3a22a47cb5cbdca74/source/Host/macosx/Host.mm#L-1311 they use it in the same manner in chrome code when creating a non-mutable CF dictionary: https://github.com/adobe/chromium/blob/master/chrome/common/mac/mock_launchd.cc#L150
Replace NULL with kCFAllocatorDefault for clarity
Attachment #8486286 - Flags: review?(giles)
Attached patch Use official constants (obsolete) — Splinter Review
Use CoreMedia Framework official constants
Attachment #8486287 - Flags: review?(giles)
Attached patch Implement Apple VDA decoder (obsolete) — Splinter Review
Carrying review comments.
Attachment #8486289 - Flags: review?(giles)
Attachment #8485512 - Attachment is obsolete: true
Rebase patch, obfuscate implementation of AppleFrameRef
Attachment #8486291 - Flags: review?(giles)
Attachment #8485514 - Attachment is obsolete: true
Attachment #8485514 - Flags: review?(giles)
Rebase patch
Attachment #8486294 - Flags: review?(giles)
Attachment #8485573 - Attachment is obsolete: true
Attachment #8485573 - Flags: review?(giles)
Attached patch Implement Apple VDA decoder (obsolete) — Splinter Review
fix stray space
Attachment #8486302 - Flags: review?(giles)
Attachment #8486289 - Attachment is obsolete: true
Attachment #8486289 - Flags: review?(giles)
Patch are to be applied in this order (bugzilla doesn't show the file name, only sorted by date) 1062654_Part0_Use_kCFAllocatorDefault.patch : Replace NULL allocator with kCFAllocatorDefault for clarity 1062654_Part0b_Use_VT_CM_constants.patch : Use official constants 1062654_Part1_Add_VDA_Decoder.patch : Implement Apple VDA decoder 1062654_Part2_Integrate_VDA_Decoder.patch : Integrate and enable VDA decoder 1062654_Part3_Derive_VT_Decoder_From_VDA.patch : Derive VT decoder from VDA decoder 1062654_Fix_Link_Unlink_Cycle.patch : Fix Link/Unlink Apple video frameworks cycle
Re-base
Attachment #8486304 - Flags: review?(giles)
Attachment #8486294 - Attachment is obsolete: true
Attachment #8486294 - Flags: review?(giles)
Provide error code when drain/flush failed. Also don't return early to allow queued frames to be drained
Attachment #8486310 - Flags: review?(giles)
Attachment #8486310 - Flags: review?(giles) → review+
Unfortunately, bugzilla doesn't know about patch ordering, and shows them in the order of attachment. It's good practice to put 'Part N: ' at the start of the description (and vM at the end) so it's clear what order to apply them in once you start uploading revised versions.
Comment on attachment 8486286 [details] [diff] [review] Replace NULL allocator with kCFAllocatorDefault for clarity Review of attachment 8486286 [details] [diff] [review]: ----------------------------------------------------------------- What about AppleUtils.cpp? It would be better to do this patch in a separate bug so we can go ahead and land it without confusion.
Attachment #8486286 - Flags: review?(giles) → review+
Comment on attachment 8486287 [details] [diff] [review] Use official constants Review of attachment 8486287 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a description of why the change was made added to the commit message. ::: content/media/fmp4/apple/AppleVTDecoder.cpp @@ +415,5 @@ > + CFDataCreate(kCFAllocatorDefault, > + mConfig.extra_data.begin(), > + mConfig.extra_data.length()); > + > + const void* atomsKey[] = { CFSTR("avcC") }; Oops. I missed a CFSTR!
Attachment #8486287 - Flags: review?(giles) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #13) > > I'm sure it wraps the constant string, but has to be allocating the wrapper > > struct, no? "An immutable string, or NULL if there was a problem creating > > the object...it will remain valid until the program terminates." > > where did you read that it could return NULL ? The CFString reference documentation. I looked into this a bit, and I think CFSTR() is ok to use in this code. The answer seems to be that on MacOS X or iOS it uses an compiler extension to generate static CFStringRef objects, so there's no allocation, and I don't see a static initializer in the disassembly. On other platforms it allocates as you'd expect. Since our code is mac-specific it's fine to use it.
Blocks: 1065111
Comment on attachment 8485513 [details] [diff] [review] Integrate and enable VDA decoder Review of attachment 8485513 [details] [diff] [review]: ----------------------------------------------------------------- I think we should work with either VDA or VT decoders, without making one depend on the other. Please address comments and let me see it again. ::: content/media/fmp4/MP4Decoder.cpp @@ +150,5 @@ > // Disabled by preference. > return false; > } > // Attempt to load the required frameworks. > + return AppleVDALinker::Link(); This makes VDA a proxy for the other frameworks. It's better to make it work if either set or frameworks are available, in case Apple removes one but not the other. How about three booleans, and then return haveVDA || (haveCoremedia && haveVideoToolbox); or whatever the correct dependency set is? ::: content/media/fmp4/apple/AppleDecoderModule.cpp @@ +16,5 @@ > > namespace mozilla { > > +bool AppleDecoderModule::sIsVTEnabled = false; > +bool AppleDecoderModule::sIsVDAEnabled = false; sIsFooAvailable might be better names now that there's two of them. @@ +37,2 @@ > return; > } I copied this from WMFDecoderModule, but I think it would be clearer to not involve sIsVDAEnabled here. There's the question of which flag to set from the preference. Really it should be both! How about just: if (!Preferences::GetBool("media.apple.mp4.enabled", false)) { return false; } @@ +55,5 @@ > { > // We don't have any per-instance initialization to do. > // Check whether ::Init() above succeeded to know if > // we're functional. > + if (!sIsVDAEnabled) { if (!sIsVDAEnabled && !sIsVTEnabled) ? @@ +103,5 @@ > + new AppleVDADecoder(aConfig, aVideoTaskQueue, aCallback, aImageContainer); > + } else { > + decoder = > + new AppleVTDecoder(aConfig, aVideoTaskQueue, aCallback, aImageContainer); > + } Reverse the order of the 'then' and 'else' clauses, and comment that we're falling back before the 'if'.
Attachment #8485513 - Flags: review?(giles)
Comment on attachment 8486291 [details] [diff] [review] Derive VT decoder from VDA decoder Review of attachment 8486291 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Attachment #8486291 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #27) > Comment on attachment 8485513 [details] [diff] [review] > Integrate and enable VDA decoder > > Review of attachment 8485513 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think we should work with either VDA or VT decoders, without making one > depend on the other. Please address comments and let me see it again. > not sure I understand. In effect, that patch on its own, and without the Link/Unlink patch isn't much of value as currently the current way only works due to a bug. Link() is called much more than Unlink() so the problem is never triggered. Once you apply this patch, the problem is triggered and it will crash once the garbage cycle is called. All your comments once the Link/Unlink patch is applied do not apply anymore, as it's already how I've done it. There's also an issue that should the frameworks are unloaded, they can't be reloaded again and any further call will crash. Maybe for the sake of clarity, and to prevent it, I should combine both patches together. It make a little bit more difficult to review things however > Reverse the order of the 'then' and 'else' clauses, and comment that we're > falling back before the 'if'. that also is done once Link/Unlink is applied
(In reply to Jean-Yves Avenard [:jya] from comment #29) > All your comments once the Link/Unlink patch is applied do not apply > anymore, as it's already how I've done it. I see that you've addressed some of these in the subsequent patch, but I'd like you to address them in this patch.
Comment on attachment 8486302 [details] [diff] [review] Implement Apple VDA decoder Review of attachment 8486302 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: content/media/fmp4/apple/AppleVDADecoder.cpp @@ +132,5 @@ > + explicit AppleFrameRef(Microseconds _dts, > + Microseconds _pts, > + Microseconds _duration, > + int64_t _byte_offset, > + bool _is_sync_point) I don't think you need 'explicit' on multi-argument ctors. The idea is to avoid implicit copy constructors on assignment, but you can't assign 5 arguments. @@ +138,5 @@ > + decode_timestamp = _dts; > + composition_timestamp = _pts; > + duration = _duration; > + byte_offset = _byte_offset; > + is_sync_point = _is_sync_point; Mozilla doesn't use the leading underscore convention (outside of macros, apparently). Local style would be AppleFrameRef(Microseconds aDts, Microseconds aPts, etc. @@ +162,2 @@ > return; > } According to Apple's VDADecoder.h, when we call Flush, it will still call our callback for each frame in flight with a NULL CVImageBufferRef and "a FrameFlushed indicator will be set in the status field". Any idea if this actually happens? I would be nice to treat such frames as expected instead of warning. TN2267 doesn't mention it, however, and I can't find a missing constant. Maybe leave a FIXME comment if you don't know either. @@ +169,2 @@ > NS_WARNING(" ...frame dropped..."); > + return; When we added this return to the VTDecoder, it caused problems? @@ +322,5 @@ > + duration, > + byte_offset, > + cfkeyframe }; > + NS_ASSERTION(ArrayLength(keys) == ArrayLength(values), > + "Invalid array declaration, check arguments."); Make this a static_assert. NS_ASSERTION is a (debug only) runtime check. @@ +361,5 @@ > > + rv = > + VDADecoderCreate(decoderConfig, > + outputConfiguration, > + (VDADecoderOutputCallback*)PlatformCallback, Why do you need the cast here? @@ +406,5 @@ > + avc_width, > + avc_format, > + avc_data }; > + NS_ASSERTION(ArrayLength(decoderKeys) == ArrayLength(decoderValue), > + "Invalid array declaration, check arguments."); static_assert @@ +441,5 @@ > const void* outputValues[] = { IOSurfaceProperties, > PixelFormatTypeNumber, > kCFBooleanTrue }; > + NS_ASSERTION(ArrayLength(outputKeys) == ArrayLength(outputKeys), > + "Invalid array declaration, check arguments."); static_assert ::: content/media/fmp4/apple/VideoDecodeAcceleration/VDADecoder.h @@ +25,5 @@ > +}; > + > +enum { > + kVDADecoderDecodeFlags_DontEmitFrame = 1 << 0 > +}; You don't use this flag. @@ +63,5 @@ > + > +OSStatus > +VDADecoderDestroy(VDADecoder decoder); > + > +#endif //mozilla_VideoDecodeAcceleration_VDADecoder_h Space between // and guard name.
Attachment #8486302 - Flags: review?(giles) → review+
Comment on attachment 8486304 [details] [diff] [review] Fix Link/Unlink Apple video frameworks cycle Review of attachment 8486304 [details] [diff] [review]: ----------------------------------------------------------------- This is bitrotted by the changes I requested for the "derive VTDecoder from VDADecoder" patch. Let me see it again after rebase.
Attachment #8486304 - Flags: review?(giles)
(In reply to Ralph Giles (:rillian) from comment #31) > Comment on attachment 8486302 [details] [diff] [review] > Implement Apple VDA decoder > > Review of attachment 8486302 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with comments addressed. > > ::: content/media/fmp4/apple/AppleVDADecoder.cpp > @@ +132,5 @@ > > + explicit AppleFrameRef(Microseconds _dts, > > + Microseconds _pts, > > + Microseconds _duration, > > + int64_t _byte_offset, > > + bool _is_sync_point) > > I don't think you need 'explicit' on multi-argument ctors. The idea is to > avoid implicit copy constructors on assignment, but you can't assign 5 > arguments. I copy paste didn't think much of it .. will do > > @@ +138,5 @@ > > + decode_timestamp = _dts; > > + composition_timestamp = _pts; > > + duration = _duration; > > + byte_offset = _byte_offset; > > + is_sync_point = _is_sync_point; > > Mozilla doesn't use the leading underscore convention (outside of macros, > apparently). Local style would be you do realise it was your original code using _ ? I only continued with the existing style :) > > AppleFrameRef(Microseconds aDts, > Microseconds aPts, etc. > > @@ +162,2 @@ > > return; > > } > > According to Apple's VDADecoder.h, when we call Flush, it will still call > our callback for each frame in flight with a NULL CVImageBufferRef and "a > FrameFlushed indicator will be set in the status field". > > Any idea if this actually happens? I would be nice to treat such frames as > expected instead of warning. TN2267 doesn't mention it, however, and I can't > find a missing constant. Maybe leave a FIXME comment if you don't know > either. I haven't seen this happening no... I thought that what was kVDADecoderDecodeFlags_DontEmitFrame, maybe we can simply exit from the output callback instead. rather than assuming an error. > > @@ +169,2 @@ > > NS_WARNING(" ...frame dropped..."); > > + return; > > When we added this return to the VTDecoder, it caused problems? I don't see the issue when returning early as I do in the VTDecoder. Returning early is the right thing to do.. > > @@ +322,5 @@ > > + duration, > > + byte_offset, > > + cfkeyframe }; > > + NS_ASSERTION(ArrayLength(keys) == ArrayLength(values), > > + "Invalid array declaration, check arguments."); > > Make this a static_assert. NS_ASSERTION is a (debug only) runtime check. ok. > > @@ +361,5 @@ > > > > + rv = > > + VDADecoderCreate(decoderConfig, > > + outputConfiguration, > > + (VDADecoderOutputCallback*)PlatformCallback, > > Why do you need the cast here? that one took me a while to figure out. As it wasn't working when doing something like: VDADecoderOutputCallback cb = PlatformCallback; rv = VDADecoderCreate(decoderConfig, outputConfiguration, &cb); it would crash According to the doc, it takes a VDADecoderOutputCallback*, while PlatformCallback is a VDADecoderOutputCallback. But I only get it to work when I pass VDADecoderOutputCallback instead. You can't static_cast, and using reinterpreter_cast it's too wide and I can't format it properly :) I think their function prototyping is just wrong. > @@ +441,5 @@ > > const void* outputValues[] = { IOSurfaceProperties, > > PixelFormatTypeNumber, > > kCFBooleanTrue }; > > + NS_ASSERTION(ArrayLength(outputKeys) == ArrayLength(outputKeys), > > + "Invalid array declaration, check arguments."); > > static_assert should I use MOZ_ASSERT instead?
(In reply to Ralph Giles (:rillian) from comment #30) > (In reply to Jean-Yves Avenard [:jya] from comment #29) > > > All your comments once the Link/Unlink patch is applied do not apply > > anymore, as it's already how I've done it. > > I see that you've addressed some of these in the subsequent patch, but I'd > like you to address them in this patch. Honestly, I see it as a waste of time. Fitting the VDA decoder in the existing broken implementation will keep it just as broken. The only way to make it work is to keep calling AppleVDALinker::Link() a lot (like AppleVTLinker::Link() is), effectively always keeping the framework in memory at all time. but if that's what you really want :)
Presumably ArrayLength(outputKeys) == ArrayLength(outputKeys) should be ArrayLength(outputKeys) == ArrayLength(outputValues)? Looks like it was changed by accident.
(In reply to Jean-Yves Avenard [:jya] from comment #33) > you do realise it was your original code using _ ? > I only continued with the existing style :) Did you? I thought this ctor was introduced in your 'implement VDADecoder' patch. In any case, consistency is better. I'm happy to fix my code too if you file a bug. > I haven't seen this happening no... I thought that what was > kVDADecoderDecodeFlags_DontEmitFrame, maybe we can simply exit from the > output callback instead. rather than assuming an error. It may not happen often. We feed frame in in decode order, so we'd need to call Flush immediately after Input to trigger it. > I don't see the issue when returning early as I do in the VTDecoder. > Returning early is the right thing to do.. Ok, great. > According to the doc, it takes a VDADecoderOutputCallback*, while > PlatformCallback is a VDADecoderOutputCallback. But I only get it to work > when I pass VDADecoderOutputCallback instead. Wow, looks like. The sample code does this too. Ok. > > > + NS_ASSERTION(ArrayLength(outputKeys) == ArrayLength(outputKeys), > > > + "Invalid array declaration, check arguments."); > > > > static_assert > > should I use MOZ_ASSERT instead? The lengths of these arrays are known at compile time, so you should use static_assert. That's better than adding a run time check. More generally, I always have to look up the difference between NS_ASSERTION and MOZ_ASSERT. MOZ_ASSERT is newer, the message is optional, and it's shorter, and it's not part of xpcom, so I tend to prefer it. Otherwise they both do the same thing, so for core gecko code it doesn't make a difference.
(In reply to Jean-Yves Avenard [:jya] from comment #34) > Honestly, I see it as a waste of time. Fitting the VDA decoder in the > existing broken implementation will keep it just as broken. > The only way to make it work is to keep calling AppleVDALinker::Link() a lot > (like AppleVTLinker::Link() is), effectively always keeping the framework in > memory at all time. It's not always fun, but having orderly history is valuable when researching why a change was made later. In my experience it's worth the effort of the rebases. One patch is about copying the VTDecoder and making it a VDADecoder. The second is about fixing a bug they both have with unloading the dependent frameworks. Conceptually separate, and could easily be separate bugs. Please make the changes and rebase the fix.
(In reply to Ralph Giles (:rillian) from comment #36) > More generally, I always have to look up the difference between NS_ASSERTION > and MOZ_ASSERT. MOZ_ASSERT is newer, the message is optional, and it's > shorter, and it's not part of xpcom, so I tend to prefer it. Otherwise they > both do the same thing, so for core gecko code it doesn't make a difference. MOZ_ASSERT is always fatal (it will turn tbpl to orange), but NS_ASSERTION is not. So MOZ_ASSERT is preferable to NS_ASSERTION.
Attached patch Use official constants (obsolete) — Splinter Review
Carrying r+. Use static_assert
Attachment #8486287 - Attachment is obsolete: true
Attachment #8486286 - Attachment is obsolete: true
Attached patch Part 0b. Use official constants (obsolete) — Splinter Review
Update title
Attachment #8487619 - Attachment is obsolete: true
(In reply to Ralph Giles (:rillian) from comment #27) > @@ +103,5 @@ > > + new AppleVDADecoder(aConfig, aVideoTaskQueue, aCallback, aImageContainer); > > + } else { > > + decoder = > > + new AppleVTDecoder(aConfig, aVideoTaskQueue, aCallback, aImageContainer); > > + } > > Reverse the order of the 'then' and 'else' clauses, and comment that we're > falling back before the 'if'. I'm not sure I understand that one (or if I do, that I agree with it :) ) We want to use VDA over VT as VDA makes use of hardware acceleration, while VT doesn't (not until 10.9). So the logic is, in order: 1- Use VDA if available and working 2- If VDA isn't working, and VT isn't available then return a dummy VDA decoder that will fail (could be a dummy VT decoder, they both behaves in the same fashion) 3- If VT is available, use it. Reverting the order would change the logic and would prioritise VT over VDA which I don't want. Note that in the Fix/Unlink patch I changed the logic as follow: 1- Use VT if available and HW decoding is available (running >= 10.9) use it 2- Use VDA if available and working 3- If VDA isn't working, and VT isn't available then return a dummy VDA decoder that will fail (could be a dummy VT decoder, they both behaves in the same fashion) 4- If VT is available, use it.
Carrying r+
Attachment #8486302 - Attachment is obsolete: true
Carrying r+
Attachment #8487719 - Flags: review?(giles)
Attachment #8485513 - Attachment is obsolete: true
Rename title
Attachment #8486291 - Attachment is obsolete: true
Reworked to be more in line with earlier comments
Attachment #8487721 - Flags: review?(giles)
Attachment #8486304 - Attachment is obsolete: true
Attachment #8486310 - Attachment is obsolete: true
Comment on attachment 8487623 [details] [diff] [review] Part 0b. Use official constants Review of attachment 8487623 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing the commit message. Note there should be a blank line between the first summary line and the body.
Attachment #8487623 - Flags: review+
Comment on attachment 8487719 [details] [diff] [review] Part 2: Integrate and enable VDA decoder Review of attachment 8487719 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/MP4Decoder.cpp @@ +25,2 @@ > #include "apple/AppleCMLinker.h" > #include "apple/AppleVTLinker.h" Alphabetical order?
Attachment #8487719 - Flags: review?(giles) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #42) > > Reverse the order of the 'then' and 'else' clauses, and comment that we're > > falling back before the 'if'. > > I'm not sure I understand that one (or if I do, that I agree with it :) ) Sorry, I meant the order of the clauses, not the logic. What you did in the new patch is what I wanted. Thanks!
Comment on attachment 8487721 [details] [diff] [review] Part 4: Fix Link/Unlink Apple video frameworks cycle Review of attachment 8487721 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/apple/AppleDecoderModule.cpp @@ +35,5 @@ > AppleDecoderModule::Init() > { > MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread."); > > + sForceVDA = Preferences::GetBool("media.apple.forcevda.enabled", false); "media.apple.forcevda" is sufficient, I think. @@ +52,5 @@ > // paired Link/Unlink calls > + bool haveVideoToolbox = AppleVTLinker::Link(); > + sIsVTAvailable = haveCoreMedia && haveVideoToolbox; > + > + sIsVTHWAvailable = AppleVTLinker::skPropHWAccel; I'm surprised this doesn't warn. C++ never ceases to amaze. @@ +89,5 @@ > + > +class LinkTask : public nsRunnable { > +public: > + NS_IMETHOD Run() MOZ_OVERRIDE { > + MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread."); MOZ_ASSERT(AppleDecoderModule::sInitialized); ?
Attachment #8487721 - Flags: review?(giles) → review+
To fix the opt build warning, check the return value from VDADecoderDecode() and return an error instead of asserting. I didn't look at the unbalanced Unlink().
(In reply to Ralph Giles (:rillian) from comment #51) > > + sIsVTHWAvailable = AppleVTLinker::skPropHWAccel; > > I'm surprised this doesn't warn. C++ never ceases to amaze. I classify this in the same basket as testing for pointers being non null: if (AppleVTLinker::skPropHWAccel) { blah; } I'm not a fan of this implicit test; I find it much more elegant and less confusing to explicitly check fur nullity.
(In reply to Ralph Giles (:rillian) from comment #49) > Comment on attachment 8487719 [details] [diff] [review] > Part 2: Integrate and enable VDA decoder > > Review of attachment 8487719 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/fmp4/MP4Decoder.cpp > @@ +25,2 @@ > > #include "apple/AppleCMLinker.h" > > #include "apple/AppleVTLinker.h" > > Alphabetical order? I like that they are grouped logically there: VDA / CM+VT
Attached patch Part 0b: Use official constants (obsolete) — Splinter Review
Carrying r+
Attachment #8487623 - Attachment is obsolete: true
Comment on attachment 8488335 [details] [diff] [review] Part 0b: Use official constants Review of attachment 8488335 [details] [diff] [review]: ----------------------------------------------------------------- Now you just need to start wrapping the commit messages to 72 columns... :)
Attachment #8488335 - Flags: review+
(In reply to Ralph Giles (:rillian) from comment #57) > Comment on attachment 8488335 [details] [diff] [review] > Part 0b: Use official constants > > Review of attachment 8488335 [details] [diff] [review]: > ----------------------------------------------------------------- > > Now you just need to start wrapping the commit messages to 72 columns... :) there too ??? oh boy...
See Also: → 1066369
Attached patch Part 0b: Use official constants (obsolete) — Splinter Review
Carrying r+ (again)
Attachment #8488335 - Attachment is obsolete: true
Carrying r+. Fix compilation warning
Attachment #8487707 - Attachment is obsolete: true
Carrying r+. Fix Unlink() assert on 10.6
Attachment #8487721 - Attachment is obsolete: true
Blocks: 1066369
Make AppleDecoderModule::CreateH264Decoder returns nullptr when an error occurs. Avoid recreating a VDA decoder for no real purpose
Attachment #8487719 - Attachment is obsolete: true
Comment on attachment 8488367 [details] [diff] [review] Part 1: Implement Apple VDA decoder Review of attachment 8488367 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/fmp4/apple/AppleVDADecoder.cpp @@ +408,4 @@ > > + const void* decoderKeys[] = { AppleVDALinker::skPropHeight, > + AppleVDALinker::skPropHeight, > + AppleVDALinker::skPropSourceFormat, grrrrrr.... AppleVDALinker::skPropWidth !
Fix typo causing VDA decoder creation to always fail.
Attachment #8488367 - Attachment is obsolete: true
'See also' is primarily for referencing bugs in other trackers (github, chrome, etc.). Putting bug numbers in the Depends/Blocks fields is sufficient for bugs here.
See Also: 1062596, 1066369
Attached patch Part6: Disable VDA in 10.6 (obsolete) — Splinter Review
Disable VDA in 10.6 (for now)
Attachment #8489740 - Flags: review?(giles)
Remove on NS_ASSERTION in favor of returning an error
Attachment #8488469 - Attachment is obsolete: true
Blocks: 1067697
Depends on: 1064847
Attachment #8487621 - Attachment is obsolete: true
Re-base following bug 1064847
Attachment #8488364 - Attachment is obsolete: true
Blocks: 1068455
Re-base following bug 1064847
Attachment #8489743 - Attachment is obsolete: true
Attachment #8488449 - Attachment is obsolete: true
Attachment #8487720 - Attachment is obsolete: true
Blocks: 1068597
Comment on attachment 8489740 [details] [diff] [review] Part6: Disable VDA in 10.6 Review of attachment 8489740 [details] [diff] [review]: ----------------------------------------------------------------- Land it! ::: content/media/fmp4/apple/AppleDecoderModule.cpp @@ +37,5 @@ > MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread."); > > + SInt32 majorVersion, minorVersion; > + Gestalt(gestaltSystemVersionMajor, &majorVersion); > + Gestalt(gestaltSystemVersionMinor, &minorVersion); Gestalt issues deprecation warnings on 10.8 and later, of course. But we use it elsewhere, so fixing that would be a separate bug.
Attachment #8489740 - Flags: review?(giles) → review+
Carrying r+. Remove use of Gestalt
Attachment #8489740 - Attachment is obsolete: true
Change error message to differentiate between VT and VDA error
Attachment #8490527 - Attachment is obsolete: true
Keywords: checkin-needed
No longer blocks: 1068597
No longer blocks: 1066369
See Also: → 941296
See Also: → 851290
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: