Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
760963003:
Adds fake hardware video encoder. (Closed)
|
Created:
6 years ago by hellner1 Modified:
5 years, 10 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds fake hardware video encoder.
Adds a fake video encode accelerator (hardware encoder) implementation that can be used for testing (on devices without hardware encoder or missing implementation) and testing the efficiency of an existing hardware encoder.
BUG=440843
Committed: https://crrev.com/a15357a6a83685010421056c16d52e94077569cd
Cr-Commit-Position: refs/heads/master@{#312893}
Patch Set 1 #Patch Set 2 : made the fake video encode accelerator look more like the one in media/cast #Patch Set 3 : Moved the fake implementation to media/video #Patch Set 4 : forgot to change include guards #Patch Set 5 : Rebase #
Total comments: 34
Patch Set 6 : Updates according to comments. #
Total comments: 8
Patch Set 7 : updates according to comments. #
Total comments: 13
Patch Set 8 : Updates according to comments. #Patch Set 9 : updates according to comments #
Total comments: 11
Patch Set 10 : Updates according to comments. #Patch Set 11 : rebase #Patch Set 12 : Update according to comments #Patch Set 13 : rebase #
Total comments: 14
Patch Set 14 : Rebase #Patch Set 15 : Fake video encoder wasnt listing supported profiles which prevented it from being used in chromium … #Patch Set 16 : Updates according to comments #
Total comments: 1
Patch Set 17 : rebase #Patch Set 18 : rebase #Patch Set 19 : Fixes + rebase #Patch Set 20 : Fixes broken unittest #Patch Set 21 : Rebase #Patch Set 22 : updated gn-files to match gyp #Patch Set 23 : Fixes link issue for component build #Patch Set 24 : Fixed broken unittest #Messages
Total messages: 60 (19 generated)
hellner@chromium.org changed reviewers: + mcasas@chromium.org, posciak@chromium.org, wuchengli@chromium.org
"I'm also not clear on the use case here. What would this benchmark?" - Pawel 1) One can test on any device. E.g. ones that don't have a hardware encoder (e.g. pixel) or ones that don't have the software hooked up to the hardware encoder yet. 2) One can benchmark a hardware encoders software implementation by comparing the achieved frame rate for fake vs non-fake. Mainly 1 is interesting since that enables me to develop on my pixel since other devices are harder to come by. Especially if it is a device that is not yet in production. It will also make it easier to start optimization for new devices as we have a encoder/decoder that will be of similar resource intensity as the hardware encoder/decoders once they are hooked up. "Also, I think a much more interesting mock would be one hooking up libvpx to act as the encoder." - Pawel That would be more resource intensive. For me the resource intensity is more important than generating a valid stream given the input frames.
On 2014/11/27 02:09:15, hellner1 wrote: > "I'm also not clear on the use case here. What would this benchmark?" - Pawel > 1) One can test on any device. E.g. ones that don't have a hardware encoder > (e.g. pixel) or ones that don't have the software hooked up to the hardware > encoder yet. > 2) One can benchmark a hardware encoders software implementation by comparing > the achieved frame rate for fake vs non-fake. We use content/common/gpu/media/video_encode_accelerator_uniitest.cc for benchmarking hardware encoder performance. > Mainly 1 is interesting since that enables me to develop on my pixel since other > devices are harder to come by. Especially if it is a device that is not yet in > production. It will also make it easier to start optimization for new devices as > we have a encoder/decoder that will be of similar resource intensity as the > hardware encoder/decoders once they are hooked up. > > "Also, I think a much more interesting mock would be one hooking up libvpx to > act as the encoder." - Pawel > That would be more resource intensive. For me the resource intensity is more > important than generating a valid stream given the input frames. Ok. Could we please reuse the existing mock implementation in cast? I think it should be moved here for consistency.
On 2014/11/27 02:25:11, Pawel Osciak wrote: > On 2014/11/27 02:09:15, hellner1 wrote: > > "I'm also not clear on the use case here. What would this benchmark?" - Pawel > > 1) One can test on any device. E.g. ones that don't have a hardware encoder > > (e.g. pixel) or ones that don't have the software hooked up to the hardware > > encoder yet. > > 2) One can benchmark a hardware encoders software implementation by comparing > > the achieved frame rate for fake vs non-fake. > > We use content/common/gpu/media/video_encode_accelerator_uniitest.cc for > benchmarking hardware encoder performance. > > > Mainly 1 is interesting since that enables me to develop on my pixel since > other > > devices are harder to come by. Especially if it is a device that is not yet in > > production. It will also make it easier to start optimization for new devices > as > > we have a encoder/decoder that will be of similar resource intensity as the > > hardware encoder/decoders once they are hooked up. > > > > "Also, I think a much more interesting mock would be one hooking up libvpx to > > act as the encoder." - Pawel > > That would be more resource intensive. For me the resource intensity is more > > important than generating a valid stream given the input frames. > > > Ok. Could we please reuse the existing mock implementation in cast? I think it > should be moved here for consistency. Content depends on media so I had to move the fake implementation to there. It seems like media/video, next to the video_encode_accelerator.h is the most logical place for it. WDYT? My fear is that since it now also touches media that it is more likely to be rejected, but we'll see how it goes.
hellner@chromium.org changed reviewers: + vrk@chromium.org
+vrk for media/* PTAL
hellner@chromium.org changed reviewers: + hclam@chromium.org
Sorry, how about vrk for media/media.gyp +hclam for media/cast
Ping @ everyone but Miguel
xhwang@chromium.org changed reviewers: + sandersd@chromium.org - vrk@chromium.org
Replace vrk with sandersd as the media/ owner.
https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. The change description should have a summary followed by a blank line because the summary will become git title. Usually each line of the summary and the detailed description should be <= 75 characters. It's still nice to associate the CL with a bug. Also please add TEST=. You can test by running cast and vea unittests. https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:29: #include "media/video/fake_video_encode_accelerator.h" move this before #include "media/video/video_encode_accelerator.h" https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:134: fake_encoder(0) {} Do you want to use different encoders for different TestStream? If not, fake_encoder should be a command line argument, stored in VideoEncodeAcceleratorTestEnvironment, and passed to VEAClient. https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:164: int fake_encoder; Use bool. https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:400: const bool keyframe = !(stream[0] & 0x01); This is a hidden dependency to FakeVideoEncodeAccelerator. Add a bool FakeVideoEncodeAccelerator::IsKeyFrame(const uint8* stream, size_t size) for this. https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:891: validator_->ProcessStreamBuffer(stream_ptr, payload_size); I think it's simpler to set |validator_| to NULL if fake encoder is used. Since it's a fake encoder, there's no need to validate the output. PassThroughValidator can be removed. Here the code can be: if (validator_) validator_->ProcessStreamBuffer(); else HandleEncodedFrame(key_frame); https://codereview.chromium.org/760963003/diff/80001/media/cast/sender/extern... File media/cast/sender/external_video_encoder_unittest.cc (right): https://codereview.chromium.org/760963003/diff/80001/media/cast/sender/extern... media/cast/sender/external_video_encoder_unittest.cc:163: const std::vector<uint32>* stored_bitrates_; Can we replace stored_bitrates_ with fake_vea_->stored_bitrates()? Since stored_bitrates is part of fake_vea_. There's no need to use a separate pointer to store it. https://codereview.chromium.org/760963003/diff/80001/media/cast/sender/video_... File media/cast/sender/video_sender_unittest.cc (right): https://codereview.chromium.org/760963003/diff/80001/media/cast/sender/video_... media/cast/sender/video_sender_unittest.cc:256: const std::vector<uint32>* stored_bitrates_; This is not very good because it is a part of PeerVideoSender. Also, I think it's better to use googlemock EXPECT_CALL to count the number of method call. We can have a mock class to inherit from FakeVEA. But there's no need to fix it in this CL. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:16: static const size_t kMinimumOutputBufferSize = 1234; Why this has been changed? https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:29: : child_message_loop_proxy_(task_runner), I assume you don't force FakeVEA to run on the child thread. Right? So this should keep the name task_runner_. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:50: if (output_profile == VIDEO_CODEC_PROFILE_UNKNOWN) { Return false for profile > VIDEO_CODEC_PROFILE_MAX https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:51: return false; Why will_initialization_succeed_ is removed? https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:80: DoBitstreamBufferReady(id, Remove DoBitstreamBufferReady and just call client_->BitstreamBufferReady? https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:94: available_buffers_.push_back( Can we store BitstreamBuffer in |available_buffers_|? Create the SharedMemory and map it in Encode(). https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.h:27: const std::vector<uint32>& stored_bitrates); This constructor parameter is not needed because you have stored_bitrates().
PTAL https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/12/09 14:29:24, wuchengli wrote: > The change description should have a summary followed by a blank line because > the summary will become git title. Usually each line of the summary and the > detailed description should be <= 75 characters. > > It's still nice to associate the CL with a bug. Also please add TEST=. You can > test by running cast and vea unittests. Done. I did run vea unittests. But since cast_unittests doesn't seem to work, even without any changes, testing it with the changes make little sense. I was planning to rope in a reviewer of cast (hclam) for helping me figure out why ToT cast unittest is not working. https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:29: #include "media/video/fake_video_encode_accelerator.h" On 2014/12/09 14:29:24, wuchengli wrote: > move this before #include "media/video/video_encode_accelerator.h" Done. https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:134: fake_encoder(0) {} On 2014/12/09 14:29:24, wuchengli wrote: > Do you want to use different encoders for different TestStream? If not, > fake_encoder should be a command line argument, stored in > VideoEncodeAcceleratorTestEnvironment, and passed to VEAClient. I made fake_encoder selection a command line argument. https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:164: int fake_encoder; On 2014/12/09 14:29:24, wuchengli wrote: > Use bool. Done. https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:400: const bool keyframe = !(stream[0] & 0x01); On 2014/12/09 14:29:25, wuchengli wrote: > This is a hidden dependency to FakeVideoEncodeAccelerator. Add a bool > FakeVideoEncodeAccelerator::IsKeyFrame(const uint8* stream, size_t size) for > this. Removed the need for this as suggested in other comment. PTAL. https://codereview.chromium.org/760963003/diff/80001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:891: validator_->ProcessStreamBuffer(stream_ptr, payload_size); On 2014/12/09 14:29:24, wuchengli wrote: > I think it's simpler to set |validator_| to NULL if fake encoder is used. Since > it's a fake encoder, there's no need to validate the output. > PassThroughValidator can be removed. Here the code can be: > if (validator_) > validator_->ProcessStreamBuffer(); > else > HandleEncodedFrame(key_frame); Good suggestion! Done. https://codereview.chromium.org/760963003/diff/80001/media/cast/sender/extern... File media/cast/sender/external_video_encoder_unittest.cc (right): https://codereview.chromium.org/760963003/diff/80001/media/cast/sender/extern... media/cast/sender/external_video_encoder_unittest.cc:163: const std::vector<uint32>* stored_bitrates_; On 2014/12/09 14:29:25, wuchengli wrote: > Can we replace stored_bitrates_ with fake_vea_->stored_bitrates()? Since > stored_bitrates is part of fake_vea_. There's no need to use a separate pointer > to store it. Done. https://codereview.chromium.org/760963003/diff/80001/media/cast/sender/video_... File media/cast/sender/video_sender_unittest.cc (right): https://codereview.chromium.org/760963003/diff/80001/media/cast/sender/video_... media/cast/sender/video_sender_unittest.cc:256: const std::vector<uint32>* stored_bitrates_; On 2014/12/09 14:29:25, wuchengli wrote: > This is not very good because it is a part of PeerVideoSender. Also, I think > it's better to use googlemock EXPECT_CALL to count the number of method call. We > can have a mock class to inherit from FakeVEA. But there's no need to fix it in > this CL. Agree. No action then as changing this would bloat the cl. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:16: static const size_t kMinimumOutputBufferSize = 1234; On 2014/12/09 14:29:25, wuchengli wrote: > Why this has been changed? It happened during debugging. I put the old value back. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:29: : child_message_loop_proxy_(task_runner), On 2014/12/09 14:29:25, wuchengli wrote: > I assume you don't force FakeVEA to run on the child thread. Right? So this > should keep the name task_runner_. Done. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:50: if (output_profile == VIDEO_CODEC_PROFILE_UNKNOWN) { On 2014/12/09 14:29:25, wuchengli wrote: > Return false for profile > VIDEO_CODEC_PROFILE_MAX Done. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:51: return false; On 2014/12/09 14:29:25, wuchengli wrote: > Why will_initialization_succeed_ is removed? Done. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:80: DoBitstreamBufferReady(id, On 2014/12/09 14:29:25, wuchengli wrote: > Remove DoBitstreamBufferReady and just call client_->BitstreamBufferReady? I have to post here because calling client_->BitstreamBufferReady will trigger another call to Encode. It also lead to a crash, which I can't remember the details for, but if it is important I can repro. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:94: available_buffers_.push_back( On 2014/12/09 14:29:25, wuchengli wrote: > Can we store BitstreamBuffer in |available_buffers_|? Create the SharedMemory > and map it in Encode(). FYI: this behavior is taken from here: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... I think since BitstreamBuffer is passed as a const reference (and e.g. not a shared pointer) we shouldn't try to store a reference to it. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.h:27: const std::vector<uint32>& stored_bitrates); On 2014/12/09 14:29:25, wuchengli wrote: > This constructor parameter is not needed because you have stored_bitrates(). Done.
https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:80: DoBitstreamBufferReady(id, On 2014/12/10 22:38:59, hellner1 wrote: > On 2014/12/09 14:29:25, wuchengli wrote: > > Remove DoBitstreamBufferReady and just call client_->BitstreamBufferReady? > > I have to post here because calling client_->BitstreamBufferReady will trigger > another call to Encode. It also lead to a crash, which I can't remember the > details for, but if it is important I can repro. So you'll change this back to PostTask? The latest patchset calls the function directly. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:94: available_buffers_.push_back( On 2014/12/10 22:38:59, hellner1 wrote: > On 2014/12/09 14:29:25, wuchengli wrote: > > Can we store BitstreamBuffer in |available_buffers_|? Create the SharedMemory > > and map it in Encode(). > > FYI: this behavior is taken from here: I guess v4l2VEA needs to map the memory in UseOutputBitstreamBuffer to make sure the memory is valid. Fake encoder doesn't need to map it at all. Pawel may know better. > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > I think since BitstreamBuffer is passed as a const reference (and e.g. not a > shared pointer) we shouldn't try to store a reference to it. You can make a copy of BitstreamBuffer. BitstreamBuffer is light-weight. std::list<BitstreamBuffer> > available_buffers_; https://codereview.chromium.org/760963003/diff/100001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/760963003/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:102: bool g_fake_encoder = false; Please document this. https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:82: stream[0] = is_key_frame ? 0x00 : 0x01; // Key frame scheme = VP8 Can remove this because is_key_frame is communicated directly by DoBitstreamBufferReady()->client_->BitstreamBufferReady()->VEAClient::HandleEncodedFrame(). https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:58: // Our original calling message loop for the child thread. nit: remove "the child thread" because it doesn't have to be the child thread. https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:63: VideoEncodeAccelerator::Client* client_; nit: add blank line
PTAL https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:80: DoBitstreamBufferReady(id, On 2014/12/11 02:14:31, wuchengli wrote: > On 2014/12/10 22:38:59, hellner1 wrote: > > On 2014/12/09 14:29:25, wuchengli wrote: > > > Remove DoBitstreamBufferReady and just call client_->BitstreamBufferReady? > > > > I have to post here because calling client_->BitstreamBufferReady will trigger > > another call to Encode. It also lead to a crash, which I can't remember the > > details for, but if it is important I can repro. > So you'll change this back to PostTask? The latest patchset calls the function > directly. Sorry, my bad here. Calling without posting does not crash (tested it). Made the change according to original comment. https://codereview.chromium.org/760963003/diff/80001/media/video/fake_video_e... media/video/fake_video_encode_accelerator.cc:94: available_buffers_.push_back( On 2014/12/11 02:14:31, wuchengli wrote: > On 2014/12/10 22:38:59, hellner1 wrote: > > On 2014/12/09 14:29:25, wuchengli wrote: > > > Can we store BitstreamBuffer in |available_buffers_|? Create the > SharedMemory > > > and map it in Encode(). > > > > FYI: this behavior is taken from here: > I guess v4l2VEA needs to map the memory in UseOutputBitstreamBuffer to make sure > the memory is valid. Fake encoder doesn't need to map it at all. Pawel may know > better. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > > > > I think since BitstreamBuffer is passed as a const reference (and e.g. not a > > shared pointer) we shouldn't try to store a reference to it. > You can make a copy of BitstreamBuffer. BitstreamBuffer is light-weight. > std::list<BitstreamBuffer> > available_buffers_; > Done. https://codereview.chromium.org/760963003/diff/100001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/760963003/diff/100001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:102: bool g_fake_encoder = false; On 2014/12/11 02:14:31, wuchengli wrote: > Please document this. Done. https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:82: stream[0] = is_key_frame ? 0x00 : 0x01; // Key frame scheme = VP8 On 2014/12/11 02:14:31, wuchengli wrote: > Can remove this because is_key_frame is communicated directly by > DoBitstreamBufferReady()->client_->BitstreamBufferReady()->VEAClient::HandleEncodedFrame(). Indeed. I forgot to remove it. Thanks for the ping! https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:58: // Our original calling message loop for the child thread. On 2014/12/11 02:14:31, wuchengli wrote: > nit: remove "the child thread" because it doesn't have to be the child thread. Done. https://codereview.chromium.org/760963003/diff/100001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:63: VideoEncodeAccelerator::Client* client_; On 2014/12/11 02:14:31, wuchengli wrote: > nit: add blank line Done.
https://codereview.chromium.org/760963003/diff/120001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/760963003/diff/120001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:729: scoped_refptr<base::SingleThreadTaskRunner>( do we need this casting? https://codereview.chromium.org/760963003/diff/120001/media/cast/cast_testing... File media/cast/cast_testing.gypi (right): https://codereview.chromium.org/760963003/diff/120001/media/cast/cast_testing... media/cast/cast_testing.gypi:87: 'net/mock_cast_transport_sender.h', It would be easier to review if rebase and your changes can be uploaded in separate patchsets. Your changes first and rebase second. https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:18: struct FakeVideoEncodeAccelerator::BitstreamBufferRef { Can remove because not used anymore. https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:78: client_->BitstreamBufferReady(id, kMinimumOutputBufferSize, is_key_frame); Sorry. After thinking again, all client methods (RequireBitstreamBuffers and BitstreamBufferReady) should be called from the same thread to avoid synchronization issues. This should be changed back to PostTask. But I don't know why SendDummyFrameForTesting calls it directly. https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:49: struct BitstreamBufferRef; not used anymore. remove https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:55: // Our original (constructor-) calling message loop used for all tasks. nit: there's an extra "-" after "constructor"?
PTAL https://codereview.chromium.org/760963003/diff/120001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/760963003/diff/120001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:729: scoped_refptr<base::SingleThreadTaskRunner>( On 2014/12/12 03:35:50, wuchengli wrote: > do we need this casting? Done. https://codereview.chromium.org/760963003/diff/120001/media/cast/cast_testing... File media/cast/cast_testing.gypi (right): https://codereview.chromium.org/760963003/diff/120001/media/cast/cast_testing... media/cast/cast_testing.gypi:87: 'net/mock_cast_transport_sender.h', On 2014/12/12 03:35:50, wuchengli wrote: > It would be easier to review if rebase and your changes can be uploaded in > separate patchsets. Your changes first and rebase second. I have done that in previous patchsets. Sorry that I missed it here :/ https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:18: struct FakeVideoEncodeAccelerator::BitstreamBufferRef { On 2014/12/12 03:35:50, wuchengli wrote: > Can remove because not used anymore. Done. https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:78: client_->BitstreamBufferReady(id, kMinimumOutputBufferSize, is_key_frame); On 2014/12/12 03:35:50, wuchengli wrote: > Sorry. After thinking again, all client methods (RequireBitstreamBuffers and > BitstreamBufferReady) should be called from the same thread to avoid > synchronization issues. This should be changed back to PostTask. > > But I don't know why SendDummyFrameForTesting calls it directly. This class assumes that it is called on the task_runner_ thread which means that all callbacks will automatically be on that thread as there are no other threads in this class. However, cast seems to use the class slightly differently so, to play it safe, all callbacks now post to the task_runner_ thread. Note that I had to start queuing up encodes in queued_frames_ as the unittest will call encode more times than there are output buffers. Posting BitstreamBufferReady means that they will be queued up and not worked on until after the unittest's encode loop finishes. https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:49: struct BitstreamBufferRef; On 2014/12/12 03:35:50, wuchengli wrote: > not used anymore. remove Done. https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:55: // Our original (constructor-) calling message loop used for all tasks. On 2014/12/12 03:35:50, wuchengli wrote: > nit: there's an extra "-" after "constructor"? Done.
https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:78: client_->BitstreamBufferReady(id, kMinimumOutputBufferSize, is_key_frame); On 2014/12/12 19:24:41, hellner1 wrote: > On 2014/12/12 03:35:50, wuchengli wrote: > > Sorry. After thinking again, all client methods (RequireBitstreamBuffers and > > BitstreamBufferReady) should be called from the same thread to avoid > > synchronization issues. This should be changed back to PostTask. > > > > But I don't know why SendDummyFrameForTesting calls it directly. > > This class assumes that it is called on the task_runner_ thread which means that > all callbacks will automatically be on that thread as there are no other threads > in this class. > > However, cast seems to use the class slightly differently so, to play it safe, > all callbacks now post to the task_runner_ thread. Basically all VEA methods should be called on the same thread. All callbacks (that is, VEA::Client methods) should be called on the same thread. Those two threads do not have to be the same. So the latest patchset is not correct. Several FakeVEA methods and FakeVEA::DoBitstreamBufferReady can access |available_buffers_| from two different threads. So DoBitstreamBufferReady should have only one line to post a task like before. Rename BitstreamBufferReday to EncodeTask. This means all the class member variables will be accessed by the same thread. void FakeVideoEncodeAccelerator::EncodeTask() { while (!queued_frames_.empty() && !available_buffers_.empty()) { bool force_key_frame = queued_frames_.front(); queued_frames_.pop(); int32 bitstream_buffer_id = available_buffers_.front().id(); available_buffers_.pop_front(); bool key_frame = first_ || force_key_frame; first_ = false; task_runner_->PostTask( FROM_HERE, base::Bind(&FakeVideoEncodeAccelerator::DoBitstreamBufferReady...); } } void FakeVideoEncodeAccelerator::DoBitstreamBufferReady() { DCHECK(task_runner_->BelongsToCurrentThread()); client_->BitstreamBufferReady( bitstream_buffer_id, kMinimumOutputBufferSize, key_frame); } What do you mean cast uses the class differently? Originally all callbacks were posted to the |task_runner_| already. But I think SendDummyFrameForTesting should also use PostTask(&client_->BitstreamBufferReady) (because all callbacks should run on |task_runner_|). In the cast unittests, all SendDummyFrameForTesting should be followed by task_runner_->RunTasks(); to make sure it finishes. > > Note that I had to start queuing up encodes in queued_frames_ as the unittest > will call encode more times than there are output buffers. You are right. This class needs to add |queued_frames_|. > Posting BitstreamBufferReady means that they will be queued up and not worked on until > after the unittest's encode loop finishes.
On 2014/12/13 02:10:22, wuchengli wrote: > https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... > File media/video/fake_video_encode_accelerator.cc (right): > > https://codereview.chromium.org/760963003/diff/120001/media/video/fake_video_... > media/video/fake_video_encode_accelerator.cc:78: > client_->BitstreamBufferReady(id, kMinimumOutputBufferSize, is_key_frame); > On 2014/12/12 19:24:41, hellner1 wrote: > > On 2014/12/12 03:35:50, wuchengli wrote: > > > Sorry. After thinking again, all client methods (RequireBitstreamBuffers and > > > BitstreamBufferReady) should be called from the same thread to avoid > > > synchronization issues. This should be changed back to PostTask. > > > > > > But I don't know why SendDummyFrameForTesting calls it directly. > > > > This class assumes that it is called on the task_runner_ thread which means > that > > all callbacks will automatically be on that thread as there are no other > threads > > in this class. > > > > However, cast seems to use the class slightly differently so, to play it safe, > > all callbacks now post to the task_runner_ thread. > Basically all VEA methods should be called on the same thread. All callbacks > (that is, VEA::Client methods) should be called on the same thread. Those two > threads do not have to be the same. So the latest patchset is not correct. > Several FakeVEA methods and FakeVEA::DoBitstreamBufferReady can access > |available_buffers_| from two different threads. > > So DoBitstreamBufferReady should have only one line to post a task like before. > Rename BitstreamBufferReday to EncodeTask. This means all the class member > variables will be accessed by the same thread. > > void FakeVideoEncodeAccelerator::EncodeTask() { > while (!queued_frames_.empty() && !available_buffers_.empty()) { > bool force_key_frame = queued_frames_.front(); > queued_frames_.pop(); > int32 bitstream_buffer_id = available_buffers_.front().id(); > available_buffers_.pop_front(); > bool key_frame = first_ || force_key_frame; > first_ = false; > task_runner_->PostTask( > FROM_HERE, > base::Bind(&FakeVideoEncodeAccelerator::DoBitstreamBufferReady...); > } > } > > void FakeVideoEncodeAccelerator::DoBitstreamBufferReady() { > DCHECK(task_runner_->BelongsToCurrentThread()); > client_->BitstreamBufferReady( > bitstream_buffer_id, > kMinimumOutputBufferSize, > key_frame); > } > > What do you mean cast uses the class differently? My bad here, please ignore that comment. > Originally all callbacks were > posted to the |task_runner_| already. But I think SendDummyFrameForTesting > should also use PostTask(&client_->BitstreamBufferReady) (because all callbacks > should run on |task_runner_|). In the cast unittests, all > SendDummyFrameForTesting should be followed by task_runner_->RunTasks(); to make > sure it finishes. PTAL > > > > Note that I had to start queuing up encodes in queued_frames_ as the unittest > > will call encode more times than there are output buffers. > You are right. This class needs to add |queued_frames_|. > > > Posting BitstreamBufferReady means that they will be queued up and not worked > on until > > after the unittest's encode loop finishes.
https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:83: EncodeTask(); This should call DoBitstreamBufferReady to have the same behavior as before. Otherwise |available_buffers_| may be empty and the frame may not be sent. https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:119: size_t kMinimumOutputBufferSize, s/kMinimumOutputBufferSize/payload_size/ https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:122: kMinimumOutputBufferSize, s/kMinimumOutputBufferSize/payload_size/ https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:56: size_t kMinimumOutputBufferSize, s/kMinimumOutputBufferSize/payload_size/ https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:72: std::queue<bool> queued_frames_; nit: add a blank line
PTAL https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:83: EncodeTask(); On 2014/12/16 02:21:48, wuchengli wrote: > This should call DoBitstreamBufferReady to have the same behavior as before. > Otherwise |available_buffers_| may be empty and the frame may not be sent. What should I use for |bitstream_buffer_id| if there are no available buffers? https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:119: size_t kMinimumOutputBufferSize, On 2014/12/16 02:21:48, wuchengli wrote: > s/kMinimumOutputBufferSize/payload_size/ Done. Thanks! https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:122: kMinimumOutputBufferSize, On 2014/12/16 02:21:48, wuchengli wrote: > s/kMinimumOutputBufferSize/payload_size/ Done. https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:56: size_t kMinimumOutputBufferSize, On 2014/12/16 02:21:48, wuchengli wrote: > s/kMinimumOutputBufferSize/payload_size/ Done. https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:72: std::queue<bool> queued_frames_; On 2014/12/16 02:21:48, wuchengli wrote: > nit: add a blank line Done.
LGTM. I'll leave the rest for other people to review. https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:83: EncodeTask(); On 2014/12/16 20:14:08, hellner1 wrote: > On 2014/12/16 02:21:48, wuchengli wrote: > > This should call DoBitstreamBufferReady to have the same behavior as before. > > Otherwise |available_buffers_| may be empty and the frame may not be sent. > > What should I use for |bitstream_buffer_id| if there are no available buffers? You are right. The original FakeVideoEncodeAccelerator::SendDummyFrameForTesting didn't even pop |available_buffers_| and use id 0 directly. It was a potential issue. But another problem is cast unittests intent to force a non keyframe using SendDummyFrameForTesting. https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... In the current patchset, , |key_frame| will be overriden by |first_| and won't be a non keyframe request. We have several options. 1. Keep the original cast version of SendDummyFrameForTesting and send 0 for bitstream buffer id. Leave the problem for cast team to fix in the future. 2. Remove |first_| completely. SendDummyFrameForTesting can simply call Encode(NULL, key_frame) here. You need to check with cast team if cast unittests really require the first encoded frame to be keyframe. 3. Skip |first_| here. void FakeVEA::SendDummyFrameForTesting(bool key_frame) { DCHECK(available_buffers_.size() > 0); int32 bitstream_buffer_id = available_buffers_.front().id(); available_buffers_.pop_front(); task_runner_->PostTask(FROM_HERE, base::Bind(&FakeVideoEncodeAccelerator::DoBitstreamBufferReady, weak_this_factory_.GetWeakPtr(), bitstream_buffer_id, kMinimumOutputBufferSize, key_frame)); }
On 2014/12/17 03:21:22, wuchengli wrote: > LGTM. I'll leave the rest for other people to review. > > https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... > File media/video/fake_video_encode_accelerator.cc (right): > > https://codereview.chromium.org/760963003/diff/160001/media/video/fake_video_... > media/video/fake_video_encode_accelerator.cc:83: EncodeTask(); > On 2014/12/16 20:14:08, hellner1 wrote: > > On 2014/12/16 02:21:48, wuchengli wrote: > > > This should call DoBitstreamBufferReady to have the same behavior as before. > > > Otherwise |available_buffers_| may be empty and the frame may not be sent. > > > > What should I use for |bitstream_buffer_id| if there are no available buffers? > You are right. The original FakeVideoEncodeAccelerator::SendDummyFrameForTesting > didn't even pop |available_buffers_| and use id 0 directly. It was a potential > issue. But another problem is cast unittests intent to force a non keyframe > using SendDummyFrameForTesting. > https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/sender/... > In the current patchset, , |key_frame| will be overriden by |first_| and won't > be a non keyframe request. > > We have several options. > 1. Keep the original cast version of SendDummyFrameForTesting and send 0 for > bitstream buffer id. Leave the problem for cast team to fix in the future. > > 2. Remove |first_| completely. SendDummyFrameForTesting can simply call > Encode(NULL, key_frame) here. You need to check with cast team if cast unittests > really require the first encoded frame to be keyframe. > > 3. Skip |first_| here. > void FakeVEA::SendDummyFrameForTesting(bool key_frame) { > DCHECK(available_buffers_.size() > 0); > int32 bitstream_buffer_id = available_buffers_.front().id(); > available_buffers_.pop_front(); > task_runner_->PostTask(FROM_HERE, > base::Bind(&FakeVideoEncodeAccelerator::DoBitstreamBufferReady, > weak_this_factory_.GetWeakPtr(), > bitstream_buffer_id, > kMinimumOutputBufferSize, > key_frame)); > } Selected option 1. Uploaded in patch set 12.
hellner@chromium.org changed reviewers: + mikhal@chromium.org - hclam@chromium.org
-Alpha (on vacation), +mikhal Mikhal, PTAL at media/cast Pawel, ping on content/common/gpu/* sandersd, PTAL at media/*
hellner@chromium.org changed reviewers: + miu@chromium.org - mikhal@chromium.org
mikhal -> miu miu, PTAL at media/cast
Mostly minor things: https://codereview.chromium.org/760963003/diff/240001/media/cast/sender/video... File media/cast/sender/video_sender_unittest.cc (right): https://codereview.chromium.org/760963003/diff/240001/media/cast/sender/video... media/cast/sender/video_sender_unittest.cc:257: const std::vector<uint32>* stored_bitrates_; Please add a " // Owned by |video_sender_|." comment. https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:10: #include "base/message_loop/message_loop_proxy.h" Looks like you don't need this #include. https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:110: bool key_frame = first_ || force_key_frame; Please remove extra space after ||. https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:125: bool key_frame) { Why is this not a const method anymore? https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:26: FakeVideoEncodeAccelerator( style: Need to have the "explicit" attribute for a one-arg ctor. (The old code was wrong to have it for two args.) https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:44: const std::vector<uint32>& stored_bitrates() { This should be a const method. https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:68: bool first_; naming: For readability, how about: next_frame_is_first_frame_?
PTAL https://codereview.chromium.org/760963003/diff/240001/media/cast/sender/video... File media/cast/sender/video_sender_unittest.cc (right): https://codereview.chromium.org/760963003/diff/240001/media/cast/sender/video... media/cast/sender/video_sender_unittest.cc:257: const std::vector<uint32>* stored_bitrates_; On 2014/12/20 03:37:57, miu wrote: > Please add a " // Owned by |video_sender_|." comment. Done. https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.cc (right): https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:10: #include "base/message_loop/message_loop_proxy.h" On 2014/12/20 03:37:57, miu wrote: > Looks like you don't need this #include. Done. https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:110: bool key_frame = first_ || force_key_frame; On 2014/12/20 03:37:57, miu wrote: > Please remove extra space after ||. Good eye! Thanks. https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.cc:125: bool key_frame) { On 2014/12/20 03:37:58, miu wrote: > Why is this not a const method anymore? Probably (mistakenly) happened during refactoring. Thanks! https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... File media/video/fake_video_encode_accelerator.h (right): https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:26: FakeVideoEncodeAccelerator( On 2014/12/20 03:37:58, miu wrote: > style: Need to have the "explicit" attribute for a one-arg ctor. (The old code > was wrong to have it for two args.) Must have forgotten to add it when refactoring. Thanks! https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:44: const std::vector<uint32>& stored_bitrates() { On 2014/12/20 03:37:58, miu wrote: > This should be a const method. Done. https://codereview.chromium.org/760963003/diff/240001/media/video/fake_video_... media/video/fake_video_encode_accelerator.h:68: bool first_; On 2014/12/20 03:37:58, miu wrote: > naming: For readability, how about: next_frame_is_first_frame_? Done.
lgtm https://codereview.chromium.org/760963003/diff/300001/media/cast/sender/video... File media/cast/sender/video_sender_unittest.cc (right): https://codereview.chromium.org/760963003/diff/300001/media/cast/sender/video... media/cast/sender/video_sender_unittest.cc:137: : stored_bitrates_(NULL) { nit: nullptr, not NULL
Ping at posciak: - content/common/gpu/media/video_encode_accelerator_unittest.cc sandersd: - media/media.gyp - media/video/fake_video_encode_accelerator.h/cc
I don't know this API like I know the VDA API, but generally lgtm for style in media/video.
Patchset #17 (id:320001) has been deleted
Patchset #17 (id:340001) has been deleted
On 2015/01/10 01:38:36, sandersd wrote: > I don't know this API like I know the VDA API, but generally lgtm for style in > media/video. sandersd is an owner of content/common/gpu/media/video_encode_accelerator_unittest.cc too so unless there are any objections I will submit tomorrow.
The CQ bit was checked by hellner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/760963003/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hellner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/760963003/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hellner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/760963003/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
"Ok. Could we please reuse the existing mock implementation in cast? I think it should be moved here for consistency." - Pawel I did as you suggested. However, it seems the --gc-sections linker flag is stripping it out for component build (as of patch set 22 and previous). Any ideas as to how to fix this? One solution could be to build the fake encoder for each project that needs it, but that's far from ideal.
The CQ bit was checked by hellner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/760963003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by hellner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/760963003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by hellner@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/760963003/500001
Message was sent while issue was closed.
Committed patchset #24 (id:500001)
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/a15357a6a83685010421056c16d52e94077569cd Cr-Commit-Position: refs/heads/master@{#312893} |
