Re: H264 HW encode using VideoToolbox (issue 1636083003 by emircan@chromium.org)

[emi...@chromium.org]

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode151
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:151:
scoped_ptr<InProgressFrameEncode> request(new InProgressFrameEncode(
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> Nit: please move above l.163.

Done.

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode238
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:238:
set_data_rate_limit_ &= session_property_setter.SetSessionProperty(
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> I believe this would have the same effect?
>
> if (s_d_r_l_) {
> s_d_r_l = SSP();
> }

Done.

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode274
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:274:
encoder->client_task_runner_->PostTask(
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> Do we need to do anything in this method at all? Could we just post to
the task
> and do error checks there?

Done.

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode288
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:288:
base::Unretained(encoder), info, sbuf));
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> Please add a comment why it's safe to do Unretained.

Actually, Unretained looks wrong to me now. If this function is called
during the destruction sequence, |enocder| might be freed by the time
task is posted. I will change it to WeakPtr and invalidate WeakPtr in
the beginning of destruction sequence.

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode296
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:296: // If
there isn't any BitstreamBuffer to copy into, add it to a queue for
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> Could we perhaps have a TryReturnBitstreamBuffer() which would do this
also,
> instead of duplicating this code here and in
UseOutputBitstreamBuffer() please?

Here we are checking if bitstream_buffer_queue_ is empty. Inside
UseOutputBitstreamBuffer() , we check if encoder_output_queue_ is empty.
The common code is already in ReturnBitstreamBuffer() and I don't know
if any further refactor can be done.

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode348
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:348: // Keep
these in-sync with those in ConfigureCompressionSession().
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> These keys below seem different from the ones in CCS()...? What does
it mean to
> keep them in sync?

Thanks for the notice, I am removing the comment since it no longer
applies.

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode370
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:370: // Try
creating session again without forcing HW encode.
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> Are we interested in doing SW encode here at all? It would be
preferable to do
> it outside of GPU process then...

Eventually, we want to compare the performance of Videotoolbox SW to our
OpenH264 SW when they both land completely. Then, we can modify this
logic. WDYT?

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode415
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:415:
reinterpret_cast<void*>(this),
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> Can we in any way guarantee CompressionCallback caller doesn't outlive
this?

I followed the logic used by cast that you can find below. I will copy
their comments as well to make it clear.
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/h264_vt_encoder.cc&l=434

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.h
File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right):

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode18
content/common/gpu/media/vt_video_encode_accelerator_mac.h:18: // safe,
so this object is pinned to the thread on which it is constructed.
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> We should always avoid doing things on GPU Child thread unless we have
no other
> choice (e.g. need GL context), because this is a performance-critical
thread.

Ok, I am adding an encoder thread. I was trying to make this patch a
starting point behind flag, but it is already big enough at this point
:) Adding the third thread that this class can be on would hurt.

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode82
content/common/gpu/media/vt_video_encode_accelerator_mac.h:82: //
Destroy the current compression session if any. Blocks until all pending
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> This sounds especially concerning if blocking GPU Child.

I will post this on the |encoder_thread| during dtor.

https://codereview.chromium.org/1636083003/diff/260001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode95
content/common/gpu/media/vt_video_encode_accelerator_mac.h:95: bool
set_data_rate_limit_;
On 2016/02/18 11:16:14, Pawel Osciak wrote:
> Please document this field.

Done.

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

PTAL, I made some updates. Here is a Design Doc that explains the limitations
and has the performance comparison with the SW encoder implementation.
https://docs.google.com/document/d/1oUTyZdNh8QstKRds-8wHEF_hqKryMiUpEOW8M57sUGU/edit?usp=sharing



https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/media/video_encode_accelerator_unittest.cc
File content/common/gpu/media/video_encode_accelerator_unittest.cc
(right):

https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/media/video_encode_accelerator_unittest.cc#newcode657
content/common/gpu/media/video_encode_accelerator_unittest.cc:657:
decoder_(new media::FFmpegVideoDecoder()),
On 2016/02/18 11:16:13, Pawel Osciak wrote:
> On 2016/02/08 23:41:23, emircan wrote:
> > On 2016/02/08 04:33:42, Pawel Osciak wrote:
> > > Did you mean ffmpeg was just not compiled/working on Mac at all,
or that it
> > > would fail for some other reason? I thought ffmpeg worked on
Mac...
> >
> > It points to this line when built:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu/media/video_encode_accelerator_unittest.cc&l=696.
> > Do you know if it is still the case?
>
> Are you building and testing on Chromium, instead of Chrome?

I built with ffmpeg branding and it works fine now. I will turn on the
unit test to verify frames now.

https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/media/vt_video_encode_accelerator.cc
File content/common/gpu/media/vt_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1636083003/diff/140001/content/common/gpu/media/vt_video_encode_accelerator.cc#newcode308
content/common/gpu/media/vt_video_encode_accelerator.cc:308:
videotoolbox_glue_->kVTProfileLevel_H264_Baseline_AutoLevel());

On 2016/02/18 11:16:14, Pawel Osciak wrote:

> On 2016/02/08 23:41:24, emircan wrote:
> > On 2016/02/08 04:33:42, Pawel Osciak wrote:
> > > Is baseline preferred by us? Normally constrained baseline is more
> compatible
> > > with various devices (unless this actually is constrained). Main
may also be
> > > better in general.
> >
> > I discussed this with WebRTC people. I learned that Baseline is the
only one
> > that openh264 supports -SW case- and compatible with Firefox as
well. I am
> > planning to support only Baseline in this CL. I can add other
profiles after
> > further discussion, and it wouldn't be hard.
>
> Are we sure that was Baseline, and not Constrained Baseline though? A
good
> number of HW decoders doesn't support Baseline, as it requires
additional
> features.

As far as I got answers from WebRTC folks, it is Baseline[0]. I can
revisit this to add new formats if needed.

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc&l=387

https://codereview.chromium.org/1636083003/diff/340001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/340001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode126
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:126:
bitstream_buffer_size_ = input_visible_size.GetArea();
Calculating this based on the initial bitrate isn't a good idea.
Eventually, when the bitrate increases, there is some keyframe which are
much bigger than expected. Setting it as frame size like VAAPI[0] is
safer.

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu/media/vaapi_video_encode_accelerator.cc&l=200

https://codereview.chromium.org/1636083003/diff/340001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode216
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:216:
target_bitrate_ / kBitsPerByte, 1.0f));
I figured out that the error I got here was due to setting 2 integers.
Encoder expects second param to be float, and it seems to work fine.

https://codereview.chromium.org/1636083003/

[jf...@chromium.org]

Other than my small comments, looks good.


https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode43
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:43: const
CMSampleBufferRef sample_buffer;
Would it be safer to use a smart pointer to manage the ref count?

https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode339
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:339:
encoder->weak_this_factory_.GetWeakPtr(), status, info, sbuf));
If this turns out to be weak, sbuf gets leaked. I'm not sure how to
cleanly work around this. Perhaps a smart pointer that can be managed by
base::Bind or base::Callback?

https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode344
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:344:
CMSampleBufferRef sbuf) {
Document that sbuf is retained and needs to be released by this
function, or use smart pointer.

https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode369
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:369:
CMSampleBufferRef sbuf,
Document that sbuf is retained and needs to be released by this
function, or use smart pointer.

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode43
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:43: const
CMSampleBufferRef sample_buffer;

On 2016/03/07 21:57:07, jfroy wrote:
> Would it be safer to use a smart pointer to manage the ref count?

Done.

https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode339
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:339:
encoder->weak_this_factory_.GetWeakPtr(), status, info, sbuf));

On 2016/03/07 21:57:07, jfroy wrote:
> If this turns out to be weak, sbuf gets leaked. I'm not sure how to
cleanly work
> around this. Perhaps a smart pointer that can be managed by base::Bind
or
> base::Callback?

Good catch. I will defer the lifetime of this object to struct
EncodeOutput.

https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode344
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:344:
CMSampleBufferRef sbuf) {

On 2016/03/07 21:57:07, jfroy wrote:
> Document that sbuf is retained and needs to be released by this
function, or use
> smart pointer.

I defer the lifetime of this object to struct EncodeOutput.

https://codereview.chromium.org/1636083003/diff/360001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode369
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:369:
CMSampleBufferRef sbuf,

On 2016/03/07 21:57:07, jfroy wrote:
> Document that sbuf is retained and needs to be released by this
function, or use
> smart pointer.

sbuf will go out of scope when struct EncodeOutput goes out of scope.

https://codereview.chromium.org/1636083003/

[jf...@chromium.org]

lgtm



https://codereview.chromium.org/1636083003/

[jf...@chromium.org]

lgtm



https://codereview.chromium.org/1636083003/

[m...@chromium.org]

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode225
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:225:
client_ptr_factory_.reset();
This is an illegal operation. The WeakPtrs being dereferenced on the
"client task" thread must always be invalidated on that same thread as
well. Otherwise, there could be code executing concurrently that has
already null-checked the WeakPtr but still assumes the object is alive.

Hmm...This is a really complex threading model here. IMO, the simplest
solution is not to access |client_| via a WeakPtr. Instead, store it as
a raw pointer (in Initialize()) and null it out (in Destroy(). Then,
for all read-write access and calls into the client's methods, always be
holding a base::Lock. And, always null-check |client_|, of course,
since tasks can run after Destroy() is called.

https://codereview.chromium.org/1636083003/

[m...@chromium.org]

Mostly small nits/tweaks comments:

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode73
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:73: }
For sanity, after the Destroy() call here, consider adding:

DCHECK(!encoder_thread_.IsRunning());
DCHECK(!weak_this_factory_.HasWeakPtrs());

(to be sure the encoder thread and all outstanding tasks have been
stopped/canceled)

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode225
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:225:
client_ptr_factory_.reset();

On 2016/03/08 21:27:41, miu wrote:
> This is an illegal operation. The WeakPtrs being dereferenced on the
"client
> task" thread must always be invalidated on that same thread as well.
Otherwise,
> there could be code executing concurrently that has already
null-checked the
> WeakPtr but still assumes the object is alive.
>
> Hmm...This is a really complex threading model here. IMO, the
simplest solution
> is not to access |client_| via a WeakPtr. Instead, store it as a raw
pointer
> (in Initialize()) and null it out (in Destroy(). Then, for all
read-write
> access and calls into the client's methods, always be holding a
base::Lock.
> And, always null-check |client_|, of course, since tasks can run after
Destroy()
> is called.

Never mind. My misunderstanding. We discussed this off-line (it's the
same thread).

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode296
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:296: //
Cancel all callbacks.
nit: Cancel all encoder thread callbacks.

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode297
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:297:
weak_this_factory_.InvalidateWeakPtrs();
naming nit: Instead of weak_this_factory_, how about
encoder_task_weak_factory_? That would make it clear to the reader that
these WeakPtrs are being used only for encoder tasks on the encode
thread.

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.h
File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right):

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode118
content/common/gpu/media/vt_video_encode_accelerator_mac.h:118:
scoped_ptr<base::WeakPtrFactory<Client> > client_ptr_factory_;
nit: It's not necessary to use scoped_ptr<> here. Meaning, in
Destroy(), you could just call:

client_ptr_factory_.InvalidateWeakPtrs();

https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videotoolbox_helpers.cc
File media/base/mac/videotoolbox_helpers.cc (right):

https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videotoolbox_helpers.cc#newcode46
media/base/mac/videotoolbox_helpers.cc:46: std::vector<CFNumberRef>
numbers;
This is a good place to use std::array:

std::array<CFNumberRef, 2> numbers = {
CFNumberCreate(nullptr, kCFNumberSInt32Type, &int_val),
CFNumberCreate(nullptr, kCFNumberFloat32Type, &float_val)
};
base::ScopedCFTypeRef<CFArrayRef> array(CFArrayCreate(
kCFAllocatorDefault, reinterpret_cast<const void**>(numbers.data()),
numbers.size(), &kCFTypeArrayCallBacks));
for (auto& number : numbers)
CFRelease(number);

https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videotoolbox_helpers.h
File media/base/mac/videotoolbox_helpers.h (right):

https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videotoolbox_helpers.h#newcode55
media/base/mac/videotoolbox_helpers.h:55: bool
SetSessionProperty(CFStringRef key, int32_t value);
nit suggestion: When I was reading the code calling these, it seemed a
bit redundant to read:

session_property_setter.SetSessionProperty(...);

Instead, consider just renaming these methods to Set() so that calling
code is simplified to:

session_property_setter.Set(...);

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode73
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:73: }

On 2016/03/08 22:36:05, miu wrote:
> For sanity, after the Destroy() call here, consider adding:
>
> DCHECK(!encoder_thread_.IsRunning());
> DCHECK(!weak_this_factory_.HasWeakPtrs());
>
> (to be sure the encoder thread and all outstanding tasks have been
> stopped/canceled)

Done.

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode296
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:296: //
Cancel all callbacks.

On 2016/03/08 22:36:05, miu wrote:
> nit: Cancel all encoder thread callbacks.

Done.

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode297
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:297:
weak_this_factory_.InvalidateWeakPtrs();

On 2016/03/08 22:36:05, miu wrote:
> naming nit: Instead of weak_this_factory_, how about
encoder_task_weak_factory_?
> That would make it clear to the reader that these WeakPtrs are being
used only
> for encoder tasks on the encode thread.

Done.

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.h
File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right):

https://codereview.chromium.org/1636083003/diff/400001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode118
content/common/gpu/media/vt_video_encode_accelerator_mac.h:118:
scoped_ptr<base::WeakPtrFactory<Client> > client_ptr_factory_;

On 2016/03/08 22:36:05, miu wrote:
> nit: It's not necessary to use scoped_ptr<> here. Meaning, in
Destroy(), you
> could just call:
>
> client_ptr_factory_.InvalidateWeakPtrs();

I cannot make this a class member as I cannot construct it in ctor and
copy is private.

https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videotoolbox_helpers.cc
File media/base/mac/videotoolbox_helpers.cc (right):

https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videotoolbox_helpers.cc#newcode46
media/base/mac/videotoolbox_helpers.cc:46: std::vector<CFNumberRef>
numbers;

On 2016/03/08 22:36:05, miu wrote:
> This is a good place to use std::array:
>
> std::array<CFNumberRef, 2> numbers = {
> CFNumberCreate(nullptr, kCFNumberSInt32Type, &int_val),
> CFNumberCreate(nullptr, kCFNumberFloat32Type, &float_val)
> };
> base::ScopedCFTypeRef<CFArrayRef> array(CFArrayCreate(
> kCFAllocatorDefault, reinterpret_cast<const
void**>(numbers.data()),
> numbers.size(), &kCFTypeArrayCallBacks));
> for (auto& number : numbers)
> CFRelease(number);

Done.

https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videotoolbox_helpers.h
File media/base/mac/videotoolbox_helpers.h (right):

https://codereview.chromium.org/1636083003/diff/400001/media/base/mac/videotoolbox_helpers.h#newcode55
media/base/mac/videotoolbox_helpers.h:55: bool
SetSessionProperty(CFStringRef key, int32_t value);

On 2016/03/08 22:36:05, miu wrote:
> nit suggestion: When I was reading the code calling these, it seemed a
bit
> redundant to read:
>
> session_property_setter.SetSessionProperty(...);
>
> Instead, consider just renaming these methods to Set() so that calling
code is
> simplified to:
>
> session_property_setter.Set(...);

Done.

https://codereview.chromium.org/1636083003/

[m...@chromium.org]

PS14 lgtm % a few things:


https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
File
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc#newcode330
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:330:
command_line->AppendSwitch(::switches::kDisableVTAcceleratedVideoEncode);
Is this still needed? If the tests now work, let's take this out and
also take out the command-line switch.

https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode95
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95:
DLOG(ERROR) << "Failed creating compression session with hardware
support.";
This would be a good thing to have as VLOG(1) instead. FWIW, I'd be
curious to try this code out in our testing lab to see which Macs are
using HW H264 encoding after this change. :)

https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode446
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:446: // Try

creating session again without forcing HW encode.

nit: Maybe add to this comment that this is seems to be needed in order
to support resolutions below 640x480, for clients that might downgrade
to those resolutions but don't expect the encoder to stop working.

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
File
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc#newcode330
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:330:
command_line->AppendSwitch(::switches::kDisableVTAcceleratedVideoEncode);

On 2016/03/09 01:48:50, miu wrote:
> Is this still needed? If the tests now work, let's take this out and
also take
> out the command-line switch.

Unfortunately it is still needed. In my local tests, I am able to create
a VT HW session, so H264 gets reported and this code triggers Cast's
H264 implementation which fails.

https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode95
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95:
DLOG(ERROR) << "Failed creating compression session with hardware
support.";

On 2016/03/09 01:48:50, miu wrote:
> This would be a good thing to have as VLOG(1) instead. FWIW, I'd be
curious to
> try this code out in our testing lab to see which Macs are using HW
H264
> encoding after this change. :)

Done.

https://codereview.chromium.org/1636083003/diff/420001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode446
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:446: // Try
creating session again without forcing HW encode.

On 2016/03/09 01:48:50, miu wrote:
> nit: Maybe add to this comment that this is seems to be needed in
order to
> support resolutions below 640x480, for clients that might downgrade to
those
> resolutions but don't expect the encoder to stop working.

Done.

https://codereview.chromium.org/1636083003/

[m...@chromium.org]

https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
File
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc#newcode330
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:330:
command_line->AppendSwitch(::switches::kDisableVTAcceleratedVideoEncode);

On 2016/03/09 02:26:03, emircan wrote:
> On 2016/03/09 01:48:50, miu wrote:
> > Is this still needed? If the tests now work, let's take this out
and also
> take
> > out the command-line switch.
>
> Unfortunately it is still needed. In my local tests, I am able to
create a VT HW
> session, so H264 gets reported and this code triggers Cast's H264
implementation
> which fails.

Ok, I think this may be the problem:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/receiver/video_decoder.cc&l=244

In other words, this test is creating a Cast Receiver, but our Chromium
implementation lacks H264 decode support. In that case, what you should
do is force VP8 to be used:

1. Modify the sender JavaScript part of this test code to use VP8
instead of H264 (which became the default, because of your change).
Relevant code is here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/extensions/api_test/cast_streaming/end_to_end_sender.js&l=157

Just set videoParams.payload.codecName = 'VP8' before start() is called
a few lines later. This will force the VP8 encoder to be used instead.

2. Remove this command-line override.

3. Confirm the test runs now. :-)

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
File
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/1636083003/diff/420001/chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc#newcode330
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:330:
command_line->AppendSwitch(::switches::kDisableVTAcceleratedVideoEncode);

Thank you very much. Now, cast test works as expected and I am removing
the flag :)

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

dalecurtis@, can you PTAL media/* and content/common/gpu/media/* as OWNERS
review?

https://codereview.chromium.org/1636083003/

[dalec...@chromium.org]

=> sandersd@

https://codereview.chromium.org/1636083003/

[sand...@chromium.org]

RS LGTM for media % nit.

There seem to be a lot of helper methods that are not actually media-related,
but I am not so worried that I would ask for changes as long as it stays in
mac/.


https://codereview.chromium.org/1636083003/diff/460001/media/base/mac/videotoolbox_glue.h
File media/base/mac/videotoolbox_glue.h (right):

https://codereview.chromium.org/1636083003/diff/460001/media/base/mac/videotoolbox_glue.h#newcode16
media/base/mac/videotoolbox_glue.h:16: // requires OS X 10.6 or iOS 6.
Linking with VideoToolbox therefore has to
This comment is out of date now. In fact we are just waiting for the
build SDK bump so that we can delete this glue file. Unless iOS is still
at 6?

(https://bugs.chromium.org/p/chromium/issues/detail?id=579648)

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

https://codereview.chromium.org/1636083003/diff/460001/media/base/mac/videotoolbox_glue.h
File media/base/mac/videotoolbox_glue.h (right):

https://codereview.chromium.org/1636083003/diff/460001/media/base/mac/videotoolbox_glue.h#newcode16
media/base/mac/videotoolbox_glue.h:16: // requires OS X 10.6 or iOS 6.
Linking with VideoToolbox therefore has to

On 2016/03/09 22:42:08, sandersd wrote:
> This comment is out of date now. In fact we are just waiting for the
build SDK
> bump so that we can delete this glue file. Unless iOS is still at 6?
>
> (https://bugs.chromium.org/p/chromium/issues/detail?id=579648)

Thanks for the heads up. I updated the comment and referenced the bug.

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

a...@chromium.org: Please review changes in content/common/BUILD.gn.

tha...@chromium.org: Please review changes in build/gn_migration.gypi


https://codereview.chromium.org/1636083003/

[tha...@chromium.org]

gn_migration.gypi lgtm

https://codereview.chromium.org/1636083003/

[a...@chromium.org]

content/common/BUILD.gn lgtm

https://codereview.chromium.org/1636083003/

[pos...@chromium.org]

https://codereview.chromium.org/1636083003/diff/460001/content/common/gpu/media/vt_video_encode_accelerator_mac.h
File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right):

https://codereview.chromium.org/1636083003/diff/460001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode54
content/common/gpu/media/vt_video_encode_accelerator_mac.h:54: //
Encoding tasks to be run on |encode_thread|.
s/encode_thread/encoder_thread_/

https://codereview.chromium.org/1636083003/diff/460001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode56
content/common/gpu/media/vt_video_encode_accelerator_mac.h:56: bool
force_keyframe);
Incorrect indent?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode31
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31:
InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks
ref_time)
const& perhaps?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode37
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:37:
DISALLOW_IMPLICIT_CONSTRUCTORS(InProgressFrameEncode);
Is it unsafe to copy this struct?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode75
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:75:
DCHECK(!encoder_task_weak_factory_.HasWeakPtrs());
This is probably not needed? The factory will be destroyed since this is
the destructor and the pointers will be invalidated.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode95
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: VLOG(1)

<< "Failed creating compression session with hardware support.";

Perhaps DLOG(ERROR)? Do we expect all Mac platforms to support hardware
encode at 4k?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode103
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:103:
profile.max_resolution = gfx::Size(kMaxResolutionWidth,
kMaxResolutionHeight);
I'm guessing there is no way to query this? Can we really support 4k,
have you tried?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode217
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:217: if
(!compression_session_) {
We use compression_session_ on encoder_thread_ normally, apart from
initialization and parameter change. Could we move the two latter tasks
to encoder thread as well please? It should still ok to do it on child
thread for GetSupportedProfiles.

I think we'd only have to post an initialization task from Initialize
and have a parameter change task as well?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode289
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:289:
DLOG(ERROR) << " VTCompressionSessionEncodeFrame failed: " << status;
Does VTCompressionSessionEncodeFrame free the request before failing,
since we already release()d it?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode311
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:311: // This
thread runs on |encoder_thread_| if it is alive, otherwise on GPU
DCHECK(encoder_thread_.IsRunning() &&
encoder_thread_.task_runner()->BelongsToCurrentThread()); perhaps?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode355
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:355:
encoder->encoder_task_weak_factory_.GetWeakPtr(),
I don't think we can create a weak pointer on this thread, but
dereference it on encoder thread (please see
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_ptr.h&l=51).

I think a solution here could be generating the weak pointer on encoder
thread and then keeping it in the encoder class and using it from here,
instead of generating it here.
This is assuming that the strong encoder_opaque pointer will remain
valid for all calls to this, since we block destruction of encoder in
DestroyTask() until no more callbacks can come according to the comment
there.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode424
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:424:
CFTypeRef attributes_keys[] = {
const?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode431
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:431:
CFTypeRef attributes_values[] = {
const?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode436
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:436:
.release()};
static_assert(arraysize(attributes_keys) ==
arraysize(attributes_values), ...)?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode441
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:441:
CFRelease(v);
Do we need to explicitly do this, won't this happen when attributes
falls out of scope when we return?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode448
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:448: //
resolutions, we need to create a session.
Would just passing false in l.444 have the same effect?

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode459
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:459:
RequestEncodingParametersChange(target_bitrate_, frame_rate_);
Do we want to Request if Configure fails? We will fail in the caller
anyway I think...?

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode31
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31:
InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks
ref_time)

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> const& perhaps?

Done.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode37
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:37:
DISALLOW_IMPLICIT_CONSTRUCTORS(InProgressFrameEncode);

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> Is it unsafe to copy this struct?

I added after mcasas's comment on #21. It ensures that we do not make an
unnecessary copy.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode75
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:75:
DCHECK(!encoder_task_weak_factory_.HasWeakPtrs());

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> This is probably not needed? The factory will be destroyed since this
is the
> destructor and the pointers will be invalidated.

I added after miu's suggestion on #59. It ensures that everything is
cleaned as expected.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode95
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: VLOG(1)
<< "Failed creating compression session with hardware support.";

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> Perhaps DLOG(ERROR)? Do we expect all Mac platforms to support
hardware encode
> at 4k?

I added after miu's suggestion on #61. It helps to check support on
different platforms.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode103
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:103:
profile.max_resolution = gfx::Size(kMaxResolutionWidth,
kMaxResolutionHeight);

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> I'm guessing there is no way to query this? Can we really support 4k,
have you
> tried?

Yes, there isn't a way to query these capabilities. I was able to create
a session with 4K setting, but I didn't go through all Macs yet.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode217
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:217: if
(!compression_session_) {

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> We use compression_session_ on encoder_thread_ normally, apart from
> initialization and parameter change. Could we move the two latter
tasks to
> encoder thread as well please? It should still ok to do it on child
thread for
> GetSupportedProfiles.
>
> I think we'd only have to post an initialization task from Initialize
and have a
> parameter change task as well?

Initialize() is expected to return a bool on child thread. We would like
to return false if we fail to create and set properties of
compression_session_. So, I think, it would make more sense to do these
on child thread.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode289
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:289:
DLOG(ERROR) << " VTCompressionSessionEncodeFrame failed: " << status;

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> Does VTCompressionSessionEncodeFrame free the request before failing,
since we
> already release()d it?

I followed the same logic with these two implementations. There isn't a
clear documentation I can point to, but I will be safe and release if
only successful.
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/h264_vt_encoder.cc&l=591
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_encoder.cc&l=290

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode311
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:311: // This
thread runs on |encoder_thread_| if it is alive, otherwise on GPU

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> DCHECK(encoder_thread_.IsRunning() &&
> encoder_thread_.task_runner()->BelongsToCurrentThread()); perhaps?

Done.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode355
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:355:
encoder->encoder_task_weak_factory_.GetWeakPtr(),

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> I don't think we can create a weak pointer on this thread, but
dereference it on
> encoder thread (please see
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_ptr.h&l=51).
>
> I think a solution here could be generating the weak pointer on
encoder thread
> and then keeping it in the encoder class and using it from here,
instead of
> generating it here.
> This is assuming that the strong encoder_opaque pointer will remain
valid for
> all calls to this, since we block destruction of encoder in
DestroyTask() until
> no more callbacks can come according to the comment there.

Thanks. Done.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode424
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:424:
CFTypeRef attributes_keys[] = {

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> const?

Done.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode431
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:431:
CFTypeRef attributes_values[] = {

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> const?

CFDictionaryCreate() expects non-const input.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode436
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:436:
.release()};

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> static_assert(arraysize(attributes_keys) ==
arraysize(attributes_values), ...)?

CFDictionaryCreate() expects non-const input.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode441
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:441:
CFRelease(v);

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> Do we need to explicitly do this, won't this happen when attributes
falls out of
> scope when we return?

Yes. We release() the attributes to create the array so we need destruct
manually. Same logic applies here:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/h264_vt_encoder.cc&l=422

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode448
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:448: //
resolutions, we need to create a session.

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> Would just passing false in l.444 have the same effect?

We want to set
kVTVideoEncoderSpecification_RequireHardwareAcceleratedVideoEncoder as
true in the first try to ensure we get HW support.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode459
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:459:
RequestEncodingParametersChange(target_bitrate_, frame_rate_);

On 2016/03/10 05:25:35, Pawel Osciak wrote:
> Do we want to Request if Configure fails? We will fail in the caller
anyway I
> think...?

Done.

https://codereview.chromium.org/1636083003/

[mca...@chromium.org]

LGTM FWIW.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode31
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31:
InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks
ref_time)
On 2016/03/10 05:25:35, Pawel Osciak wrote:
> const& perhaps?

Negative, see [1]. There's even a number
of presubmit checks catching this e.g. [2].

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&sq=package:chromium&type=cs&q=base::timeticks&l=24
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/cc/PRESUBMIT.py&q=presubmit%20timeticks&sq=package:chromium&type=cs&l=107

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

Thanks.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode31
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31:
InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks
ref_time)

On 2016/03/10 17:42:07, mcasas wrote:
> On 2016/03/10 05:25:35, Pawel Osciak wrote:
> > const& perhaps?
>
> Negative, see [1]. There's even a number
> of presubmit checks catching this e.g. [2].
>
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&sq=package:chromium&type=cs&q=base::timeticks&l=24
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/PRESUBMIT.py&q=presubmit%20timeticks&sq=package:chromium&type=cs&l=107

Reverted.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode424
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:424:
CFTypeRef attributes_keys[] = {

On 2016/03/10 06:51:21, emircan wrote:
> On 2016/03/10 05:25:35, Pawel Osciak wrote:

> > const?
>
> Done.

Ignore my earlier reply. CFDictionaryCreate() expects non-const input.

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

https://chromiumcodereview.appspot.com/1636083003/diff/460001/content/common/gpu/media/vt_video_encode_accelerator_mac.h
File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right):

https://chromiumcodereview.appspot.com/1636083003/diff/460001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode54

content/common/gpu/media/vt_video_encode_accelerator_mac.h:54: //
Encoding tasks to be run on |encode_thread|.

On 2016/03/10 05:25:35, Pawel Osciak wrote:

> s/encode_thread/encoder_thread_/

Done.

https://chromiumcodereview.appspot.com/1636083003/diff/460001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode56
content/common/gpu/media/vt_video_encode_accelerator_mac.h:56: bool
force_keyframe);

On 2016/03/10 05:25:35, Pawel Osciak wrote:

> Incorrect indent?

Done.

https://chromiumcodereview.appspot.com/1636083003/

[pos...@chromium.org]

https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode31

content/common/gpu/media/vt_video_encode_accelerator_mac.cc:31:
InProgressFrameEncode(base::TimeDelta rtp_timestamp, base::TimeTicks
ref_time)

On 2016/03/10 18:53:20, emircan wrote:
> On 2016/03/10 17:42:07, mcasas wrote:

> > On 2016/03/10 05:25:35, Pawel Osciak wrote:

> > > const& perhaps?
> >
> > Negative, see [1]. There's even a number
> > of presubmit checks catching this e.g. [2].
> >
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&sq=package:chromium&type=cs&q=base::timeticks&l=24
> > [2]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/PRESUBMIT.py&q=presubmit%20timeticks&sq=package:chromium&type=cs&l=107
>
> Reverted.

Acknowledged. Good point Miguel.

https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode95

content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: VLOG(1)
<< "Failed creating compression session with hardware support.";

On 2016/03/10 06:51:21, emircan wrote:

> On 2016/03/10 05:25:35, Pawel Osciak wrote:

> > Perhaps DLOG(ERROR)? Do we expect all Mac platforms to support
hardware encode
> > at 4k?
>
> I added after miu's suggestion on #61. It helps to check support on
different
> platforms.

Perhaps the message should say something like "HW acceleration not
available", to make the message not look like an error on platforms
where it is not available?

https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode217
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:217: if
(!compression_session_) {

On 2016/03/10 06:51:21, emircan wrote:

> On 2016/03/10 05:25:35, Pawel Osciak wrote:

> > We use compression_session_ on encoder_thread_ normally, apart from
> > initialization and parameter change. Could we move the two latter
tasks to
> > encoder thread as well please? It should still ok to do it on child
thread for
> > GetSupportedProfiles.
> >
> > I think we'd only have to post an initialization task from
Initialize and have
> a
> > parameter change task as well?
>
> Initialize() is expected to return a bool on child thread. We would
like to
> return false if we fail to create and set properties of
compression_session_.
> So, I think, it would make more sense to do these on child thread.

You could block Initialize until that returned on a WaitableEvent. But
I'm also ok with doing Initialize things on Child, before encoder thread
is started. But parameter change should hopefully happen on encoder
thread please. Better not to access compression_session_ concurrently
from multiple threads.

https://chromiumcodereview.appspot.com/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode448

content/common/gpu/media/vt_video_encode_accelerator_mac.cc:448: //
resolutions, we need to create a session.

On 2016/03/10 06:51:21, emircan wrote:

> On 2016/03/10 05:25:35, Pawel Osciak wrote:

> > Would just passing false in l.444 have the same effect?
>
> We want to set
> kVTVideoEncoderSpecification_RequireHardwareAcceleratedVideoEncoder as
true in
> the first try to ensure we get HW support.

I think you could use
kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder for
that then?
Doc says: "(...)If set to kCFBooleanTrue, use a hardware accelerated
video encoder if available.(...)"

https://chromiumcodereview.appspot.com/1636083003/diff/520001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://chromiumcodereview.appspot.com/1636083003/diff/520001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode295
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:295:
CHECK(request.release());
Unless I'm missing something, we passed a raw pointer to
VTCompressionSessionEncodeFrame(), so this frees it I believe, and since
VTCompressionSessionEncodeFrame() didn't take a ref, now it has a
dangling pointer?

https://chromiumcodereview.appspot.com/1636083003/

[emi...@chromium.org]

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode95

content/common/gpu/media/vt_video_encode_accelerator_mac.cc:95: VLOG(1)
<< "Failed creating compression session with hardware support.";

On 2016/03/11 00:43:59, Pawel Osciak wrote:
> On 2016/03/10 06:51:21, emircan wrote:
> > On 2016/03/10 05:25:35, Pawel Osciak wrote:
> > > Perhaps DLOG(ERROR)? Do we expect all Mac platforms to support
hardware
> encode
> > > at 4k?
> >
> > I added after miu's suggestion on #61. It helps to check support on
different
> > platforms.
>
> Perhaps the message should say something like "HW acceleration not
available",
> to make the message not look like an error on platforms where it is
not
> available?

Done.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode217
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:217: if
(!compression_session_) {

I see. I will move RequestEncodingParametersChange to the encoder
thread.

https://codereview.chromium.org/1636083003/diff/480001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode448

content/common/gpu/media/vt_video_encode_accelerator_mac.cc:448: //
resolutions, we need to create a session.

On 2016/03/11 00:43:59, Pawel Osciak wrote:
> On 2016/03/10 06:51:21, emircan wrote:
> > On 2016/03/10 05:25:35, Pawel Osciak wrote:
> > > Would just passing false in l.444 have the same effect?
> >
> > We want to set
> > kVTVideoEncoderSpecification_RequireHardwareAcceleratedVideoEncoder
as true in
> > the first try to ensure we get HW support.
>
> I think you could use
> kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder for
that
> then?
> Doc says: "(...)If set to kCFBooleanTrue, use a hardware accelerated
video
> encoder if available.(...)"

As we discussed offline, I am setting it to false on l.444.

https://codereview.chromium.org/1636083003/diff/520001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/520001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode295
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:295:
CHECK(request.release());

On 2016/03/11 00:43:59, Pawel Osciak wrote:
> Unless I'm missing something, we passed a raw pointer to
> VTCompressionSessionEncodeFrame(), so this frees it I believe, and
since
> VTCompressionSessionEncodeFrame() didn't take a ref, now it has a
dangling
> pointer?

I am updating the comment. If encode is successful, we release and let
callback take care of it. Otherwise we can let if fall out of scope.

https://codereview.chromium.org/1636083003/

[pos...@chromium.org]

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode97
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:97: VLOG(1)
<< "Hardware support is not available on this platform.";
Perhaps something like "Hardware encode acceleration is not..."

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode171
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:171:
encoder_thread_.message_loop()->PostTask(
Nit: you could in Initialize:

encoder_thread_task_runner_ = encoder_thread_.task_runner();

and then:

encoder_thread_task_runner_->PostTask()

everywhere

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode174
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:174:
encoder_weak_ptr_,
Actually, unretained is ok, because we have a guarantee that the encoder
thread will be destroyed before |this|, as it's a member of |this|.

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode278
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:278:
CHECK(request.release());
Sorry, perhaps I'm missing something, however we are releasing this
here, but then releasing it again in CompressionCallback from what I can
see?

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode482
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:482:
->kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder());
I think Enable is not needed if we Require per docs...

so perhaps:

if (require_hw_encoding) {
push back require
} else {
push back enable
}

?

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode544
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:544: // This
method may be called on |encoder thread| or GPU child thread.
You could DCHECK((encoder_thread_.IsRunning() && <on encoder>) || <on
child>)

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.h
File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right):

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode54
content/common/gpu/media/vt_video_encode_accelerator_mac.h:54: //
Encoding tasks to be run on |encoder_thread|.
Nit: encoder_thread_

https://chromiumcodereview.appspot.com/1636083003/

[emi...@chromium.org]

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode97

content/common/gpu/media/vt_video_encode_accelerator_mac.cc:97: VLOG(1)
<< "Hardware support is not available on this platform.";

On 2016/03/11 09:54:46, Pawel Osciak wrote:
> Perhaps something like "Hardware encode acceleration is not..."

Done.

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode171
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:171:
encoder_thread_.message_loop()->PostTask(

On 2016/03/11 09:54:46, Pawel Osciak wrote:
> Nit: you could in Initialize:
>
> encoder_thread_task_runner_ = encoder_thread_.task_runner();
>
> and then:
>
> encoder_thread_task_runner_->PostTask()
>
> everywhere

Done.

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode174
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:174:
encoder_weak_ptr_,

On 2016/03/11 09:54:46, Pawel Osciak wrote:
> Actually, unretained is ok, because we have a guarantee that the
encoder thread
> will be destroyed before |this|, as it's a member of |this|.

I agree. I changed them to WeakPtr after my recent discussion about
WeakPtrs vs Unretained where we concluded that it is much safer to avoid
Unretained wherever we can. In this instance, Unretained is safe as you
explained and WeakPtr is safe as |encoder_weak_ptr| is dereferenced and
invalidated in dtor() on |encoder_thread_|. Still, if you think it is
not a good practice or cause a different behavior, I can change.

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode278
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:278:
CHECK(request.release());

On 2016/03/11 09:54:46, Pawel Osciak wrote:
> Sorry, perhaps I'm missing something, however we are releasing this
here, but
> then releasing it again in CompressionCallback from what I can see?

I pass request.get() to VTCompressionSessionEncodeFrame which is a naked
ptr.
- If encode is successful, I call release() here so that |request|
doesn't get freed when it goes out of scope. In CompressionCallback, I
wrap it into a scoped_ptr and let it go out of scope (by calling
reset()).
- If encode isn't successful, I let |request| go out of scope and get
freed.
The reason why I have CHECK() is to suppress unused result warning.

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode482
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:482:
->kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder());

On 2016/03/11 09:54:46, Pawel Osciak wrote:
> I think Enable is not needed if we Require per docs...
>
> so perhaps:
>
> if (require_hw_encoding) {
> push back require
> } else {
> push back enable
> }
>
> ?

Done.

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode544

content/common/gpu/media/vt_video_encode_accelerator_mac.cc:544: // This
method may be called on |encoder thread| or GPU child thread.

On 2016/03/11 09:54:46, Pawel Osciak wrote:
> You could DCHECK((encoder_thread_.IsRunning() && <on encoder>) || <on
child>)

Done.

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.h
File content/common/gpu/media/vt_video_encode_accelerator_mac.h (right):

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.h#newcode54

content/common/gpu/media/vt_video_encode_accelerator_mac.h:54: //
Encoding tasks to be run on |encoder_thread|.

On 2016/03/11 09:54:46, Pawel Osciak wrote:
> Nit: encoder_thread_

Done.

https://codereview.chromium.org/1636083003/

[pos...@chromium.org]

c/c/g/m/ lgtm % comment

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://chromiumcodereview.appspot.com/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode174
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:174:
encoder_weak_ptr_,

On 2016/03/12 00:16:11, emircan wrote:
> On 2016/03/11 09:54:46, Pawel Osciak wrote:
> > Actually, unretained is ok, because we have a guarantee that the
encoder
> thread
> > will be destroyed before |this|, as it's a member of |this|.
>
> I agree. I changed them to WeakPtr after my recent discussion about
WeakPtrs vs
> Unretained where we concluded that it is much safer to avoid
Unretained wherever
> we can. In this instance, Unretained is safe as you explained and
WeakPtr is
> safe as |encoder_weak_ptr| is dereferenced and invalidated in dtor()
on
> |encoder_thread_|. Still, if you think it is not a good practice or
cause a
> different behavior, I can change.

Personally I'd prefer unretained please.

https://chromiumcodereview.appspot.com/1636083003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1636083003/620001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1636083003/620001


https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

Thanks.

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc
File content/common/gpu/media/vt_video_encode_accelerator_mac.cc
(right):

https://codereview.chromium.org/1636083003/diff/580001/content/common/gpu/media/vt_video_encode_accelerator_mac.cc#newcode174
content/common/gpu/media/vt_video_encode_accelerator_mac.cc:174:
encoder_weak_ptr_,

On 2016/03/15 07:27:47, Pawel Osciak wrote:
> On 2016/03/12 00:16:11, emircan wrote:
> > On 2016/03/11 09:54:46, Pawel Osciak wrote:
> > > Actually, unretained is ok, because we have a guarantee that the
encoder
> > thread
> > > will be destroyed before |this|, as it's a member of |this|.
> >
> > I agree. I changed them to WeakPtr after my recent discussion about
WeakPtrs
> vs
> > Unretained where we concluded that it is much safer to avoid
Unretained
> wherever
> > we can. In this instance, Unretained is safe as you explained and
WeakPtr is
> > safe as |encoder_weak_ptr| is dereferenced and invalidated in dtor()
on
> > |encoder_thread_|. Still, if you think it is not a good practice or
cause a
> > different behavior, I can change.
>
> Personally I'd prefer unretained please.

Done.

https://codereview.chromium.org/1636083003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #22 (id:620001)

https://codereview.chromium.org/1636083003/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 22 (id:??) landed as
https://crrev.com/3956fa1cac34dd5682c271d77463accdd7191102
Cr-Commit-Position: refs/heads/master@{#381286}

https://codereview.chromium.org/1636083003/

[dew...@chromium.org]

A revert of this CL (patchset #22 id:620001) has been created in
https://codereview.chromium.org/1801343002/ by dew...@chromium.org.

The reason for reverting is: This probably breaks Mac compile:
https://build.chromium.org/p/chromium/builders/Mac/builds/13208/steps/compile/logs/stdio

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/content/common/gpu/media/jpeg_decode_accelerator_unittest.jpeg_decode_accelerator_unittest.o.d
-DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2
-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DCHROMIUM_BUILD
-DCR_CLANG_REVISION=263324-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1
-DENABLE_MEDIA_ROUTER=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY
-DENABLE_NOTIFICATIONS -DENABLE_TOPCHROME_MD=1 -DFIELDTRIAL_TESTING_ENABLED
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PDF=1
-DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1
-DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
-DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1
-DENABLE_SERVICE_DISCOVERY=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING
-DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DGTEST_HAS_POSIX_RE=0
-DGTEST_LANG_CXX11=0 -DMOJO_USE_SYSTEM_IMPL -DUNIT_TEST -DGTEST_HAS_RTTI=0
-DSK_SUPPORT_GPU=1 -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS
-DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION
-DMESA_EGL_NO_X11_HEADERS -DUSE_LIBPCI=1 -DUSE_OPENSSL=1
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND
-DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen
-I../../third_party/libva -I../../third_party/libyuv -I../../third_party/khronos
-I../../gpu -I../.. -I../../skia/config -Igen/angle
-I../../third_party/WebKit/Source -I../../third_party/opus/src/include
-I../../testing/gtest/include -I../../third_party/libyuv/include
-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../../third_party/skia/include/utils/mac -I../../skia/ext
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-I../../third_party/mesa/src/include -I../../third_party/WebKit -isysroot
/Applications/Xcode5.1.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk
-O2 -gdwarf-2 -fvisibility=hidden -Werror -mmacosx-version-min=10.6 -arch x86_64
-Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers
-Wno-selector-type-mismatch -Wpartial-availability -Wheader-hygiene
-Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wno-inconsistent-missing-override
-Wno-shift-negative-value -std=c++11 -stdlib=libc++ -fno-rtti -fno-exceptions
-fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang -load -Xclang
/b/build/slave/Mac/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -Xclang
-plugin-arg-find-bad-constructs -Xclang follow-macro-expansion
-fcolor-diagnostics -fno-strict-aliasing -c
../../content/common/gpu/media/jpeg_decode_accelerator_unittest.cc -o
obj/content/common/gpu/media/jpeg_decode_accelerator_unittest.jpeg_decode_accelerator_unittest.o
../../content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:152:2: error:
The JpegDecodeAccelerator is not supported on this platform.
#error The JpegDecodeAccelerator is not supported on this platform.
^
1 error generated.
ninja: build stopped: subcommand failed..

https://codereview.chromium.org/1636083003/

[emi...@chromium.org]

gyp issue is fixed and CL relanded on https://codereview.webrtc.org/1805723002/.

https://codereview.webrtc.org/1636083003/