Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
446163003:
Extend media::VideoFrame to wrap CVPixelBuffer on OS X and iOS. (Closed)
DescriptionExtend media::VideoFrame to wrap CVPixelBuffer on OS X and iOS.
This allows better integration of OS X and iOS media frameworks
with chromium code and makes buffer management less problematic,
especially when using CVPixelBufferPools provided by hardware
encoders or decoders.
BUG=401308
R=dalecurtis,hubbe
Committed: https://crrev.com/7eef1763b2118e82ed463e7414066e51690fbe53
Cr-Commit-Position: refs/heads/master@{#292023}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Conditionally support YV12, import CoreVide.h in header, edit comments. #
Total comments: 5
Patch Set 3 : Add space between system and user header include. #
Total comments: 2
Patch Set 4 : Use literal value of kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange instead of (wrong) availabilit… #
Total comments: 2
Patch Set 5 : Edit kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange comment as requested, fix lint issues. #Patch Set 6 : Despite the lint warning, CoreVideo.h needs to be included after project headers because only then … #Patch Set 7 : Rebase #
Total comments: 2
Patch Set 8 : Only import CVPixelBuffer.h to avoid build failures due to conflicting OpenGL function prototypes. #Messages
Total messages: 52 (0 generated)
I'm no OSX expert, so +rsesek. I'll stamp after his approval.
https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc#ne... media/base/video_frame.cc:295: // there are very few compatible CV pixel formats, so just check each nit: capitalization and punctuation https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc#ne... media/base/video_frame.cc:298: } else if (cv_format == kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) { This is only available on 10.7 and later. This will not compile. https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.h#new... media/base/video_frame.h:23: // From CoreVideo headers Why can't you just include CoreVideo.h here?
https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc#ne... media/base/video_frame.cc:295: // there are very few compatible CV pixel formats, so just check each On 2014/08/07 16:12:57, rsesek wrote: > nit: capitalization and punctuation Acknowledged. https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc#ne... media/base/video_frame.cc:298: } else if (cv_format == kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) { On 2014/08/07 16:12:57, rsesek wrote: > This is only available on 10.7 and later. This will not compile. Wow, are you telling me Chrome needs to build on 10.6? That is positively ancient... And that won't be tenable with the new codesigning requirements Apple is about to enforce. Anyways, I'll conditionally support this format when the builder is >= 10.7 and ensure it won't break if the symbol is not available (which should be automatic via the availability declaration on the symbol, assuming chrome builds with the right min-version compiler/linker flags). https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.h#new... media/base/video_frame.h:23: // From CoreVideo headers On 2014/08/07 16:12:57, rsesek wrote: > Why can't you just include CoreVideo.h here? I thought the general guideline in chromium code was to forward-declare types when possible to keep compile times low. Bringing in all of CoreVideo in this header seems overkill. I'm not overly fond of copying system type declarations either, but on the flip side I can hardly see them ever changing.
https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc#ne... media/base/video_frame.cc:298: } else if (cv_format == kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) { On 2014/08/07 16:22:14, jfroy wrote: > On 2014/08/07 16:12:57, rsesek wrote: > > This is only available on 10.7 and later. This will not compile. > > Wow, are you telling me Chrome needs to build on 10.6? That is positively > ancient... Yes. Build and run. This has been the requirement for at least two years. > And that won't be tenable with the new codesigning requirements Apple > is about to enforce. They claim that the signatures will be backwards compatible. > Anyways, I'll conditionally support this format when the builder is >= 10.7 and > ensure it won't break if the symbol is not available (which should be automatic > via the availability declaration on the symbol, assuming chrome builds with the > right min-version compiler/linker flags). You could also just use the literal value. https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.h#new... media/base/video_frame.h:23: // From CoreVideo headers On 2014/08/07 16:22:14, jfroy wrote: > On 2014/08/07 16:12:57, rsesek wrote: > > Why can't you just include CoreVideo.h here? > > I thought the general guideline in chromium code was to forward-declare types > when possible to keep compile times low. Bringing in all of CoreVideo in this > header seems overkill. > > I'm not overly fond of copying system type declarations either, but on the flip > side I can hardly see them ever changing. No, don't forward-declare system types unless you have to for forward declaration purposes. Pull in CoreVideo.h here.
https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc#ne... media/base/video_frame.cc:298: } else if (cv_format == kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) { On 2014/08/07 16:34:42, rsesek wrote: > On 2014/08/07 16:22:14, jfroy wrote: > > On 2014/08/07 16:12:57, rsesek wrote: > > > This is only available on 10.7 and later. This will not compile. > > > > Wow, are you telling me Chrome needs to build on 10.6? That is positively > > ancient... > > Yes. Build and run. This has been the requirement for at least two years. I'm brand new to Chrome development, I didn't know that. The runtime requirement is fine, but building on really old toolchains is harder. > > > And that won't be tenable with the new codesigning requirements Apple > > is about to enforce. > > They claim that the signatures will be backwards compatible. Yes, however the technote on the details of codesigning (https://developer.apple.com/library/prerelease/mac/technotes/tn2206/_index.html) specifically calls out that the codesigning process itself must happen on a >=10.9 host because the codesigning machinery is provided by system frameworks and not by the codesign binary itself. I suppose we could have dedicated codesigning bots. > > > Anyways, I'll conditionally support this format when the builder is >= 10.7 > and > > ensure it won't break if the symbol is not available (which should be > automatic > > via the availability declaration on the symbol, assuming chrome builds with > the > > right min-version compiler/linker flags). > > You could also just use the literal value. Yeah I remembered post-comment that they were just enums. I'd rather make the code conditional than hardcode a constant. https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.h#new... media/base/video_frame.h:23: // From CoreVideo headers On 2014/08/07 16:34:42, rsesek wrote: > On 2014/08/07 16:22:14, jfroy wrote: > > On 2014/08/07 16:12:57, rsesek wrote: > > > Why can't you just include CoreVideo.h here? > > > > I thought the general guideline in chromium code was to forward-declare types > > when possible to keep compile times low. Bringing in all of CoreVideo in this > > header seems overkill. > > > > I'm not overly fond of copying system type declarations either, but on the > flip > > side I can hardly see them ever changing. > > No, don't forward-declare system types unless you have to for forward > declaration purposes. Pull in CoreVideo.h here. Acknowledged.
Drive-by comment. You should probably loop in sandersd@ since he has been doing Mac hardware decode work for <video> and it didn't appear that he needed to add this field to VideoFrame.
On 2014/08/07 16:59:54, acolwell wrote: > Drive-by comment. You should probably loop in sandersd@ since he has been doing > Mac hardware decode work for <video> and it didn't appear that he needed to add > this field to VideoFrame. I'm aware of his work, but I haven't read his latest patchset (which IIRC adds actual frame decoding, albeit without support for reordering). I believe he uploads decoded frames to native textures for display. My eventual use case for this is encoding.
https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/1/media/base/video_frame.cc#ne... media/base/video_frame.cc:298: } else if (cv_format == kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) { On 2014/08/07 16:52:45, jfroy wrote: > On 2014/08/07 16:34:42, rsesek wrote: > > On 2014/08/07 16:22:14, jfroy wrote: > > > On 2014/08/07 16:12:57, rsesek wrote: > > > > This is only available on 10.7 and later. This will not compile. > > > > > > Wow, are you telling me Chrome needs to build on 10.6? That is positively > > > ancient... > > > > Yes. Build and run. This has been the requirement for at least two years. > > I'm brand new to Chrome development, I didn't know that. The runtime requirement > is fine, but building on really old toolchains is harder. Actually, it's quite trivial. You just need go/mac-10.6-sdk. > > > > > And that won't be tenable with the new codesigning requirements Apple > > > is about to enforce. > > > > They claim that the signatures will be backwards compatible. > > Yes, however the technote on the details of codesigning > (https://developer.apple.com/library/prerelease/mac/technotes/tn2206/_index.html) > specifically calls out that the codesigning process itself must happen on a > >=10.9 host because the codesigning machinery is provided by system frameworks > and not by the codesign binary itself. I suppose we could have dedicated > codesigning bots. We already have dedicated signing bots because of key material isolation. > > > > > Anyways, I'll conditionally support this format when the builder is >= 10.7 > > and > > > ensure it won't break if the symbol is not available (which should be > > automatic > > > via the availability declaration on the symbol, assuming chrome builds with > > the > > > right min-version compiler/linker flags). > > > > You could also just use the literal value. > > Yeah I remembered post-comment that they were just enums. I'd rather make the > code conditional than hardcode a constant. https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.c... media/base/video_frame.cc:298: (__IPHONE_OS_VERSION_MIN_REQUIRED >= 40000) Does //media even get compiled on iOS? If not, remove, or just use the literal '420v' without guards. https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.h... media/base/video_frame.h:21: #include <CoreVideo/CoreVideo.h> nit: blank line between system C headers and project headers.
https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.c... media/base/video_frame.cc:298: (__IPHONE_OS_VERSION_MIN_REQUIRED >= 40000) On 2014/08/07 18:00:15, rsesek wrote: > Does //media even get compiled on iOS? If not, remove, or just use the literal > '420v' without guards. It is for my purpose. I can still switch to using the literal if you prefer. I don't know which way is more "chrome"-y. https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.h... media/base/video_frame.h:21: #include <CoreVideo/CoreVideo.h> On 2014/08/07 18:00:15, rsesek wrote: > nit: blank line between system C headers and project headers. Acknowledged.
https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/20001/media/base/video_frame.c... media/base/video_frame.cc:298: (__IPHONE_OS_VERSION_MIN_REQUIRED >= 40000) On 2014/08/07 18:08:20, jfroy wrote: > On 2014/08/07 18:00:15, rsesek wrote: > > Does //media even get compiled on iOS? If not, remove, or just use the literal > > '420v' without guards. > > It is for my purpose. I can still switch to using the literal if you prefer. I > don't know which way is more "chrome"-y. If this code doesn't build on iOS, then remove the iOS guard. I think it's perfectly fine to use the literal, since its definition is part of the headers. Just leave a "TODO(jfroy): Switch to the constant kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange in the 10.7 SDK." comment.
On 2014/08/07 17:06:21, jfroy wrote: > On 2014/08/07 16:59:54, acolwell wrote: > > Drive-by comment. You should probably loop in sandersd@ since he has been > doing > > Mac hardware decode work for <video> and it didn't appear that he needed to > add > > this field to VideoFrame. > > I'm aware of his work, but I haven't read his latest patchset (which IIRC adds > actual frame decoding, albeit without support for reordering). I believe he > uploads decoded frames to native textures for display. My eventual use case for > this is encoding. My main concern is that you are introducing a platform specific struct inside a class that has primarily avoided containing platform specific structs. I realize the dma fd's is an exception at the moment, but we might want to rethink that as well. Why won't one of the existing WrapExternalXXX methods work for your use case? If this is just about pooling can't you do something like what the FFmpegVideoDecoder or VpxVideoDecoder do? Do you have another CL where this change is being used so we can get a sense of why you need this?
On 2014/08/07 18:24:17, acolwell wrote: > On 2014/08/07 17:06:21, jfroy wrote: > > On 2014/08/07 16:59:54, acolwell wrote: > > > Drive-by comment. You should probably loop in sandersd@ since he has been > > doing > > > Mac hardware decode work for <video> and it didn't appear that he needed to > > add > > > this field to VideoFrame. > > > > I'm aware of his work, but I haven't read his latest patchset (which IIRC adds > > actual frame decoding, albeit without support for reordering). I believe he > > uploads decoded frames to native textures for display. My eventual use case > for > > this is encoding. > > My main concern is that you are introducing a platform specific struct inside a > class that has primarily avoided containing platform specific structs. I realize > the dma fd's is an exception at the moment, but we might want to rethink that as > well. Why won't one of the existing WrapExternalXXX methods work for your use > case? If this is just about pooling can't you do something like what the > FFmpegVideoDecoder or VpxVideoDecoder do? > > Do you have another CL where this change is being used so we can get a sense of > why you need this? In general I had a lot of trouble dealing with refcounting for image buffers, so I welcome features to make lifetime management easier. I can't however use this specifically, though, because the VDA API requires passing via textures. That said, it doesn't make much sense to wrap an image buffer in a VideoFrame if the other features of VideoFrame are not also implemented; the result is that you have to both produce and consume the frames, and not let them leak out, in which case you could have just used your own type. I'd also like to see a CL where it is used.
On 2014/08/07 18:24:17, acolwell wrote: > On 2014/08/07 17:06:21, jfroy wrote: > > On 2014/08/07 16:59:54, acolwell wrote: > > > Drive-by comment. You should probably loop in sandersd@ since he has been > > doing > > > Mac hardware decode work for <video> and it didn't appear that he needed to > > add > > > this field to VideoFrame. > > > > I'm aware of his work, but I haven't read his latest patchset (which IIRC adds > > actual frame decoding, albeit without support for reordering). I believe he > > uploads decoded frames to native textures for display. My eventual use case > for > > this is encoding. > > My main concern is that you are introducing a platform specific struct inside a > class that has primarily avoided containing platform specific structs. I realize > the dma fd's is an exception at the moment, but we might want to rethink that as > well. Why won't one of the existing WrapExternalXXX methods work for your use > case? If this is just about pooling can't you do something like what the > FFmpegVideoDecoder or VpxVideoDecoder do? > > Do you have another CL where this change is being used so we can get a sense of > why you need this? The purpose is for iOS cast streaming. The CL below is the full encoder implementation (preliminary). https://codereview.chromium.org/450693006 The general issue is that I want to use encoder-provided CVPIxelBuffers (via an encoder-provided CVPixelBufferPool) and minimize memory copies. There doesn't seem to be a better way than allowing VideoFrame to wrap a pixel buffer. The encoder-provided pool issues buffers backed by encoder-accessible shared memory. Writing the unencoded frame data directly to these buffers is the most efficient data path. I could allocate my own memory and create a wrapping CVPixelBuffer for it inside the encoder, but that will internally incur a copy to the encoder-accessible memory, which is really damaging for performance on lower spec devices.
On 2014/08/07 18:51:28, jfroy wrote: > The purpose is for iOS cast streaming. The CL below is the full encoder > implementation > (preliminary). > > https://codereview.chromium.org/450693006 Why is that review private?
On 2014/08/07 18:51:28, jfroy wrote: > On 2014/08/07 18:24:17, acolwell wrote: > > On 2014/08/07 17:06:21, jfroy wrote: > > > On 2014/08/07 16:59:54, acolwell wrote: > > > > Drive-by comment. You should probably loop in sandersd@ since he has been > > > doing > > > > Mac hardware decode work for <video> and it didn't appear that he needed > to > > > add > > > > this field to VideoFrame. > > > > > > I'm aware of his work, but I haven't read his latest patchset (which IIRC > adds > > > actual frame decoding, albeit without support for reordering). I believe he > > > uploads decoded frames to native textures for display. My eventual use case > > for > > > this is encoding. > > > > My main concern is that you are introducing a platform specific struct inside > a > > class that has primarily avoided containing platform specific structs. I > realize > > the dma fd's is an exception at the moment, but we might want to rethink that > as > > well. Why won't one of the existing WrapExternalXXX methods work for your use > > case? If this is just about pooling can't you do something like what the > > FFmpegVideoDecoder or VpxVideoDecoder do? > > > > Do you have another CL where this change is being used so we can get a sense > of > > why you need this? > > The purpose is for iOS cast streaming. The CL below is the full encoder > implementation > (preliminary). > > https://codereview.chromium.org/450693006 > > The general issue is that I want to use encoder-provided CVPIxelBuffers (via an > encoder-provided CVPixelBufferPool) and minimize memory copies. There doesn't > seem to be a better way than allowing VideoFrame to wrap a pixel buffer. > > The encoder-provided pool issues buffers backed by encoder-accessible shared > memory. Writing the unencoded frame data directly to these buffers is the most > efficient data path. I could allocate my own memory and create a wrapping > CVPixelBuffer for it inside the encoder, but that will internally incur a copy > to the > encoder-accessible memory, which is really damaging for performance on lower > spec devices. I don't see any code in that CL that calls WrapCVPixelBuffer(). If you are just trying to wrap existing memory, you should be able to just use WrapExternalYuvData() and hold a reference to the original pixel buffer by base:Bind()ing it into the no_longer_needed_cb passed to that function.
On 2014/08/07 20:06:03, acolwell wrote: > On 2014/08/07 18:51:28, jfroy wrote: > > On 2014/08/07 18:24:17, acolwell wrote: > > > On 2014/08/07 17:06:21, jfroy wrote: > > > > On 2014/08/07 16:59:54, acolwell wrote: > > > > > Drive-by comment. You should probably loop in sandersd@ since he has > been > > > > doing > > > > > Mac hardware decode work for <video> and it didn't appear that he needed > > to > > > > add > > > > > this field to VideoFrame. > > > > > > > > I'm aware of his work, but I haven't read his latest patchset (which IIRC > > adds > > > > actual frame decoding, albeit without support for reordering). I believe > he > > > > uploads decoded frames to native textures for display. My eventual use > case > > > for > > > > this is encoding. > > > > > > My main concern is that you are introducing a platform specific struct > inside > > a > > > class that has primarily avoided containing platform specific structs. I > > realize > > > the dma fd's is an exception at the moment, but we might want to rethink > that > > as > > > well. Why won't one of the existing WrapExternalXXX methods work for your > use > > > case? If this is just about pooling can't you do something like what the > > > FFmpegVideoDecoder or VpxVideoDecoder do? > > > > > > Do you have another CL where this change is being used so we can get a sense > > of > > > why you need this? > > > > The purpose is for iOS cast streaming. The CL below is the full encoder > > implementation > > (preliminary). > > > > https://codereview.chromium.org/450693006 > > > > The general issue is that I want to use encoder-provided CVPIxelBuffers (via > an > > encoder-provided CVPixelBufferPool) and minimize memory copies. There doesn't > > seem to be a better way than allowing VideoFrame to wrap a pixel buffer. > > > > The encoder-provided pool issues buffers backed by encoder-accessible shared > > memory. Writing the unencoded frame data directly to these buffers is the most > > efficient data path. I could allocate my own memory and create a wrapping > > CVPixelBuffer for it inside the encoder, but that will internally incur a copy > > to the > > encoder-accessible memory, which is really damaging for performance on lower > > spec devices. > > I don't see any code in that CL that calls WrapCVPixelBuffer(). If you are just > trying to wrap existing memory, you should be able to just use > WrapExternalYuvData() and hold a reference to the original pixel buffer by > base:Bind()ing it into the no_longer_needed_cb passed to that function. That CL makes use of VideoFrames wrapping a CVPixelBuffer. The code creating these VideoFrames is not open source.
On 2014/08/07 20:09:28, jfroy wrote: > On 2014/08/07 20:06:03, acolwell wrote: > > On 2014/08/07 18:51:28, jfroy wrote: > > > On 2014/08/07 18:24:17, acolwell wrote: > > > > On 2014/08/07 17:06:21, jfroy wrote: > > > > > On 2014/08/07 16:59:54, acolwell wrote: > > > > > > Drive-by comment. You should probably loop in sandersd@ since he has > > been > > > > > doing > > > > > > Mac hardware decode work for <video> and it didn't appear that he > needed > > > to > > > > > add > > > > > > this field to VideoFrame. > > > > > > > > > > I'm aware of his work, but I haven't read his latest patchset (which > IIRC > > > adds > > > > > actual frame decoding, albeit without support for reordering). I believe > > he > > > > > uploads decoded frames to native textures for display. My eventual use > > case > > > > for > > > > > this is encoding. > > > > > > > > My main concern is that you are introducing a platform specific struct > > inside > > > a > > > > class that has primarily avoided containing platform specific structs. I > > > realize > > > > the dma fd's is an exception at the moment, but we might want to rethink > > that > > > as > > > > well. Why won't one of the existing WrapExternalXXX methods work for your > > use > > > > case? If this is just about pooling can't you do something like what the > > > > FFmpegVideoDecoder or VpxVideoDecoder do? > > > > > > > > Do you have another CL where this change is being used so we can get a > sense > > > of > > > > why you need this? > > > > > > The purpose is for iOS cast streaming. The CL below is the full encoder > > > implementation > > > (preliminary). > > > > > > https://codereview.chromium.org/450693006 > > > > > > The general issue is that I want to use encoder-provided CVPIxelBuffers (via > > an > > > encoder-provided CVPixelBufferPool) and minimize memory copies. There > doesn't > > > seem to be a better way than allowing VideoFrame to wrap a pixel buffer. > > > > > > The encoder-provided pool issues buffers backed by encoder-accessible shared > > > memory. Writing the unencoded frame data directly to these buffers is the > most > > > efficient data path. I could allocate my own memory and create a wrapping > > > CVPixelBuffer for it inside the encoder, but that will internally incur a > copy > > > to the > > > encoder-accessible memory, which is really damaging for performance on lower > > > spec devices. > > > > I don't see any code in that CL that calls WrapCVPixelBuffer(). If you are > just > > trying to wrap existing memory, you should be able to just use > > WrapExternalYuvData() and hold a reference to the original pixel buffer by > > base:Bind()ing it into the no_longer_needed_cb passed to that function. > > That CL makes use of VideoFrames wrapping a CVPixelBuffer. The code creating > these VideoFrames is not open source. Since you already have code to wrap non-CVPixelBuffer sourced data and it doesn't look too complicated or cause a memcpy(). I think you should just use that code path for all VideoFrames and simply use WrapExternalYuvData() to avoid the copy. I can't really comment more w/o seeing the VideoFrame creation code.
On 2014/08/07 20:15:47, acolwell wrote: > On 2014/08/07 20:09:28, jfroy wrote: > > On 2014/08/07 20:06:03, acolwell wrote: > > > On 2014/08/07 18:51:28, jfroy wrote: > > > > On 2014/08/07 18:24:17, acolwell wrote: > > > > > On 2014/08/07 17:06:21, jfroy wrote: > > > > > > On 2014/08/07 16:59:54, acolwell wrote: > > > > > > > Drive-by comment. You should probably loop in sandersd@ since he has > > > been > > > > > > doing > > > > > > > Mac hardware decode work for <video> and it didn't appear that he > > needed > > > > to > > > > > > add > > > > > > > this field to VideoFrame. > > > > > > > > > > > > I'm aware of his work, but I haven't read his latest patchset (which > > IIRC > > > > adds > > > > > > actual frame decoding, albeit without support for reordering). I > believe > > > he > > > > > > uploads decoded frames to native textures for display. My eventual use > > > case > > > > > for > > > > > > this is encoding. > > > > > > > > > > My main concern is that you are introducing a platform specific struct > > > inside > > > > a > > > > > class that has primarily avoided containing platform specific structs. I > > > > realize > > > > > the dma fd's is an exception at the moment, but we might want to rethink > > > that > > > > as > > > > > well. Why won't one of the existing WrapExternalXXX methods work for > your > > > use > > > > > case? If this is just about pooling can't you do something like what the > > > > > FFmpegVideoDecoder or VpxVideoDecoder do? > > > > > > > > > > Do you have another CL where this change is being used so we can get a > > sense > > > > of > > > > > why you need this? > > > > > > > > The purpose is for iOS cast streaming. The CL below is the full encoder > > > > implementation > > > > (preliminary). > > > > > > > > https://codereview.chromium.org/450693006 > > > > > > > > The general issue is that I want to use encoder-provided CVPIxelBuffers > (via > > > an > > > > encoder-provided CVPixelBufferPool) and minimize memory copies. There > > doesn't > > > > seem to be a better way than allowing VideoFrame to wrap a pixel buffer. > > > > > > > > The encoder-provided pool issues buffers backed by encoder-accessible > shared > > > > memory. Writing the unencoded frame data directly to these buffers is the > > most > > > > efficient data path. I could allocate my own memory and create a wrapping > > > > CVPixelBuffer for it inside the encoder, but that will internally incur a > > copy > > > > to the > > > > encoder-accessible memory, which is really damaging for performance on > lower > > > > spec devices. > > > > > > I don't see any code in that CL that calls WrapCVPixelBuffer(). If you are > > just > > > trying to wrap existing memory, you should be able to just use > > > WrapExternalYuvData() and hold a reference to the original pixel buffer by > > > base:Bind()ing it into the no_longer_needed_cb passed to that function. > > > > That CL makes use of VideoFrames wrapping a CVPixelBuffer. The code creating > > these VideoFrames is not open source. > > Since you already have code to wrap non-CVPixelBuffer sourced data and it > doesn't look too complicated or cause a memcpy(). I think you should just use > that code path for all VideoFrames and simply use WrapExternalYuvData() to avoid > the copy. I can't really comment more w/o seeing the VideoFrame creation code. As I wrote in a previous comment, a memory copy will occur inside the video encoder automatically if it detects that a submitted pixel buffer is not backed by its internal pixel buffer pool (when using a hardware encoder).
On 2014/08/07 20:18:46, jfroy wrote: > On 2014/08/07 20:15:47, acolwell wrote: > > On 2014/08/07 20:09:28, jfroy wrote: > > > On 2014/08/07 20:06:03, acolwell wrote: > > > > On 2014/08/07 18:51:28, jfroy wrote: > > > > > On 2014/08/07 18:24:17, acolwell wrote: > > > > > > On 2014/08/07 17:06:21, jfroy wrote: > > > > > > > On 2014/08/07 16:59:54, acolwell wrote: > > > > > > > > Drive-by comment. You should probably loop in sandersd@ since he > has > > > > been > > > > > > > doing > > > > > > > > Mac hardware decode work for <video> and it didn't appear that he > > > needed > > > > > to > > > > > > > add > > > > > > > > this field to VideoFrame. > > > > > > > > > > > > > > I'm aware of his work, but I haven't read his latest patchset (which > > > IIRC > > > > > adds > > > > > > > actual frame decoding, albeit without support for reordering). I > > believe > > > > he > > > > > > > uploads decoded frames to native textures for display. My eventual > use > > > > case > > > > > > for > > > > > > > this is encoding. > > > > > > > > > > > > My main concern is that you are introducing a platform specific struct > > > > inside > > > > > a > > > > > > class that has primarily avoided containing platform specific structs. > I > > > > > realize > > > > > > the dma fd's is an exception at the moment, but we might want to > rethink > > > > that > > > > > as > > > > > > well. Why won't one of the existing WrapExternalXXX methods work for > > your > > > > use > > > > > > case? If this is just about pooling can't you do something like what > the > > > > > > FFmpegVideoDecoder or VpxVideoDecoder do? > > > > > > > > > > > > Do you have another CL where this change is being used so we can get a > > > sense > > > > > of > > > > > > why you need this? > > > > > > > > > > The purpose is for iOS cast streaming. The CL below is the full encoder > > > > > implementation > > > > > (preliminary). > > > > > > > > > > https://codereview.chromium.org/450693006 > > > > > > > > > > The general issue is that I want to use encoder-provided CVPIxelBuffers > > (via > > > > an > > > > > encoder-provided CVPixelBufferPool) and minimize memory copies. There > > > doesn't > > > > > seem to be a better way than allowing VideoFrame to wrap a pixel buffer. > > > > > > > > > > The encoder-provided pool issues buffers backed by encoder-accessible > > shared > > > > > memory. Writing the unencoded frame data directly to these buffers is > the > > > most > > > > > efficient data path. I could allocate my own memory and create a > wrapping > > > > > CVPixelBuffer for it inside the encoder, but that will internally incur > a > > > copy > > > > > to the > > > > > encoder-accessible memory, which is really damaging for performance on > > lower > > > > > spec devices. > > > > > > > > I don't see any code in that CL that calls WrapCVPixelBuffer(). If you are > > > just > > > > trying to wrap existing memory, you should be able to just use > > > > WrapExternalYuvData() and hold a reference to the original pixel buffer by > > > > base:Bind()ing it into the no_longer_needed_cb passed to that function. > > > > > > That CL makes use of VideoFrames wrapping a CVPixelBuffer. The code creating > > > these VideoFrames is not open source. > > > > Since you already have code to wrap non-CVPixelBuffer sourced data and it > > doesn't look too complicated or cause a memcpy(). I think you should just use > > that code path for all VideoFrames and simply use WrapExternalYuvData() to > avoid > > the copy. I can't really comment more w/o seeing the VideoFrame creation code. > > As I wrote in a previous comment, a memory copy will occur inside the video > encoder > automatically if it detects that a submitted pixel buffer is not backed by its > internal > pixel buffer pool (when using a hardware encoder). I should clarify a bit; by video encoder, I meant VideoToolbox.
https://codereview.chromium.org/446163003/diff/40001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/40001/media/base/video_frame.c... media/base/video_frame.cc:297: #if (__MAC_OS_X_VERSION_MIN_REQUIRED >= 1070) || \ This is testing if the minimum runtime requirement is 10.7 or iOS 4, not whether the SDK is 10.7 or iOS 4. For that, you use MAC_OS_X_VERSION_MAX_ALLOWED. The best way to handle this would be to define the constant in this file for the appropriate SDK: https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/sdk_forwa...
https://codereview.chromium.org/446163003/diff/40001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/40001/media/base/video_frame.c... media/base/video_frame.cc:297: #if (__MAC_OS_X_VERSION_MIN_REQUIRED >= 1070) || \ On 2014/08/08 14:22:30, rsesek wrote: > This is testing if the minimum runtime requirement is 10.7 or iOS 4, not whether > the SDK is 10.7 or iOS 4. For that, you use MAC_OS_X_VERSION_MAX_ALLOWED. > > The best way to handle this would be to define the constant in this file for the > appropriate SDK: > https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/sdk_forwa... Ooops, that's rather embarrassing. That looks like the right file indeed.
On 2014/08/08 21:52:49, jfroy wrote: > https://codereview.chromium.org/446163003/diff/40001/media/base/video_frame.cc > File media/base/video_frame.cc (right): > > https://codereview.chromium.org/446163003/diff/40001/media/base/video_frame.c... > media/base/video_frame.cc:297: #if (__MAC_OS_X_VERSION_MIN_REQUIRED >= 1070) || > \ > On 2014/08/08 14:22:30, rsesek wrote: > > This is testing if the minimum runtime requirement is 10.7 or iOS 4, not > whether > > the SDK is 10.7 or iOS 4. For that, you use MAC_OS_X_VERSION_MAX_ALLOWED. > > > > The best way to handle this would be to define the constant in this file for > the > > appropriate SDK: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/sdk_forwa... > > Ooops, that's rather embarrassing. That looks like the right file indeed. Actually, since I also want to support iOS, I'll go with just using the value inline.
https://codereview.chromium.org/446163003/diff/50001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/50001/media/base/video_frame.c... media/base/video_frame.cc:297: // NOTE: kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange TODO(jfroy): Use kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange in the 10.7 SDK.
https://codereview.chromium.org/446163003/diff/50001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/50001/media/base/video_frame.c... media/base/video_frame.cc:297: // NOTE: kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange On 2014/08/12 00:09:40, rsesek wrote: > TODO(jfroy): Use kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange in the 10.7 > SDK. Acknowledged.
LGTM Also, small code reviewing note: generally people use "Done" when they've addressed a code review comment and use "Acknowledged" for unactionable comments/general feedback.
On 2014/08/12 01:07:49, rsesek wrote: > LGTM > > Also, small code reviewing note: generally people use "Done" when they've > addressed a code review comment and use "Acknowledged" for unactionable > comments/general feedback. Thank you, noted.
The CQ bit was checked by jfroy@chromium.org
The CQ bit was unchecked by jfroy@chromium.org
On 2014/08/07 00:54:37, DaleCurtis wrote: > I'm no OSX expert, so +rsesek. I'll stamp after his approval. Waiting for DaleCurtis's stamp before adding to the try queue.
Sorry didn't realize you were waiting for my stamp. Since miu@ is reviewing the other half of this change still, I'm going to defer to him before stamping.
lgtm % this one clean-up: https://codereview.chromium.org/446163003/diff/110001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/110001/media/base/video_frame.... media/base/video_frame.cc:297: // TODO(jfroy): Use kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange when the I've seen others solve this problem by using #ifdefs to define constants at the top of the file based on the SDK version. Meaning, when the SDK is too old, we define the constants ourselves using those found in the newer SDK. Example: https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/sdk_forwa...
https://codereview.chromium.org/446163003/diff/110001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/446163003/diff/110001/media/base/video_frame.... media/base/video_frame.cc:297: // TODO(jfroy): Use kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange when the On 2014/08/25 18:39:35, miu wrote: > I've seen others solve this problem by using #ifdefs to define constants at the > top of the file based on the SDK version. Meaning, when the SDK is too old, we > define the constants ourselves using those found in the newer SDK. Example: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/sdk_forwa... There was already back-and-forth about this on the review. This is fine.
BTW--This change isn't meant to be used by your other change (https://codereview.chromium.org/450693006/), right? If not, why are we adding it?
On 2014/08/25 18:48:39, miu wrote: > BTW--This change isn't meant to be used by your other change > (https://codereview.chromium.org/450693006/), right? If not, why are we adding > it? Yes it is. The VideoToolbox encoder patch depends on this patch (as an optimization, but the code won't build without it).
On 2014/08/25 19:28:19, jfroy wrote: > On 2014/08/25 18:48:39, miu wrote: > > BTW--This change isn't meant to be used by your other change > > (https://codereview.chromium.org/450693006/), right? If not, why are we > adding > > it? > > Yes it is. The VideoToolbox encoder patch depends on this patch (as an > optimization, but the code won't build without it). Okay. Well, let's get the ownership of the CVPixelBuffer sorted out (see comments in other change) before moving forward with this one.
On 2014/08/25 20:08:32, miu wrote: > On 2014/08/25 19:28:19, jfroy wrote: > > On 2014/08/25 18:48:39, miu wrote: > > > BTW--This change isn't meant to be used by your other change > > > (https://codereview.chromium.org/450693006/), right? If not, why are we > > adding > > > it? > > > > Yes it is. The VideoToolbox encoder patch depends on this patch (as an > > optimization, but the code won't build without it). > > Okay. Well, let's get the ownership of the CVPixelBuffer sorted out (see > comments in other change) before moving forward with this one. I've commented on that in the other CL. Let's keep the conversation about that there.
On 2014/08/25 21:32:54, jfroy wrote: > On 2014/08/25 20:08:32, miu wrote: > > On 2014/08/25 19:28:19, jfroy wrote: > > > On 2014/08/25 18:48:39, miu wrote: > > > > BTW--This change isn't meant to be used by your other change > > > > (https://codereview.chromium.org/450693006/), right? If not, why are we > > > adding > > > > it? > > > > > > Yes it is. The VideoToolbox encoder patch depends on this patch (as an > > > optimization, but the code won't build without it). > > > > Okay. Well, let's get the ownership of the CVPixelBuffer sorted out (see > > comments in other change) before moving forward with this one. > > I've commented on that in the other CL. Let's keep the conversation about that > there. At this point, this change does not lgtm anymore. The other change has CVPixelBuffer wrapping VideoFrame because the Mac platform wants to own the buffer. That's perfectly fine, but it also implies that having VideoFrame wrapping CVPixelBuffer (i.e., this change) is the wrong design choice.
On 2014/08/25 21:50:08, miu wrote: > On 2014/08/25 21:32:54, jfroy wrote: > > On 2014/08/25 20:08:32, miu wrote: > > > On 2014/08/25 19:28:19, jfroy wrote: > > > > On 2014/08/25 18:48:39, miu wrote: > > > > > BTW--This change isn't meant to be used by your other change > > > > > (https://codereview.chromium.org/450693006/), right? If not, why are we > > > > adding > > > > > it? > > > > > > > > Yes it is. The VideoToolbox encoder patch depends on this patch (as an > > > > optimization, but the code won't build without it). > > > > > > Okay. Well, let's get the ownership of the CVPixelBuffer sorted out (see > > > comments in other change) before moving forward with this one. > > > > I've commented on that in the other CL. Let's keep the conversation about that > > there. > > At this point, this change does not lgtm anymore. The other change has > CVPixelBuffer wrapping VideoFrame because the Mac platform wants to own the > buffer. That's perfectly fine, but it also implies that having VideoFrame > wrapping CVPixelBuffer (i.e., this change) is the wrong design choice. This is a highly relevant performance optimization which I've explained in detail both in this review and in a design document you should have access to.
On 2014/08/25 21:52:12, jfroy wrote: > This is a highly relevant performance optimization which I've explained in > detail both in this review and in a design document you should have access to. lgtm. I missed the design doc link, but found it and I understand now. ;-) Thanks for being patient with me. So, in the ideal end-result of all these changes: VideoFrames that wrap CVPixelBuffers will be provided to media::cast::CastSender (and passed directly to your hardware H264 encoder impl in the other change). The code there will either use the CVPixelBuffer already wrapped by the VideoFrame, or it will create its own CVPixelBuffer that wraps the VideoFrame as an "adapter" for the platform library.
Yep! On Mon, Aug 25, 2014 at 18:15 <miu@chromium.org> wrote: > On 2014/08/25 21:52:12, jfroy wrote: > > This is a highly relevant performance optimization which I've explained > in > > detail both in this review and in a design document you should have > > access to. > > lgtm. I missed the design doc link, but found it and I understand now. > ;-) > Thanks for being patient with me. > > So, in the ideal end-result of all these changes: VideoFrames that wrap > CVPixelBuffers will be provided to media::cast::CastSender (and passed > directly > to your hardware H264 encoder impl in the other change). The code there > will > either use the CVPixelBuffer already wrapped by the VideoFrame, or it will > create its own CVPixelBuffer that wraps the VideoFrame as an "adapter" for > the > platform library. > > https://codereview.chromium.org/446163003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've looked through both changes, lgtm
The CQ bit was checked by jfroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/446163003/110001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/46049) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2014/08/26 00:41:29, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_chromium_compile_dbg on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_rel_swarming on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) Lame errors caused by mismatched OpenGL function prototypes :/ FAILED: /Volumes/data/b/build/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/cc/layers/cc.video_frame_provider_client_impl.o.d -DV8_DEPRECATION_WARNINGS -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=214024 -DCOMPONENT_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY -DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDCHECK_ALWAYS_ON=1 -DENABLE_EGLIMAGE=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DCLD2_DATA_SOURCE=static -DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_MANAGED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_WIFI_BOOTSTRAPPING=1 -DCC_IMPLEMENTATION=1 -DSKIA_DLL -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT -DSK_SUPPORT_LEGACY_PICTURE_CLONE -DSK_SUPPORT_LEGACY_GETDEVICE -DSK_IGNORE_ETC1_SUPPORT -DSK_IGNORE_GPU_DITHER -DSK_USE_POSIX_THREADS -DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -Igen -I../.. -I../../third_party/khronos -I../../gpu -I../../skia/config -I../../third_party/skia/src/core -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../skia/ext -I../../third_party/skia/include/utils/mac -I../../third_party/icu/source/i18n -I../../third_party/icu/source/common -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk -O0 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.6 -arch i386 -Wall -Wendif-labels -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wno-selector-type-mismatch -Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration -Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing -Wno-deprecated-register -std=gnu++11 -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -fno-slp-vectorize -Xclang -load -Xclang /Volumes/data/b/build/slave/mac/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib -Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics -fno-strict-aliasing -fstack-protector-all -Wno-undefined-bool-conversion -Wno-tautological-undefined-compare -c ../../cc/layers/video_frame_provider_client_impl.cc -o obj/cc/layers/cc.video_frame_provider_client_impl.o In file included from ../../cc/layers/video_frame_provider_client_impl.cc:10: In file included from ../../media/base/video_frame.h:19: In file included from /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CoreVideo.h:24: In file included from /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CVDisplayLink.h:27: In file included from /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/OpenGL.framework/Headers/OpenGL.h:10: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/OpenGL.framework/Headers/gl.h:2765:13: error: conflicting types for 'GLES2TexImage2D' extern void glTexImage2D (GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels); ^ ../../gpu/GLES2/gl2chromium_autogen.h:124:22: note: expanded from macro 'glTexImage2D' #define glTexImage2D GLES2_GET_FUN(TexImage2D) ^ ../../gpu/GLES2/gl2chromium.h:22:29: note: expanded from macro 'GLES2_GET_FUN' #define GLES2_GET_FUN(name) GLES2 ## name ^ <scratch space>:136:1: note: expanded from here GLES2TexImage2D ^ ../../third_party/khronos/GLES2/gl2.h:483:29: note: previous declaration is here GL_APICALL void GL_APIENTRY glTexImage2D (GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const void *pixels); ^ ../../gpu/GLES2/gl2chromium_autogen.h:124:22: note: expanded from macro 'glTexImage2D' #define glTexImage2D GLES2_GET_FUN(TexImage2D) ^ ../../gpu/GLES2/gl2chromium.h:22:29: note: expanded from macro 'GLES2_GET_FUN' #define GLES2_GET_FUN(name) GLES2 ## name ^ <scratch space>:42:1: note: expanded from here GLES2TexImage2D ^ In file included from ../../cc/layers/video_frame_provider_client_impl.cc:10: In file included from ../../media/base/video_frame.h:19: In file included from /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CoreVideo.h:24: In file included from /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/CoreVideo.framework/Headers/CVDisplayLink.h:27: In file included from /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/OpenGL.framework/Headers/OpenGL.h:10: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/OpenGL.framework/Headers/gl.h:2973:13: error: conflicting types for 'GLES2ShaderSource' extern void glShaderSource (GLuint shader, GLsizei count, const GLchar* *string, const GLint *length); ^ ../../gpu/GLES2/gl2chromium_autogen.h:115:24: note: expanded from macro 'glShaderSource' #define glShaderSource GLES2_GET_FUN(ShaderSource) ^ ../../gpu/GLES2/gl2chromium.h:22:29: note: expanded from macro 'GLES2_GET_FUN' #define GLES2_GET_FUN(name) GLES2 ## name ^ <scratch space>:136:1: note: expanded from here GLES2ShaderSource ^ ../../third_party/khronos/GLES2/gl2.h:476:29: note: previous declaration is here GL_APICALL void GL_APIENTRY glShaderSource (GLuint shader, GLsizei count, const GLchar *const*string, const GLint *length); ^ ../../gpu/GLES2/gl2chromium_autogen.h:115:24: note: expanded from macro 'glShaderSource' #define glShaderSource GLES2_GET_FUN(ShaderSource) ^ ../../gpu/GLES2/gl2chromium.h:22:29: note: expanded from macro 'GLES2_GET_FUN' #define GLES2_GET_FUN(name) GLES2 ## name ^ <scratch space>:35:1: note: expanded from here GLES2ShaderSource ^ 2 errors generated.
The CQ bit was checked by jfroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jfroy@chromium.org/446163003/130001
Message was sent while issue was closed.
Committed patchset #8 (130001) as 9f0bfba2ccbdb3850d96e757eec8619f6893f597
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7eef1763b2118e82ed463e7414066e51690fbe53 Cr-Commit-Position: refs/heads/master@{#292023} |
