Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20962003:
ExynosVideoEncodeAccelerator (Closed)
|
Created:
7 years, 4 months ago by sheu Modified:
7 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, feature-media-reviews_chromium.org, Jorge Lucangeli Obes Base URL:
https://chromium.googlesource.com/chromium/src.git@screencast_vea Visibility:
Public. |
DescriptionExynosVideoEncodeAccelerator
This is the media::VideoEncodeAccelerator implementation for the Exynos
platform.
BUG=221441
BUG=260210
TEST=build, run on CrOS snow
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217805
Patch Set 1 #Patch Set 2 : 98e247b0 EXPBUF fix. #
Total comments: 6
Patch Set 3 : 847d2469 media::VideoFrame #Patch Set 4 : a0857c06 Fixes after debugging. #Patch Set 5 : 53779052 Added framerate changing. #
Total comments: 25
Patch Set 6 : 36a90e71 Tighter bitrate control. #Patch Set 7 : 156d3432 Update to match 'final' VEA #Patch Set 8 : 391c4a03 Comments from old EVEA. #Patch Set 9 : a496697c Nits, rebase. #
Total comments: 66
Patch Set 10 : 0d6e96d3 Nits. #
Total comments: 22
Patch Set 11 : 197ac76e Comments, sandbox fix, #
Total comments: 5
Patch Set 12 : 66c93654 Tested. #
Total comments: 2
Patch Set 13 : 8b1dcfaa Using RequestEncodingParametersChange() now for bitrate in Initialize(). #
Messages
Total messages: 27 (0 generated)
https://chromiumcodereview.appspot.com/20962003/diff/3001/content/common/gpu/... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/3001/content/common/gpu/... content/common/gpu/media/exynos_video_encode_accelerator.cc:724: GscInputRecord& input_record = gsc_input_buffer_map_[gsc_buffer]; I think you need: input_record.buffer_ref = input_buffer; here. https://chromiumcodereview.appspot.com/20962003/diff/3001/content/common/gpu/... content/common/gpu/media/exynos_video_encode_accelerator.cc:726: DCHECK(!input_record.buffer_ref.get()); s/!//, unless you wanted to DCHECK this before assigning to input_record.buffer_ref
https://chromiumcodereview.appspot.com/20962003/diff/3001/content/common/gpu/... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/3001/content/common/gpu/... content/common/gpu/media/exynos_video_encode_accelerator.cc:612: while (!mfc_free_input_buffers_.empty() && !encoder_input_queue_.empty()) { Instead of: while (!mfc_free_input_buffers_.empty() && !encoder_input_queue_.empty()) you want to only: while (!mfc_ready_input_buffers_.empty()) encoder_input_queue_ goes into GSC again now.
Updated for media::VideoFrame, PTAL. Untested as of yet locally, actually. https://chromiumcodereview.appspot.com/20962003/diff/3001/content/common/gpu/... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/3001/content/common/gpu/... content/common/gpu/media/exynos_video_encode_accelerator.cc:612: while (!mfc_free_input_buffers_.empty() && !encoder_input_queue_.empty()) { On 2013/08/02 13:54:45, posciak wrote: > Instead of: > while (!mfc_free_input_buffers_.empty() && !encoder_input_queue_.empty()) > > you want to only: > while (!mfc_ready_input_buffers_.empty()) > > encoder_input_queue_ goes into GSC again now. Fixed up already, thanks. https://chromiumcodereview.appspot.com/20962003/diff/3001/content/common/gpu/... content/common/gpu/media/exynos_video_encode_accelerator.cc:724: GscInputRecord& input_record = gsc_input_buffer_map_[gsc_buffer]; On 2013/08/02 11:29:55, posciak wrote: > I think you need: > > input_record.buffer_ref = input_buffer; > > here. Yeah, there was a segfault further down at l.733. Fixed now. https://chromiumcodereview.appspot.com/20962003/diff/3001/content/common/gpu/... content/common/gpu/media/exynos_video_encode_accelerator.cc:726: DCHECK(!input_record.buffer_ref.get()); On 2013/08/02 11:29:55, posciak wrote: > s/!//, unless you wanted to DCHECK this before assigning to > input_record.buffer_ref That was the idea.
This thing _works_ now.
Some comments on ps5, but those are nits, so LGTM. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:248: if (!CreateMfcOutputBuffers()) Do we need to do this here? I'd do it together with CreateMfcInputBuffers() (keeping the right order of course) for clarity. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:313: int32 bitrate, uint32 framerate_num, uint32 framerate_denom) { DCHECK(child_message_loop_proxy_->BelongsToCurrentThread()); https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:378: if (force_keyframe) { Perhaps comment that we can't really control the timing of the keyframe? https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:600: DVLOG(3) << "EnqueueMfc()"; DCHECK_EQ(encoder_thread_.message_loop(), base::MessageLoop::current()); https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:642: DVLOG(3) << "DequeueMfc()"; DCHECK_EQ(encoder_thread_.message_loop(), base::MessageLoop::current()); https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:705: output_buffer_byte_size_ - stream_header_size_ >= output_size) { Ok, this is a huge nit, but this is the second place where I wonder why you make those comparisons read less naturally. Why not: stream_header_size_ + output_size <= output_buffer_byte_size_ ? (In case you are wondering, the first place was: DCHECK_GE(1, gsc_output_buffer_queued_count_); instead of DCHECK_LE(gsc_output_buffer_queued_count_, 1); ) https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:752: qbuf.length = 1; arraysize(qbuf_planes) and everywhere else. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1027: VLOG(3) << "DevicePollTask(): adding GSC to poll() set"; s/VLOG/DVLOG/ https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1102: control.count = 1; arraysize etc. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1122: struct v4l2_control control; You could in fact use S_EXT_CTRLS to save a few ioctls, but oh well. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1300: ctrls[0].id = V4L2_CID_MPEG_VIDEO_B_FRAMES; Would ctrls[] = { { .id = foo, .value = bar }, { ... } ... }; etc work here or gcc only? https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1311: ctrls[5].value = 51; constants? https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:147: // Set/clear the device poll interrupt (using device_ooll_interrupt_fd_). s/ooll/poll/
Perhaps we could make vea.h a part of this CL and submit this?
Updated. fischman@: PTAL https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:248: if (!CreateMfcOutputBuffers()) On 2013/08/09 13:40:02, posciak wrote: > Do we need to do this here? I'd do it together with CreateMfcInputBuffers() > (keeping the right order of course) for clarity. I'm trying to frontload as much init as I can in Initialize(), both so we can fail-early and so we can reduce first frame jank. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:313: int32 bitrate, uint32 framerate_num, uint32 framerate_denom) { On 2013/08/09 13:40:02, posciak wrote: > DCHECK(child_message_loop_proxy_->BelongsToCurrentThread()); Done. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:378: if (force_keyframe) { On 2013/08/09 13:40:02, posciak wrote: > Perhaps comment that we can't really control the timing of the keyframe? We kind of can now, since I put in my dynamic frame config change. I'm cheating here a little - we can get precise config changes by passing a config struct in with the GSC buffer in our tracking state, then applying them when we do the actual encode. As it stands though we don't really have a need to do precise (yet), so I'm gonna stick with this for now. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:600: DVLOG(3) << "EnqueueMfc()"; On 2013/08/09 13:40:02, posciak wrote: > DCHECK_EQ(encoder_thread_.message_loop(), base::MessageLoop::current()); Done. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:642: DVLOG(3) << "DequeueMfc()"; On 2013/08/09 13:40:02, posciak wrote: > DCHECK_EQ(encoder_thread_.message_loop(), base::MessageLoop::current()); Done. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:705: output_buffer_byte_size_ - stream_header_size_ >= output_size) { On 2013/08/09 13:40:02, posciak wrote: > Ok, this is a huge nit, but this is the second place where I wonder why you make > those comparisons read less naturally. Why not: > > stream_header_size_ + output_size <= output_buffer_byte_size_ > ? > > (In case you are wondering, the first place was: > DCHECK_GE(1, gsc_output_buffer_queued_count_); > instead of > DCHECK_LE(gsc_output_buffer_queued_count_, 1); > ) To avoid overflow. The DCHECK_LE business above is actually a valid nit -- I keep getting confused switching between the gtest checks (which expect the expected value as the first argument) and these checks. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:752: qbuf.length = 1; On 2013/08/09 13:40:02, posciak wrote: > arraysize(qbuf_planes) > > and everywhere else. RGB32 has 1 plane, YUV420M has 3. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1027: VLOG(3) << "DevicePollTask(): adding GSC to poll() set"; On 2013/08/09 13:40:02, posciak wrote: > s/VLOG/DVLOG/ Done. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1102: control.count = 1; On 2013/08/09 13:40:02, posciak wrote: > arraysize etc. Done. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1300: ctrls[0].id = V4L2_CID_MPEG_VIDEO_B_FRAMES; On 2013/08/09 13:40:02, posciak wrote: > Would > > ctrls[] = { { .id = foo, .value = bar }, { ... } ... }; etc work here or gcc > only? That's a GCC extension, so I'd avoid it. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1311: ctrls[5].value = 51; On 2013/08/09 13:40:02, posciak wrote: > constants? Normally yes, but I think it would in this case make things even less readable, since these are tied closely with the V4L2 enums. https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/20962003/diff/25001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:147: // Set/clear the device poll interrupt (using device_ooll_interrupt_fd_). On 2013/08/09 13:40:02, posciak wrote: > s/ooll/poll/ Done.
https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:40: return false; \ Most of the VDA stuff returns false XOR notifyerrors, but not both, I think. You sure this is what you want? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:153: output_visible_size_ = converted_visible_size_; WHy is o_v_s_ necessary? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:175: // Open the video converter device. This is the only place you refer to GSC as a "video converter", fwiw. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:187: struct v4l2_capability caps; here and elsewhere is it important to zero out caps before using it? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:251: RequestEncodingParametersChangeTask(initial_bitrate, 30); lol "30" https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:304: if (!shm->Map(buffer.size())) { I guess someday all this Map()ping will make a perf optimizer sad... https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:323: if (framerate == 0) { why check framerate but not bitrate? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:349: // DestroyTask() will cause the encoder_thread_ to flush all tasks. This is pretty opaque. How will it cause this flushing? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:398: IOCTL_OR_ERROR_RETURN(mfc_fd_, VIDIOC_S_EXT_CTRLS, &control); Is this just a vague association between the frame passed to Encode and the request for a keyframe? (might be the best the HW/driver can do, but if so should be called out here). https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:468: // shut it down, in which case we're either in kError state, and we should "either" is left dangling https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:521: // GSC is liable to race conditions if more than one buffer is s/buffer/output buffer/ ? (worth a crbug pointer?) https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:902: 0)); "0" worth explaining? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:989: if ((write(device_poll_interrupt_fd_, &buf, sizeof(buf))) < HANDLE_EINTR? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1061: // touch encoder state from this thread. Would it be worthwhile to indicate which pollfds actually triggered the poll for ServiceDeviceTask, do you think? (probably not) https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1126: parms.parm.output.timeperframe.denominator = framerate; What effect does setting this have on the generated bitstream? (doesn't all timestamping info get carried in the container with h264?) https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1151: IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_S_CTRL, &control); These are all necessary? (GSC doesn't just assume 0 values?) https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1154: control.id = V4L2_CID_GLOBAL_ALPHA; How does this jive with accepting RGBA frames? (whose A might not be 0xFF) https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1176: input_allocated_size_.width(); Shouldn't this be 12 bits per pixel? (i.e. width() * 3/2) (ditto on the other planes) https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1237: converted_allocated_size_.width(); ditto 12bpp here and two lines up, and in SetMfcFormats below. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1321: ctrls[2].value = 10; What is this? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1323: ctrls[3].value = 20480000; What is this? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1327: ctrls[5].value = 51; Where does this come from? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:2: // Use of this source code is governed by a BSD-style license that can be I assume you applied all my comments from https://chromiumcodereview.appspot.com/16693005/ ? https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:2: // Use of this source code is governed by a BSD-style license that can be Please add perf #s to CL description. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:37: ExynosVideoEncodeAccelerator(media::VideoEncodeAccelerator::Client* client); explicit https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:38: ~ExynosVideoEncodeAccelerator(); virtual https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:104: kError, // Error in kEncoding state. Errors can happen in any state, not just kEncoding. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:140: // Set/clear the device poll interrupt (using device_ooll_interrupt_fd_). typo: ooll https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:223: size_t output_buffer_byte_size_; This is only ever kMfcOutputBufferSize so seems silly to have extra var for the future when maybe they differ. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:227: scoped_ptr<uint8[]> stream_header_; optional: personally I prefer to put this sort of thing in a std::string or a vector<uint8> https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:234: // Bitstream buffers ready to be encoded. s/Bitstream buffers/Video frames/ https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:251: int gsc_output_buffer_queued_count_; Is this always equal to gsc_output_buffer_map_.size() - gsc_free_output_buffers_.size() ? If so, is it worth tracking separately? (ditto to the other occurrences of this pattern) https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:286: // The device polling thread handles notifications of V4L2 device changes. TODO to lose this thread, like EVEA has.
https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:40: return false; \ On 2013/08/13 03:25:17, Ami Fischman wrote: > Most of the VDA stuff returns false XOR notifyerrors, but not both, I think. > You sure this is what you want? Yeah. In all cases we want to NotifyError, but in some cases we propagate the error back to the caller. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:153: output_visible_size_ = converted_visible_size_; On 2013/08/13 03:25:17, Ami Fischman wrote: > WHy is o_v_s_ necessary? I could collapse some of these *_size_ members, but I'd rather see them spelled out explicitly here so we know what's resizing to what. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:175: // Open the video converter device. On 2013/08/13 03:25:17, Ami Fischman wrote: > This is the only place you refer to GSC as a "video converter", fwiw. Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:187: struct v4l2_capability caps; On 2013/08/13 03:25:17, Ami Fischman wrote: > here and elsewhere is it important to zero out caps before using it? No, but let's do it for consistency's sake. Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:251: RequestEncodingParametersChangeTask(initial_bitrate, 30); On 2013/08/13 03:25:17, Ami Fischman wrote: > lol "30" Err, fine. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:304: if (!shm->Map(buffer.size())) { On 2013/08/13 03:25:17, Ami Fischman wrote: > I guess someday all this Map()ping will make a perf optimizer sad... When we have buffer allocation and passing plumbed through libjingle and webrtc, we can think about using a static pool of these. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:323: if (framerate == 0) { On 2013/08/13 03:25:17, Ami Fischman wrote: > why check framerate but not bitrate? I'll do that then. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:349: // DestroyTask() will cause the encoder_thread_ to flush all tasks. On 2013/08/13 03:25:17, Ami Fischman wrote: > This is pretty opaque. How will it cause this flushing? DestroyTask() sets encoder state to kError, which causes all tasks to early-out. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:398: IOCTL_OR_ERROR_RETURN(mfc_fd_, VIDIOC_S_EXT_CTRLS, &control); On 2013/08/13 03:25:17, Ami Fischman wrote: > Is this just a vague association between the frame passed to Encode and the > request for a keyframe? > (might be the best the HW/driver can do, but if so should be called out here). From previous CL: I'm cheating here a little - we can get precise config changes by passing a config struct in with the GSC buffer in our tracking state, then applying them when we do the actual encode. As it stands though we don't really have a need to do precise (yet), so I'm gonna stick with this for now. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:468: // shut it down, in which case we're either in kError state, and we should On 2013/08/13 03:25:17, Ami Fischman wrote: > "either" is left dangling Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:521: // GSC is liable to race conditions if more than one buffer is On 2013/08/13 03:25:17, Ami Fischman wrote: > s/buffer/output buffer/ ? > (worth a crbug pointer?) Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:902: 0)); On 2013/08/13 03:25:17, Ami Fischman wrote: > "0" worth explaining? Sure. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:989: if ((write(device_poll_interrupt_fd_, &buf, sizeof(buf))) < On 2013/08/13 03:25:17, Ami Fischman wrote: > HANDLE_EINTR? Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1061: // touch encoder state from this thread. On 2013/08/13 03:25:17, Ami Fischman wrote: > Would it be worthwhile to indicate which pollfds actually triggered the poll for > ServiceDeviceTask, do you think? > (probably not) DevicePollTask posts a ServiceDeviceTask, so there could be some latency before the ServiceDeviceTask actually gets run. It would be better just to run over all the devices in case a device that was not waited on has received an event in the meantime. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1126: parms.parm.output.timeperframe.denominator = framerate; On 2013/08/13 03:25:17, Ami Fischman wrote: > What effect does setting this have on the generated bitstream? > (doesn't all timestamping info get carried in the container with h264?) It's mainly for adjusting the variable bitrate controls. Lower framerate means a higher per-frame bit budget. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1151: IOCTL_OR_ERROR_RETURN_FALSE(gsc_fd_, VIDIOC_S_CTRL, &control); On 2013/08/13 03:25:17, Ami Fischman wrote: > These are all necessary? (GSC doesn't just assume 0 values?) It probably does, but just in case, I'd like to have things explicit here. I've had trouble before. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1154: control.id = V4L2_CID_GLOBAL_ALPHA; On 2013/08/13 03:25:17, Ami Fischman wrote: > How does this jive with accepting RGBA frames? > (whose A might not be 0xFF) We just nuke the A channel. media::VideoFrame doesn't have a separate RGBX format, and GSC doesn't support RGBA anyways. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1176: input_allocated_size_.width(); On 2013/08/13 03:25:17, Ami Fischman wrote: > Shouldn't this be 12 bits per pixel? (i.e. width() * 3/2) > (ditto on the other planes) YUV420M is three-planar YUV. Y plane is 8bpp, U plane is 2bpp (over 2 lines), V plane is 2bpp (over 2 lines). https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1237: converted_allocated_size_.width(); On 2013/08/13 03:25:17, Ami Fischman wrote: > ditto 12bpp here and two lines up, and in SetMfcFormats below. See above. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1321: ctrls[2].value = 10; On 2013/08/13 03:25:17, Ami Fischman wrote: > What is this? Magic value from the Samsung people. It's documented in the V4L2 docs, and since this is sufficiently hardware-specific I'm fine with leaving them as magic values here. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1323: ctrls[3].value = 20480000; On 2013/08/13 03:25:17, Ami Fischman wrote: > What is this? Took it out in favor of the call from RequestEncodingParametersChangeTask(). https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1327: ctrls[5].value = 51; On 2013/08/13 03:25:17, Ami Fischman wrote: > Where does this come from? Magic value from Samsung -- see above. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:2: // Use of this source code is governed by a BSD-style license that can be On 2013/08/13 03:25:17, Ami Fischman wrote: > Please add perf #s to CL description. I went through the old changelist and applied the comments as I thought reasonable. I can go back and tick them if that makes your job easier. No perf numbers right now :-( https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:37: ExynosVideoEncodeAccelerator(media::VideoEncodeAccelerator::Client* client); On 2013/08/13 03:25:17, Ami Fischman wrote: > explicit Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:38: ~ExynosVideoEncodeAccelerator(); On 2013/08/13 03:25:17, Ami Fischman wrote: > virtual Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:104: kError, // Error in kEncoding state. On 2013/08/13 03:25:17, Ami Fischman wrote: > Errors can happen in any state, not just kEncoding. Probably an errant search-replace. Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:140: // Set/clear the device poll interrupt (using device_ooll_interrupt_fd_). On 2013/08/13 03:25:17, Ami Fischman wrote: > typo: ooll Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:223: size_t output_buffer_byte_size_; On 2013/08/13 03:25:17, Ami Fischman wrote: > This is only ever kMfcOutputBufferSize so seems silly to have extra var for the > future when maybe they differ. Maybe a bit redundant but I think it documents itself a little better. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:227: scoped_ptr<uint8[]> stream_header_; On 2013/08/13 03:25:17, Ami Fischman wrote: > optional: personally I prefer to put this sort of thing in a std::string or a > vector<uint8> Personally I think std::string here is a Pretty Bad Idea. vector<uint8> is better but I still think it implies things that I don't need here. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:234: // Bitstream buffers ready to be encoded. On 2013/08/13 03:25:17, Ami Fischman wrote: > s/Bitstream buffers/Video frames/ Done. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:251: int gsc_output_buffer_queued_count_; On 2013/08/13 03:25:17, Ami Fischman wrote: > Is this always equal to > gsc_output_buffer_map_.size() - gsc_free_output_buffers_.size() > ? > If so, is it worth tracking separately? > > (ditto to the other occurrences of this pattern) For some of these it is. For example, MFC inputs might be in the GSC->MFC queue, so it would be both no queued to the hardware and not in its free buffers queue. If nothing else it's useful for debugging, so I'd like to leave it. https://chromiumcodereview.appspot.com/20962003/diff/50001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:286: // The device polling thread handles notifications of V4L2 device changes. On 2013/08/13 03:25:17, Ami Fischman wrote: > TODO to lose this thread, like EVEA has. Done.
https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:253: RequestEncodingParametersChangeTask(initial_bitrate, kInitialFramerate); Where is this "kInitialFramerate" defined?
Mostly comment and code structure comments. https://codereview.chromium.org/20962003/diff/50001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_encode_accelerator.h (right): https://codereview.chromium.org/20962003/diff/50001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.h:2: // Use of this source code is governed by a BSD-style license that can be On 2013/08/13 09:06:25, sheu wrote: > On 2013/08/13 03:25:17, Ami Fischman wrote: > > Please add perf #s to CL description. > > I went through the old changelist and applied the comments as I thought > reasonable. I can go back and tick them if that makes your job easier. No need for that. > No perf numbers right now :-( :( https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:354: // DestroyTask() will cause the encoder_thread_ to flush all tasks. s/flush all tasks/turn all tasks into no-ops as state will be kError/ https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:403: IOCTL_OR_ERROR_RETURN(mfc_fd_, VIDIOC_S_EXT_CTRLS, &control); On 2013/08/13 09:06:25, sheu wrote: > On 2013/08/13 03:25:17, Ami Fischman wrote: > > Is this just a vague association between the frame passed to Encode and the > > request for a keyframe? > > (might be the best the HW/driver can do, but if so should be called out here). > > From previous CL: > > I'm cheating here a little - we can get precise config changes by passing a > config struct in with the GSC buffer in our tracking state, then applying them > when we do the actual encode. As it stands though we don't really have a need > to do precise (yet), so I'm gonna stick with this for now. Please comment to this effect here. https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:435: // Set our state to kError. Just in case. This isn't "just in case" because you promise it in a comment above ("flush all tasks"). This comment should reflect that this is done to no-op out any pending tasks. https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:471: // * device_poll_thread_ is running normally This is now a single-bullet list headed by "either". Did you mean to have an asterisk bullet on the next line, or what? https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:528: // simultaneously enqueued, so enqueue just one. crbug for the GSC race conditions? https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:1162: control.id = V4L2_CID_GLOBAL_ALPHA; I wonder if it's worth asserting that A==0xFF somewhere for RGBA inputs. Maybe only in !NDEBUG. Or maybe not. https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:1188: input_allocated_size_.width() / 2; On 2013/08/13 09:06:25, sheu wrote: > On 2013/08/13 03:25:17, Ami Fischman wrote: > > Shouldn't this be 12 bits per pixel? (i.e. width() * 3/2) > > (ditto on the other planes) > > YUV420M is three-planar YUV. D'oh! Of course. https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:1318: struct v4l2_ext_control ctrls[6]; You might enjoy a form more like: struct v4l2_ext_control ctrls[] = { { V4L2_CID_MPEG_VIDEO_B_FRAMES, 0 }, { V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 1 }, ... }; which would - allow you to drop the "6" here - allow you to drop the explicit subscripting below - drop a bunch of boilerplate - avoid the need for the memset. Of course, it might also not actually work, so there's that. https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:1321: memset(&control, 0, sizeof(control)); Please comment of the magical values that they are received wisdom. https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_encode_accelerator.h (right): https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.h:288: // TODO(sheu): replace this thread with an TYPE_IO decoder_thread_. s/decoder/encoder/ https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://codereview.chromium.org/20962003/diff/68001/content/common/gpu/media/... content/common/gpu/media/gpu_video_encode_accelerator.cc:160: media::VideoFrame::I420, What guarantees this? IOW I think you need to make Initialize error out if input_frame is not I420. (at that point, you can also delete the input_frame_ member)
Sandbox fix to go with this. Yay for my image-build scripts that automatically add "--no-sandbox" to my startup line. https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:253: RequestEncodingParametersChangeTask(initial_bitrate, kInitialFramerate); On 2013/08/13 17:11:20, hshi1 wrote: > Where is this "kInitialFramerate" defined? Uhh.. it actually went into exynos_video_*decode*_accelerator.h. Fixed. https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:354: // DestroyTask() will cause the encoder_thread_ to flush all tasks. On 2013/08/13 18:13:19, Ami Fischman wrote: > s/flush all tasks/turn all tasks into no-ops as state will be kError/ Done. https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:403: IOCTL_OR_ERROR_RETURN(mfc_fd_, VIDIOC_S_EXT_CTRLS, &control); On 2013/08/13 18:13:19, Ami Fischman wrote: > On 2013/08/13 09:06:25, sheu wrote: > > On 2013/08/13 03:25:17, Ami Fischman wrote: > > > Is this just a vague association between the frame passed to Encode and the > > > request for a keyframe? > > > (might be the best the HW/driver can do, but if so should be called out > here). > > > > From previous CL: > > > > I'm cheating here a little - we can get precise config changes by passing a > > config struct in with the GSC buffer in our tracking state, then applying them > > when we do the actual encode. As it stands though we don't really have a need > > to do precise (yet), so I'm gonna stick with this for now. > > Please comment to this effect here. Done. https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:435: // Set our state to kError. Just in case. On 2013/08/13 18:13:19, Ami Fischman wrote: > This isn't "just in case" because you promise it in a comment above ("flush all > tasks"). This comment should reflect that this is done to no-op out any pending > tasks. Done. https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:471: // * device_poll_thread_ is running normally On 2013/08/13 18:13:19, Ami Fischman wrote: > This is now a single-bullet list headed by "either". Did you mean to have an > asterisk bullet on the next line, or what? A linebreak sneaked in somehow. Fixed. https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1162: control.id = V4L2_CID_GLOBAL_ALPHA; On 2013/08/13 18:13:19, Ami Fischman wrote: > I wonder if it's worth asserting that A==0xFF somewhere for RGBA inputs. > Maybe only in !NDEBUG. Or maybe not. I'm gonna say, not really worth it. We're converting to NV12 to input to the encoder anyways, and (1) NV12 has no alpha and (2) since when did H264 have chroma keying? https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1318: struct v4l2_ext_control ctrls[6]; On 2013/08/13 18:13:19, Ami Fischman wrote: > You might enjoy a form more like: > > struct v4l2_ext_control ctrls[] = { > { V4L2_CID_MPEG_VIDEO_B_FRAMES, 0 }, > { V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 1 }, > ... > }; > > which would > - allow you to drop the "6" here > - allow you to drop the explicit subscripting below > - drop a bunch of boilerplate > - avoid the need for the memset. > > Of course, it might also not actually work, so there's that. There are more members to v4l2_ext_control than just those, though. And initialize-by-member-name in a struct brace initializer is a compiler extension. https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.cc:1321: memset(&control, 0, sizeof(control)); On 2013/08/13 18:13:19, Ami Fischman wrote: > Please comment of the magical values that they are received wisdom. Done. https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... File content/common/gpu/media/exynos_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/exynos_video_encode_accelerator.h:288: // TODO(sheu): replace this thread with an TYPE_IO decoder_thread_. On 2013/08/13 18:13:19, Ami Fischman wrote: > s/decoder/encoder/ Done. https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/68001/content/common/gpu... content/common/gpu/media/gpu_video_encode_accelerator.cc:160: media::VideoFrame::I420, On 2013/08/13 18:13:19, Ami Fischman wrote: > What guarantees this? IOW I think you need to make Initialize error out if > input_frame is not I420. > (at that point, you can also delete the input_frame_ member) This should be input_format_. The specific VEA implementation will handle erroring on unsupported formats. Done.
Sandbox change. jln@: PTAL at content/common/sandbox_*.
https://codereview.chromium.org/20962003/diff/65001/content/common/sandbox_se... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://codereview.chromium.org/20962003/diff/65001/content/common/sandbox_se... content/common/sandbox_seccomp_bpf_linux.cc:1808: read_whitelist->push_back(kDevMfcEncPath); Did you mean write_whitelist->push_back(kDevMfcEncPath); ?
https://chromiumcodereview.appspot.com/20962003/diff/65001/content/common/san... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://chromiumcodereview.appspot.com/20962003/diff/65001/content/common/san... content/common/sandbox_seccomp_bpf_linux.cc:1798: static const char kDevMfcEncPath[] = "/dev/mfc-enc"; Is there no way that this device could be opened, once, prior to engaging the sandbox? Same question for kDevMfcDecPath[]. Since the code to use this seems to be in Chromium, hopefully it could be "Pre warmed" ? See WarmUpSandbox() in gpu_main.cc. We really need to remove complexity from the sandbox and instead have sandbox-friendly code. https://chromiumcodereview.appspot.com/20962003/diff/65001/content/common/san... content/common/sandbox_seccomp_bpf_linux.cc:1808: read_whitelist->push_back(kDevMfcEncPath); On 2013/08/14 01:21:35, hshi1 wrote: > Did you mean write_whitelist->push_back(kDevMfcEncPath); ? If this works fine without write access, please remove this line entirely, but that seems very surprising.
I haven't been able to see connection flake with this build. fischman@: PTAL hshi@: I should have your comments addressed. jln@: addressed, PTAL. https://codereview.chromium.org/20962003/diff/65001/content/common/sandbox_se... File content/common/sandbox_seccomp_bpf_linux.cc (right): https://codereview.chromium.org/20962003/diff/65001/content/common/sandbox_se... content/common/sandbox_seccomp_bpf_linux.cc:1798: static const char kDevMfcEncPath[] = "/dev/mfc-enc"; On 2013/08/14 04:30:17, Julien Tinnes wrote: > Is there no way that this device could be opened, once, prior to engaging the > sandbox? > > Same question for kDevMfcDecPath[]. Since the code to use this seems to be in > Chromium, hopefully it could be "Pre warmed" ? > > See WarmUpSandbox() in gpu_main.cc. > > We really need to remove complexity from the sandbox and instead have > sandbox-friendly code. Yep. Every time you open the device you get another encoder/decoder context. We need a new context for each encode/decode. https://codereview.chromium.org/20962003/diff/65001/content/common/sandbox_se... content/common/sandbox_seccomp_bpf_linux.cc:1808: read_whitelist->push_back(kDevMfcEncPath); On 2013/08/14 04:30:17, Julien Tinnes wrote: > On 2013/08/14 01:21:35, hshi1 wrote: > > Did you mean write_whitelist->push_back(kDevMfcEncPath); ? > > If this works fine without write access, please remove this line entirely, but > that seems very surprising. Lulz. I forgot to actually remove the --no-sandbox flag when testing. Tested now.
LGTM thanks! Works on my setup.
LGTM if you can add the DCHECK below back. https://codereview.chromium.org/20962003/diff/86001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://codereview.chromium.org/20962003/diff/86001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:1113: DVLOG(3) << "RequestEncodingParametersChangeTask(): bitrate=" << bitrate Why isn't this DCHECKing running on the encoder thread? And why is Initialize calling this (*Task) version instead of trampolining using REPC()?
https://codereview.chromium.org/20962003/diff/86001/content/common/gpu/media/... File content/common/gpu/media/exynos_video_encode_accelerator.cc (right): https://codereview.chromium.org/20962003/diff/86001/content/common/gpu/media/... content/common/gpu/media/exynos_video_encode_accelerator.cc:1113: DVLOG(3) << "RequestEncodingParametersChangeTask(): bitrate=" << bitrate On 2013/08/14 19:12:55, Ami Fischman wrote: > Why isn't this DCHECKing running on the encoder thread? > And why is Initialize calling this (*Task) version instead of trampolining using > REPC()? I took out the DCHECK precisely because we weren't trampoling. And _that's_ happening since on Initialize(), we still not not threading yet and we can afford to do this directly without swingin' on the threads, just like with the rest of the setup in Initialize. I'll trampoline this if you really want, but it seems to me like an unnecessary round-trip.
On Wed, Aug 14, 2013 at 12:18 PM, <sheu@chromium.org> wrote: > https://codereview.chromium.**org/20962003/diff/86001/** > content/common/gpu/media/**exynos_video_encode_**accelerator.cc<https://codereview.chromium.org/20962003/diff/86001/content/common/gpu/media/exynos_video_encode_accelerator.cc> > File content/common/gpu/media/**exynos_video_encode_**accelerator.cc > (right): > > https://codereview.chromium.**org/20962003/diff/86001/** > content/common/gpu/media/**exynos_video_encode_** > accelerator.cc#newcode1113<https://codereview.chromium.org/20962003/diff/86001/content/common/gpu/media/exynos_video_encode_accelerator.cc#newcode1113> > content/common/gpu/media/**exynos_video_encode_**accelerator.cc:1113: > DVLOG(3) << "**RequestEncodingParametersChang**eTask(): bitrate=" << > bitrate > On 2013/08/14 19:12:55, Ami Fischman wrote: > >> Why isn't this DCHECKing running on the encoder thread? >> And why is Initialize calling this (*Task) version instead of >> > trampolining using > >> REPC()? >> > > I took out the DCHECK precisely because we weren't trampoling. And > _that's_ happening since on Initialize(), we still not not threading yet > and we can afford to do this directly without swingin' on the threads, > just like with the rest of the setup in Initialize. > > I'll trampoline this if you really want, but it seems to me like an > unnecessary round-trip. > The trip is not round. If you post the task in Initialize then you don't have to wait for it to get processed since you're guaranteed it'll be processed before any encode calls. I think the safety makes this worthwhile. > https://codereview.chromium.**org/20962003/<https://codereview.chromium.org/2... >
Done.
piman@: just you for content/content_common.gypi
content/common/sandbox_seccomp_bpf_linux.cc lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/20962003/97001
Message was sent while issue was closed.
Change committed as 217805 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
