Closed Bug 1275217 Opened 4 years ago Closed 4 years ago

Remove QTKit dependency

Categories

(Core :: WebRTC: Audio/Video, defect, P3)

All
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file)

After bug 1180725 added support for using AVFoundation instead of QTKit and we no longer need to support Mac OSX 10.6 we should remove the QTKit related code and dependencies.
Rank: 35
Comment on attachment 8755767 [details]
MozReview Request: Bug 1275217: remove QuickTime and QTKit related code and dependecies. r=jib

https://reviewboard.mozilla.org/r/54796/#review51558

Looks like you have some build errors on try...

Otherwise looks good, except we should maybe update video_capture_mac.mm as well to not mention qtkit (maybe remove 10.4 QuickTime as well)? Or maybe it serves as a historic record, I don't know.

[1] http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/mac/video_capture_mac.mm#30
Attachment #8755767 - Flags: review?(jib)
Comment on attachment 8755767 [details]
MozReview Request: Bug 1275217: remove QuickTime and QTKit related code and dependecies. r=jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54796/diff/1-2/
Attachment #8755767 - Flags: review?(jib)
Comment on attachment 8755767 [details]
MozReview Request: Bug 1275217: remove QuickTime and QTKit related code and dependecies. r=jib

https://reviewboard.mozilla.org/r/54796/#review51656
Attachment #8755767 - Flags: review?(jib) → review+
Comment on attachment 8755767 [details]
MozReview Request: Bug 1275217: remove QuickTime and QTKit related code and dependecies. r=jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54796/diff/2-3/
Attachment #8755767 - Attachment description: MozReview Request: Bug 1275217: remove QTKit related code and dependecies. r=jib → MozReview Request: Bug 1275217: remove QuickTime and QTKit related code and dependecies. r=jib
@:jib: I took your suggest from comment #2 to heart and massively cleaned up the code in video_capture_mac.mm as well (including replacing the deprecated Gestalt interface). If you could please have a look one more time and let me know if this is still r+ from you :-)
Flags: needinfo?(jib)
https://reviewboard.mozilla.org/r/54796/#review51916

(didn't notice review was unpublished. sorry for the delay)

Loving net negative patches. There's a bug in your version check, but r=me once that's fixed.

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/video_capture_mac.mm:30
(Diff revision 3)
> -// static
> +static int intAtStringIndex(NSArray *array, int index)
> -bool CheckOSVersion()
>  {
> -    // Check OSX version
> +    return [(NSString *)[array objectAtIndex:index] integerValue];

Isn't a c-wrapper around the same call in Objective-C a bit redundant?

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/video_capture_mac.mm:57
(Diff revision 3)
> -
> +    if (minor < 8) {
> -    if (version < 0x07000000) // QT v. 7.x or newer (QT 5.0.2 0x05020000)
> -    {
> -        WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceVideoCapture, 0,
> -                     "QuickTime version too old: 0x%x", version);
> -        return false;
> +      return false;
>      }

Wont this fail on e.g. version 11.7?
Flags: needinfo?(jib)
Comment on attachment 8755767 [details]
MozReview Request: Bug 1275217: remove QuickTime and QTKit related code and dependecies. r=jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54796/diff/3-4/
https://hg.mozilla.org/mozilla-central/rev/fcedd1b3ef25
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.