Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(41)

Issue 518313003: Move Mac media glue code from media/video/capture/mac to media/base/mac (Closed)

Created:
6 years, 3 months ago by jfroy
Modified:
6 years, 3 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Move Mac media glue code from media/video/capture/mac to media/base/mac. The upcoming OS X / iOS H.264 hardware encoder for media/cast uses CoreMedia, VideoToolbox and CoreVideo APIs that are not available in Chromium's minimum systems (10.6, 6.0). As a preparation for additional glue code to support the encoder, media glue code should be relocated to a more neutral location now that it will serve multiple "clients". BUG=409194 R=tommi, sky, rsesek, mcasas, DaleCurtis, perkj Committed: https://crrev.com/f59abe0ce199aac0cd2eccf32ad7975155dbeded Cr-Commit-Position: refs/heads/master@{#293413}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Use base/apple instead of base/mac because of automatic exclusion rule. Import Foundation.h in core… #

Patch Set 3 : Fix glue header import in device_monitor_mac.mm #

Patch Set 4 : Move media glue code back to base/mac and add suitable rules in build files to compile the code for… #

Total comments: 8

Patch Set 5 : Remove iOS-related changes to media build files. #

Patch Set 6 : Remove iOS-related changes to media build files. #

Total comments: 4

Patch Set 7 : Rebase. #

Patch Set 8 : Sort header includes by path. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -598 lines) Patch
M content/browser/device_monitor_mac.mm View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M media/BUILD.gn View 2 3 4 2 chunks +3 lines, -4 lines 0 comments Download
A media/base/mac/BUILD.gn View 1 2 3 1 chunk +16 lines, -0 lines 0 comments Download
A + media/base/mac/avfoundation_glue.h View 2 3 2 chunks +4 lines, -4 lines 0 comments Download
A + media/base/mac/avfoundation_glue.mm View 2 3 1 chunk +1 line, -1 line 0 comments Download
A + media/base/mac/coremedia_glue.h View 2 3 2 chunks +4 lines, -5 lines 0 comments Download
A + media/base/mac/coremedia_glue.mm View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M media/media.gyp View 2 3 4 2 chunks +4 lines, -4 lines 0 comments Download
D media/video/capture/mac/avfoundation_glue.h View 1 chunk +0 lines, -171 lines 0 comments Download
D media/video/capture/mac/avfoundation_glue.mm View 1 chunk +0 lines, -221 lines 0 comments Download
D media/video/capture/mac/coremedia_glue.h View 1 chunk +0 lines, -66 lines 0 comments Download
D media/video/capture/mac/coremedia_glue.mm View 1 chunk +0 lines, -114 lines 0 comments Download
M media/video/capture/mac/video_capture_device_avfoundation_mac.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M media/video/capture/mac/video_capture_device_factory_mac.mm View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M media/video/capture/mac/video_capture_device_factory_mac_unittest.mm View 2 3 1 chunk +1 line, -1 line 0 comments Download
M media/video/capture/mac/video_capture_device_mac.mm View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 45 (8 generated)
jfroy
6 years, 3 months ago (2014-08-29 22:29:04 UTC) #1
Robert Sesek
LGTM https://codereview.chromium.org/518313003/diff/1/media/base/mac/coremedia_glue.mm File media/base/mac/coremedia_glue.mm (right): https://codereview.chromium.org/518313003/diff/1/media/base/mac/coremedia_glue.mm#newcode8 media/base/mac/coremedia_glue.mm:8: #import <Foundation/NSBundle.h> Foundation.h, never the leaf headers.
6 years, 3 months ago (2014-09-02 18:52:33 UTC) #2
jfroy
https://codereview.chromium.org/518313003/diff/1/media/base/mac/coremedia_glue.mm File media/base/mac/coremedia_glue.mm (right): https://codereview.chromium.org/518313003/diff/1/media/base/mac/coremedia_glue.mm#newcode8 media/base/mac/coremedia_glue.mm:8: #import <Foundation/NSBundle.h> On 2014/09/02 18:52:33, rsesek wrote: > Foundation.h, ...
6 years, 3 months ago (2014-09-02 20:58:53 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/518313003/20001
6 years, 3 months ago (2014-09-02 21:09:41 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/518313003/40001
6 years, 3 months ago (2014-09-02 21:49:19 UTC) #7
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu_retina_triggered_tests on tryserver.chromium.gpu ...
6 years, 3 months ago (2014-09-02 22:53:31 UTC) #8
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/8256)
6 years, 3 months ago (2014-09-02 23:13:16 UTC) #10
Robert Sesek
"apple" is not a directory name we use. Switch this back to "mac" and use ...
6 years, 3 months ago (2014-09-02 23:52:25 UTC) #11
jfroy
On 2014/09/02 23:52:25, rsesek wrote: > "apple" is not a directory name we use. Switch ...
6 years, 3 months ago (2014-09-03 00:00:06 UTC) #12
mcasas
I'm curious as to why avfoundation_glue.{h,mm} needs to be teleported to base/mac;it seems like the ...
6 years, 3 months ago (2014-09-03 07:06:31 UTC) #14
jfroy
The AVFoundation glue does not need to move per se since it will continue to ...
6 years, 3 months ago (2014-09-03 16:01:27 UTC) #15
Robert Sesek
On 2014/09/03 16:01:27, jfroy wrote: > The AVFoundation glue does not need to move per ...
6 years, 3 months ago (2014-09-03 16:05:43 UTC) #16
jfroy
On 2014/09/03 16:05:43, rsesek wrote: > On 2014/09/03 16:01:27, jfroy wrote: > > The AVFoundation ...
6 years, 3 months ago (2014-09-03 17:55:43 UTC) #17
Robert Sesek
https://codereview.chromium.org/518313003/diff/60001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/518313003/diff/60001/media/BUILD.gn#newcode348 media/BUILD.gn:348: "CoreMedia.framework", This list doesn't match the one in gyp. ...
6 years, 3 months ago (2014-09-03 21:03:56 UTC) #18
Robert Sesek
On 2014/09/03 17:55:43, jfroy wrote: > On 2014/09/03 16:05:43, rsesek wrote: > > On 2014/09/03 ...
6 years, 3 months ago (2014-09-03 21:04:32 UTC) #19
jfroy
https://codereview.chromium.org/518313003/diff/60001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/518313003/diff/60001/media/BUILD.gn#newcode348 media/BUILD.gn:348: "CoreMedia.framework", On 2014/09/03 21:03:56, rsesek wrote: > This list ...
6 years, 3 months ago (2014-09-03 21:17:00 UTC) #20
Robert Sesek
https://codereview.chromium.org/518313003/diff/60001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/518313003/diff/60001/media/media.gyp#newcode890 media/media.gyp:890: '$(SDKROOT)/System/Library/Frameworks/AudioToolbox.framework', On 2014/09/03 21:17:00, jfroy wrote: > On 2014/09/03 ...
6 years, 3 months ago (2014-09-03 21:25:42 UTC) #21
jfroy
Sorry for the dummy patch set 5. git cl threw an exception...
6 years, 3 months ago (2014-09-03 21:26:01 UTC) #22
Robert Sesek
LGTM, but please get approval from mcasas and/or dalecurtis.
6 years, 3 months ago (2014-09-04 14:26:18 UTC) #23
mcasas
LGTM from my side. (There was a comment on a frame rate being changed from ...
6 years, 3 months ago (2014-09-04 16:39:25 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/518313003/100001
6 years, 3 months ago (2014-09-04 16:41:32 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/8726)
6 years, 3 months ago (2014-09-04 17:04:23 UTC) #28
jfroy
On 2014/09/04 17:04:23, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 3 months ago (2014-09-04 17:24:23 UTC) #29
jfroy
Adding owners for content/browser and media/video/capture. DaleCurtis needs to LGTM as well for media/base.
6 years, 3 months ago (2014-09-04 17:27:27 UTC) #31
tommi (sloooow) - chröme
On 2014/09/04 17:27:27, jfroy wrote: > Adding owners for content/browser and media/video/capture. DaleCurtis needs to ...
6 years, 3 months ago (2014-09-04 17:38:19 UTC) #32
tommi (sloooow) - chröme
lgtm for media/video/capture https://codereview.chromium.org/518313003/diff/100001/media/video/capture/mac/video_capture_device_avfoundation_mac.h File media/video/capture/mac/video_capture_device_avfoundation_mac.h (right): https://codereview.chromium.org/518313003/diff/100001/media/video/capture/mac/video_capture_device_avfoundation_mac.h#newcode13 media/video/capture/mac/video_capture_device_avfoundation_mac.h:13: #import "media/base/mac/avfoundation_glue.h" Should this be below ...
6 years, 3 months ago (2014-09-04 17:41:53 UTC) #33
sky
content/browser/device_monitor_mac.mm LGTM
6 years, 3 months ago (2014-09-04 17:55:24 UTC) #34
jfroy
On 2014/09/04 17:38:19, tommi wrote: > On 2014/09/04 17:27:27, jfroy wrote: > > Adding owners ...
6 years, 3 months ago (2014-09-04 18:01:17 UTC) #35
jfroy
https://codereview.chromium.org/518313003/diff/100001/media/video/capture/mac/video_capture_device_avfoundation_mac.h File media/video/capture/mac/video_capture_device_avfoundation_mac.h (right): https://codereview.chromium.org/518313003/diff/100001/media/video/capture/mac/video_capture_device_avfoundation_mac.h#newcode13 media/video/capture/mac/video_capture_device_avfoundation_mac.h:13: #import "media/base/mac/avfoundation_glue.h" On 2014/09/04 17:41:53, tommi wrote: > Should ...
6 years, 3 months ago (2014-09-04 18:05:24 UTC) #36
jfroy
https://codereview.chromium.org/518313003/diff/100001/media/video/capture/mac/video_capture_device_avfoundation_mac.h File media/video/capture/mac/video_capture_device_avfoundation_mac.h (right): https://codereview.chromium.org/518313003/diff/100001/media/video/capture/mac/video_capture_device_avfoundation_mac.h#newcode13 media/video/capture/mac/video_capture_device_avfoundation_mac.h:13: #import "media/base/mac/avfoundation_glue.h" On 2014/09/04 17:41:53, tommi wrote: > Should ...
6 years, 3 months ago (2014-09-04 18:31:08 UTC) #37
Robert Sesek
https://codereview.chromium.org/518313003/diff/100001/media/video/capture/mac/video_capture_device_avfoundation_mac.h File media/video/capture/mac/video_capture_device_avfoundation_mac.h (right): https://codereview.chromium.org/518313003/diff/100001/media/video/capture/mac/video_capture_device_avfoundation_mac.h#newcode13 media/video/capture/mac/video_capture_device_avfoundation_mac.h:13: #import "media/base/mac/avfoundation_glue.h" On 2014/09/04 18:31:08, jfroy wrote: > On ...
6 years, 3 months ago (2014-09-04 18:52:42 UTC) #38
jfroy
tommi: I've updated the header imports to sort just by path.
6 years, 3 months ago (2014-09-04 19:02:09 UTC) #39
tommi (sloooow) - chröme
On 2014/09/04 19:02:09, jfroy wrote: > tommi: I've updated the header imports to sort just ...
6 years, 3 months ago (2014-09-04 19:13:01 UTC) #40
DaleCurtis
rs lgtm
6 years, 3 months ago (2014-09-04 23:04:01 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/518313003/140001
6 years, 3 months ago (2014-09-04 23:40:10 UTC) #43
commit-bot: I haz the power
Committed patchset #8 (id:140001) as 1ea38f51996507472609ae05ce1130677b53c48d
6 years, 3 months ago (2014-09-05 03:01:53 UTC) #44
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 03:36:01 UTC) #45
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/f59abe0ce199aac0cd2eccf32ad7975155dbeded
Cr-Commit-Position: refs/heads/master@{#293413}

Powered by Google App Engine
This is Rietveld 408576698