Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1125263005:
MJPEG acceleration for V4L2 (Closed)
|
Created:
5 years, 7 months ago by henryhsu Modified:
5 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, rickyz+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org, jln+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMJPEG acceleration for V4L2
cpu governor: performance
jpeg clock: 200 MHz
software decode: latency 10.42ms, cpu 18.85%
hardware decode: latency 5.67ms, cpu 13.98%
hardware decode with output memcpy: latency 12.57ms, cpu 20.62%
We are fixing driver to support IOMMU to avoid memcpy overhead.
BUG=426383
TEST=run unittest on peach_pi, apprtc website over night.
Committed: https://crrev.com/840b38edfdb6bb8c7c82e6f207bf2d7f6f505249
Cr-Commit-Position: refs/heads/master@{#337990}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Add unittest and address review comments #
Total comments: 38
Patch Set 3 : address kcwu's comments #
Total comments: 21
Patch Set 4 : address review comments of patch set 3 #
Total comments: 167
Patch Set 5 : address comments of patch set 4 #
Total comments: 2
Patch Set 6 : rebase #
Total comments: 66
Patch Set 7 : address review comments of patch set 6 #
Total comments: 19
Patch Set 8 : address comments of patch set 7 #
Total comments: 6
Patch Set 9 : #
Total comments: 26
Patch Set 10 : rebase #Patch Set 11 : #
Total comments: 20
Patch Set 12 : address comments #
Total comments: 4
Patch Set 13 : ioctl return -1 id #
Total comments: 11
Patch Set 14 : check dqbuf.flags #
Total comments: 13
Patch Set 15 : #
Total comments: 8
Patch Set 16 : #Patch Set 17 : #
Total comments: 1
Patch Set 18 : fix nit #Messages
Total messages: 55 (7 generated)
wuchengli@chromium.org changed reviewers: + wuchengli@chromium.org
https://codereview.chromium.org/1125263005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1125263005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:26: #elif defined(OS_CHROMEOS) && defined(USE_V4L2_CODEC) Can we do this? #if defined(OS_CHROMEOS) #if defined(ARCH_CPU_X86_FAMILY) || defined(USE_V4L2_CODEC) return true; #endif #endif https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/gp... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:21: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) #if defined(OS_CHROMEOS) #if defined(ARCH_CPU_X86_FAMILY) #elif defined(USE_V4L2_CODEC) #endif #endif https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:93: if (!child_message_loop_proxy_->BelongsToCurrentThread()) { Don't use this pattern. See piman's comment in https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va.... https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:117: const __u32 kCapsRequired = V4L2_CAP_VIDEO_CAPTURE | We should only need V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING. Fix the driver. V4L2_CAP_VIDEO_CAPTURE and V4L2_CAP_VIDEO_OUTPUT are legacy. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:456: // TODO(posciak): Fix this to be non-Exynos specific. This is a new driver. We have a chance not to have any workaround in the code. Please fix the driver if it's a bug. https://codereview.chromium.org/1125263005/diff/1/content/content_common.gypi File content/content_common.gypi (right): https://codereview.chromium.org/1125263005/diff/1/content/content_common.gypi... content/content_common.gypi:863: 'common/gpu/media/v4l2_jpeg_decode_accelerator.cc', Add this to BUILD.gn.
posciak@chromium.org changed reviewers: + posciak@chromium.org
https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:456: // TODO(posciak): Fix this to be non-Exynos specific. On 2015/05/25 10:29:23, wuchengli wrote: > This is a new driver. We have a chance not to have any workaround in the code. > Please fix the driver if it's a bug. As far as I know this only applies to GSC, I don't think JPEG has the same issue.
kcwu@chromium.org changed reviewers: + kcwu@chromium.org
https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:112: client_ = client_ptr_factory_->GetWeakPtr(); No need to use weak pointer for |client|. |client| must outlive than |this| according to JDA interface. And in practice, |this| is owned by |client|. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:131: // StartDevicePoll will NotifyError on failure, so IgnoreResult is fine here. Nobody use the return value of StartDevicePoll. How about make it return void? https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:445: if (!device_->SetDevicePollInterrupt()) { Why do we need to SetDevicePollInterrupt? Assume there is no decode pending now and one new input comming. The code flow will be: 1. main thread Decode, post DecodeTask to device thread 2. device thread DecodeTask, call Enqueue and SetDevicePollInterrupt here. 3. the poll thread got interrupted and post ServiceDeviceTask to device thread 4. device thread ServiceDeviceTask call Dequeue & Enqueue. But this is sooner than device decoding finished, Dequeue got nothing. Then it posts DevicePollTask to poll thread. 5. poll thread wait device decoding finished. Then post ServiceDeviceTask to device thread again. 6. this time ServiceDeviceTask call Dequeue and got output. step 3 & 4 is actually doing nothing. They could be saved without SetDevicePollInterrupt. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:37: FrameReadyCB; unused https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:46: const scoped_refptr<media::VideoFrame>& video_frame); override; https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:176: } } // namespace content
Please ignore TIME_ENTER and TIME_LEAVE in v4l2_jpeg_decode_accelerator.cc. These two macros are used for measuring latency and these will be removed eventually. https://codereview.chromium.org/1125263005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1125263005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/gpu_jpeg_decoder.cc:26: #elif defined(OS_CHROMEOS) && defined(USE_V4L2_CODEC) On 2015/05/25 10:29:23, wuchengli wrote: > Can we do this? > #if defined(OS_CHROMEOS) > #if defined(ARCH_CPU_X86_FAMILY) || defined(USE_V4L2_CODEC) > return true; > #endif > #endif Because veyron and nyan series don't support JPEG HW decode yet, I prefer to keep original method. ARM platform will be false in this CL. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/gp... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/gp... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:21: #if defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) On 2015/05/25 10:29:23, wuchengli wrote: > #if defined(OS_CHROMEOS) > #if defined(ARCH_CPU_X86_FAMILY) > #elif defined(USE_V4L2_CODEC) > #endif > #endif Done. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:93: if (!child_message_loop_proxy_->BelongsToCurrentThread()) { On 2015/05/25 10:29:23, wuchengli wrote: > Don't use this pattern. See piman's comment in > https://codereview.chromium.org/1124423008/diff/1/content/common/gpu/media/va.... > Done. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:112: client_ = client_ptr_factory_->GetWeakPtr(); On 2015/05/26 10:47:23, kcwu wrote: > No need to use weak pointer for |client|. |client| must outlive than |this| > according to JDA interface. And in practice, |this| is owned by |client|. Done. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:117: const __u32 kCapsRequired = V4L2_CAP_VIDEO_CAPTURE | On 2015/05/25 10:29:23, wuchengli wrote: > We should only need V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING. Fix the > driver. V4L2_CAP_VIDEO_CAPTURE and V4L2_CAP_VIDEO_OUTPUT are legacy. s5p-jpeg doesn't support multiple planar. I think it should be V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:131: // StartDevicePoll will NotifyError on failure, so IgnoreResult is fine here. On 2015/05/26 10:47:22, kcwu wrote: > Nobody use the return value of StartDevicePoll. How about make it return void? Done. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:445: if (!device_->SetDevicePollInterrupt()) { On 2015/05/26 10:47:23, kcwu wrote: > Why do we need to SetDevicePollInterrupt? > Assume there is no decode pending now and one new input comming. The code flow > will be: > 1. main thread Decode, post DecodeTask to device thread > 2. device thread DecodeTask, call Enqueue and SetDevicePollInterrupt here. > 3. the poll thread got interrupted and post ServiceDeviceTask to device thread > 4. device thread ServiceDeviceTask call Dequeue & Enqueue. But this is sooner > than device decoding finished, Dequeue got nothing. > Then it posts DevicePollTask to poll thread. > 5. poll thread wait device decoding finished. Then post ServiceDeviceTask to > device thread again. > 6. this time ServiceDeviceTask call Dequeue and got output. > > step 3 & 4 is actually doing nothing. They could be saved without > SetDevicePollInterrupt. As discussed before, I removed this to save one trip for one frame. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:456: // TODO(posciak): Fix this to be non-Exynos specific. On 2015/05/25 10:29:23, wuchengli wrote: > This is a new driver. We have a chance not to have any workaround in the code. > Please fix the driver if it's a bug. Done. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:37: FrameReadyCB; On 2015/05/26 10:47:23, kcwu wrote: > unused Done. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:46: const scoped_refptr<media::VideoFrame>& video_frame); On 2015/05/26 10:47:23, kcwu wrote: > override; Done. https://codereview.chromium.org/1125263005/diff/1/content/common/gpu/media/v4... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:176: } On 2015/05/26 10:47:23, kcwu wrote: > } // namespace content Done. https://codereview.chromium.org/1125263005/diff/1/content/content_common.gypi File content/content_common.gypi (right): https://codereview.chromium.org/1125263005/diff/1/content/content_common.gypi... content/content_common.gypi:863: 'common/gpu/media/v4l2_jpeg_decode_accelerator.cc', On 2015/05/25 10:29:23, wuchengli wrote: > Add this to BUILD.gn. Done.
https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... File content/common/gpu/media/jpeg_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:48: explicit TestImageFile(base::FilePath::StringType file_name) const ... & https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:72: JpegClient(std::vector<TestImageFile*>* test_image_files, const ... & https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:92: std::vector<TestImageFile*>* test_image_files_; const ... & https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:266: uint8 *yplane, *uplane, *vplane; Please declare these variables and use them together. I mean, uint8* yplane = ...; uint8* uplane = ...; uint8* vplane = ...; int yplane_stride = ...; int uv_plane_stride = ...; https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:269: yplane = (uint8 *)sw_out_shm_->memory(); use c++ casting https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:306: void ReadTestData(base::FilePath::StringType data, const ... & https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:309: std::vector<TestImageFile*> test_image_files_; how about ScopedVector? https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:323: for (size_t i = 0; i < test_image_files_.size(); i++) { STLDeleteElements https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:329: std::vector<TestImageFile*>* test_image_files) { indent looks strange? https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:354: // - Use software deocder or not. s/deocder/decoder/ https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:358: public ::testing::WithParamInterface< base::Tuple<bool, ClientState> > { s/< ... >/<...>/ https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:368: ClientStateNotification<ClientState>* note = scoped_ptr https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:370: JpegClient* client = new JpegClient( scoped_ptr<JpegClient> https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:402: ::testing::Values(base::MakeTuple(false, CS_DECODE_FAIL))); I don't understand the point of this test. IIUC, what it actually did is checking whether the output of hw decode is not zero. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:21: #define DVLOG VLOG don't forget to remove this https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:423: void V4L2JpegDecodeAccelerator::DevicePollTask(bool poll_device) { poll_device unused https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:459: bool poll_device = actually unused https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:657: // Enqueue a poll task with no devices to poll on - will wait only for the revise the comment https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:32: // Note: Initialize() and Destroy() are synchronous. no Destroy()
PTAL https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... File content/common/gpu/media/jpeg_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:48: explicit TestImageFile(base::FilePath::StringType file_name) On 2015/06/08 10:04:28, kcwu wrote: > const ... & Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:72: JpegClient(std::vector<TestImageFile*>* test_image_files, On 2015/06/08 10:04:28, kcwu wrote: > const ... & Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:92: std::vector<TestImageFile*>* test_image_files_; On 2015/06/08 10:04:28, kcwu wrote: > const ... & Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:266: uint8 *yplane, *uplane, *vplane; On 2015/06/08 10:04:28, kcwu wrote: > Please declare these variables and use them together. > I mean, > > uint8* yplane = ...; > uint8* uplane = ...; > uint8* vplane = ...; > int yplane_stride = ...; > int uv_plane_stride = ...; Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:269: yplane = (uint8 *)sw_out_shm_->memory(); On 2015/06/08 10:04:28, kcwu wrote: > use c++ casting Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:306: void ReadTestData(base::FilePath::StringType data, On 2015/06/08 10:04:28, kcwu wrote: > const ... & Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:309: std::vector<TestImageFile*> test_image_files_; On 2015/06/08 10:04:28, kcwu wrote: > how about ScopedVector? Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:323: for (size_t i = 0; i < test_image_files_.size(); i++) { On 2015/06/08 10:04:28, kcwu wrote: > STLDeleteElements not used. ScopedVector will handle this. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:329: std::vector<TestImageFile*>* test_image_files) { On 2015/06/08 10:04:28, kcwu wrote: > indent looks strange? Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:354: // - Use software deocder or not. On 2015/06/08 10:04:28, kcwu wrote: > s/deocder/decoder/ Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:358: public ::testing::WithParamInterface< base::Tuple<bool, ClientState> > { On 2015/06/08 10:04:28, kcwu wrote: > s/< ... >/<...>/ Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:368: ClientStateNotification<ClientState>* note = On 2015/06/08 10:04:28, kcwu wrote: > scoped_ptr Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:370: JpegClient* client = new JpegClient( On 2015/06/08 10:04:28, kcwu wrote: > scoped_ptr<JpegClient> Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:402: ::testing::Values(base::MakeTuple(false, CS_DECODE_FAIL))); On 2015/06/08 10:04:29, kcwu wrote: > I don't understand the point of this test. > IIUC, what it actually did is checking whether the output of hw decode is not > zero. Removed https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:21: #define DVLOG VLOG On 2015/06/08 10:04:29, kcwu wrote: > don't forget to remove this Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:423: void V4L2JpegDecodeAccelerator::DevicePollTask(bool poll_device) { On 2015/06/08 10:04:29, kcwu wrote: > poll_device unused Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:459: bool poll_device = On 2015/06/08 10:04:29, kcwu wrote: > actually unused Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:657: // Enqueue a poll task with no devices to poll on - will wait only for the On 2015/06/08 10:04:29, kcwu wrote: > revise the comment Done. https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/20001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:32: // Note: Initialize() and Destroy() are synchronous. On 2015/06/08 10:04:29, kcwu wrote: > no Destroy() Done.
media/test/data/README also needs update for the new jpeg file. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... File content/common/gpu/media/jpeg_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:105: }; DISALLOW_COPY_AND_ASSIGN(JpegClient); https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:264: uint8 *yplane = reinterpret_cast<uint8*>(sw_out_shm_->memory()); "uint8* foo" instead of "uint8 *foo". https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:359: ScopedVector<ClientStateNotification<ClientState> > notes; s/> >/>>/ https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:651: // Start a poll task and will wait only for the poll interrupt. We always poll with poll_device=true now, so it not only wait for poll interrupt but also device.
https://codereview.chromium.org/1125263005/diff/40001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Update the performance number in the change description. https://codereview.chromium.org/1125263005/diff/40001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:27: return true; Make this a separate CL. Do not enable it directly. If something goes wrong, the entire CL won't be get reverted. Also, we cannot enable this for all ARM devices. Please prepare a CL to properly query the capabilities via V4L2 API and put it in GpuInfo. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... File content/common/gpu/media/jpeg_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Please make JDA unittest a separate CL. This file is big enough. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... File content/common/gpu/media/tegra_v4l2_device.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/tegra_v4l2_device.cc:155: case kImageProcessor: Just change this to default.
I'll continue the review tomorrow. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:32: IOCTL_OR_ERROR_RETURN_VALUE(type, arg, false) |type| will be incorrectly expanded. Fix this by the following code. #define IOCTL_OR_ERROR_RETURN_FALSE(type, arg) \ IOCTL_OR_ERROR_RETURN_VALUE(type, arg, false, #type) #define IOCTL_OR_ERROR_RETURN_VALUE(type, type_name, arg, value) \ ... PLOG(ERROR) << __func__ << "(): ioctl() failed: " << type_name; \ https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:40: #define TIME_ENTER() \ Remove or use TRACE_EVENT0. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:43: #define TIME_LEAVE() \ Remove all TIME_LEAVE. Expand a macro to a scoped variable can do both TIME_ENTER and TIME_LEAVE. See autolock or TRACE_EVENT0.
all done https://codereview.chromium.org/1125263005/diff/40001/content/browser/rendere... File content/browser/renderer_host/gpu_jpeg_decoder.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/browser/rendere... content/browser/renderer_host/gpu_jpeg_decoder.cc:27: return true; On 2015/06/11 11:08:54, wuchengli wrote: > Make this a separate CL. Do not enable it directly. If something goes wrong, the > entire CL won't be get reverted. > > Also, we cannot enable this for all ARM devices. Please prepare a CL to properly > query the capabilities via V4L2 API and put it in GpuInfo. Done. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... File content/common/gpu/media/jpeg_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/06/11 11:08:54, wuchengli wrote: > Please make JDA unittest a separate CL. This file is big enough. Done. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:105: }; On 2015/06/09 12:17:44, kcwu wrote: > DISALLOW_COPY_AND_ASSIGN(JpegClient); Done. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:264: uint8 *yplane = reinterpret_cast<uint8*>(sw_out_shm_->memory()); On 2015/06/09 12:17:44, kcwu wrote: > "uint8* foo" instead of "uint8 *foo". Done. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/jpeg_decode_accelerator_unittest.cc:359: ScopedVector<ClientStateNotification<ClientState> > notes; On 2015/06/09 12:17:44, kcwu wrote: > s/> >/>>/ Done. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... File content/common/gpu/media/tegra_v4l2_device.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/tegra_v4l2_device.cc:155: case kImageProcessor: On 2015/06/11 11:08:54, wuchengli wrote: > Just change this to default. Done. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:32: IOCTL_OR_ERROR_RETURN_VALUE(type, arg, false) On 2015/06/11 11:19:46, wuchengli wrote: > |type| will be incorrectly expanded. Fix this by the following code. > > #define IOCTL_OR_ERROR_RETURN_FALSE(type, arg) \ > IOCTL_OR_ERROR_RETURN_VALUE(type, arg, false, #type) > > #define IOCTL_OR_ERROR_RETURN_VALUE(type, type_name, arg, value) \ > ... > PLOG(ERROR) << __func__ << "(): ioctl() failed: " << type_name; \ Done. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:40: #define TIME_ENTER() \ On 2015/06/11 11:19:46, wuchengli wrote: > Remove or use TRACE_EVENT0. Done. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:43: #define TIME_LEAVE() \ On 2015/06/11 11:19:46, wuchengli wrote: > Remove all TIME_LEAVE. Expand a macro to a scoped variable can do both > TIME_ENTER and TIME_LEAVE. See autolock or TRACE_EVENT0. Done. https://codereview.chromium.org/1125263005/diff/40001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:651: // Start a poll task and will wait only for the poll interrupt. On 2015/06/09 12:17:44, kcwu wrote: > We always poll with poll_device=true now, so it not only wait for poll interrupt > but also device. Removed this from here
lgtm nits https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:68: : reset_buffer_flag_(false), s/false/0/ https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:124: device_weak_, bitstream_buffer_id, error)); s/device_weak_/device_weak_factory_.GetWeakPtr()/ https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:110: // Set to true when input or output buffer have to re-allocate. s/true/non-zero/ https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:165: base::WeakPtr<V4L2JpegDecodeAccelerator> device_weak_; Do we need to keep this? Just call device_weak_factory_.GetWeakPtr() instead.
I haven't finished the review. I'll continue next week. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:5: #include <fcntl.h> What's this for? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:12: #include "base/bind.h" do we need this? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:14: #include "base/callback.h" do we need this? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:85: // If the device thread is running, destroy using posted task. s/device/decoder/ https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:90: // Wait for tasks to finish/early-exit. Where is the code related to early-exit? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:93: DCHECK(!decoder_thread_.IsRunning()); Remove. The code above ensures this. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:101: device_weak_factory_.InvalidateWeakPtrs(); The invalidation and dereference of weak pointers have to be on the same thread. Move this to ~V4L2JpegDecodeAccelerator (line 84). https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:120: Error error) { combine with the previous line? Does git cl format do this? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:130: client_ = client; Set client_ after other initialization succeeds (after line 145). https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:137: LOG(ERROR) << "Initialize(): ioctl() failed: VIDIOC_QUERYCAP" Remove "ioctl() failed: ". Ioctl actually succeeds. It's capabilities check failed. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:185: bool V4L2JpegDecodeAccelerator::CheckBufferAttributes() { This is a bad name. This function does more than checking attributes. Need refactor here. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:199: if (job_record->frame->format() != output_format_ || This can be removed because DCHECK of line 162 means the format shouldn't change. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:454: while (!input_queue_.empty() && !free_input_buffers_.empty()) { Can we check the resolution change here? If the size has changed, do not enqueue it. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:458: if (old_inputs_queued == 0 && input_buffer_queued_count_ != 0) { Why do we need old_inputs_queued == 0 check? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:540: DVLOG(3) << "Processing finished, returning frame, ts=" s/Processing/Decoding/ https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:543: DCHECK(client_); This isn't very useful. If this DCHECK doesn't exist, the code just crashes in the next line. The effect is almost the same. Remove. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:546: ResetBuffers(); Move this out of this function. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:625: if (!device_->SetDevicePollInterrupt()) VDA notifies error here. VEA and V4L2imageProcessor don't. I checked with Pawel. SetDevicePollInterrupt is too simple to fail. Let's notify error here. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:647: if (!keep_input_queue) { Remove |keep_input_queue| and move the code out of this function. while (!input_queue_.empty()) input_queue_.pop(); https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:653: running_jobs_.pop(); When the resolution changes, shouldn't we wait until all |running_jobs_| finishes? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:32: // Note: Initialize() are synchronous. What do you mean it's synchronous? It posts a StartDevicePoll task. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:45: bool at_device; Explain what this means. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:54: bool at_device; Explain what this means. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:86: bool CheckBufferAttributes(); Add documentation for this. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:91: void ResetBuffers(); Add documentation for this. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:95: void DestroyTask(); Add documentation for this. I think DestroyTask shouldn't be grouped together with NotifyError and NotifyErrorFromDecoderThread. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:100: // Attempt to start/stop device_poll_thread_. s/Attempt to// https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:107: media::VideoFrame::Format output_format_; Add a blank line if there's comment. Same for all other variables. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:108: // Record current image size for checking image size is changed or not. // Current image size used for checking if the size is changed. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:111: uint32_t reset_buffer_flag_; Two booleans is simpler than a bit mask. Just use |recreate_input_buffers_pending_| and |recreate_output_buffers_pending_|. "reset" is not clear about whether the buffers are destroyed. "recreate" should be more clear. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:125: // Thread to communicate with the device on. I don't understand. Maybe you mean "Thread to communicate with the device"? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:134: // All the below members are to be accessed from decoder_thread_ only s/to be// https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:136: std::queue<linked_ptr<JobRecord> > input_queue_; The names of |input_queue_| and |running_jobs_| should be consistent. s/input_queue_/input_jobs_/. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:145: // Mapping of int index to an input buffer record. This is not a map. Update the comment. s/input_buffer_map_/input_buffers/ https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:152: // Output buffers ready to use; LIFO since we don't care about ordering. Explain what the int stands for. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:154: // Mapping of int index to an output buffer record. This is not a map. s/output_buffer_map_/output_buffers_/. Update the comment. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:158: base::WeakPtrFactory<V4L2JpegDecodeAccelerator> device_weak_factory_; s/device_weak_factory_/weak_factory_/. This is the ptr factory for itself. Calling it "device" is strange. It's common to call it |weak_facotry_|. https://code.google.com/p/chromium/codesearch#search/&q=file:%5C.h%20WeakPtrF... https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:165: base::WeakPtr<V4L2JpegDecodeAccelerator> device_weak_; s/device_weak_/weak_this_/
https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:143: LOG(ERROR) << "Initialize(): encoder thread failed to start"; s/encoder/decoder/ https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:177: if (!reset_buffer_flag_) { This is always set to false in ResetBuffers. Remove. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:185: bool V4L2JpegDecodeAccelerator::CheckBufferAttributes() { On 2015/06/12 11:07:36, wuchengli wrote: > This is a bad name. This function does more than checking attributes. Need > refactor here. CreateBuffersIfNecessary should be a better name. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:409: // touch encoder state from this thread. s/encoder/decoder/ https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:546: ResetBuffers(); On 2015/06/12 11:07:36, wuchengli wrote: > Move this out of this function. Do we need to do this in Dequeue or ServiceDeviceTask? It seems it's enough to just do it in DecodeTask.
https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:43: V4L2JpegDecodeAccelerator::InputRecord::InputRecord() : at_device(false) { Initialize |address| and |length|. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:78: device_weak_factory_(this) { Initialize output_format_(0), client_(nullptr) https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:79: device_weak_ = device_weak_factory_.GetWeakPtr(); This can be done in constructor member initializer list. Right? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:85: // If the device thread is running, destroy using posted task. On 2015/06/12 11:07:36, wuchengli wrote: > s/device/decoder/ This comment doesn't provide too much information. I think this can be removed. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:90: // Wait for tasks to finish/early-exit. On 2015/06/12 11:07:36, wuchengli wrote: > Where is the code related to early-exit? This comment doesn't provide too much information. I think this can be removed. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:121: DCHECK(decoder_task_runner_->BelongsToCurrentThread()); This function can be called from poll thread. Change the function name to PostNotifyError? https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:164: scoped_ptr<JobRecord> job_record( We don't need to use scoped_ptr. Copying BitstreamBuffer is cheap and JobRecord::frame is a scoped_refptr. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:199: if (job_record->frame->format() != output_format_ || On 2015/06/12 11:07:36, wuchengli wrote: > This can be removed because DCHECK of line 162 means the format shouldn't > change. |output_format_| can also be removed. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:211: reset_buffer_flag_ = reset_input_buffer | reset_output_buffer; As we discussed, suppose there are two resolution changes in |input_queue_|. The first resolution change is not done yet. The second resolution change may overwrite the value of |reset_buffer_flag_|. So the check of |reset_input_buffer| and |reset_output_buffer| should be done just before Enqueue to avoid this. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:214: image_coded_size_ = job_record->frame->coded_size(); Set |image_coded_size_| at the end of CreateOutputBuffers to make sure output buffers and |image_coded_size_| will always match. Same for line 246. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:263: size_t reserve_size = job_record->bitstream_buffer.size() * 2; Add a simple comment to explain why we multiple by 2. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:324: format.fmt.pix.pixelformat = output_format_fourcc_; Just use V4L2_PIX_FMT_YUV420 here. Remove output_format_fourcc_. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:404: NotifyError(-1, media::JpegDecodeAccelerator::PLATFORM_FAILURE); NotifyErrorFromDecoderThread https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:500: NotifyError(dqbuf.index, media::JpegDecodeAccelerator::PLATFORM_FAILURE); NotifyErrorFromDecoderThread https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:523: NotifyError(dqbuf.index, media::JpegDecodeAccelerator::PLATFORM_FAILURE); NotifyErrorFromDecoderThread https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:579: input_buffer_queued_count_++; You can see we always manipulate |free_input_buffers_| and |input_buffer_queued_count_| together. We can use a function to get input_buffer_queued_count_. We can maintain one less variable. int input_buffer_queued_count() { return input_buffer_map_.size() - free_input_buffers_(); } Same for |output_buffer_queued_count_|. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:610: // Start up the device poll thread and schedule its first DevicePollTask(). There is no "schedule its first DevicePollTask" here. Remove this comment. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:137: std::queue<linked_ptr<JobRecord> > running_jobs_; I don't think you need to use linked_ptr for |input_queue_| and |running_jobs_|. It's cheap to copy BitstreamBuffer and media::VideoFrame is scoped_refptr.
https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:302: if (device.get()) { Nit: I think just if (device) should work. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:101: device_weak_factory_.InvalidateWeakPtrs(); On 2015/06/12 11:07:36, wuchengli wrote: > The invalidation and dereference of weak pointers have to be on the same thread. > Move this to ~V4L2JpegDecodeAccelerator (line 84). > > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... Good catch. In V4L2ImageProcessor, the class this class is based on, the factory is called device_weak_factory, because it's used from the device thread to produce pointers for VideoFrame destruction callback. Which is later on dereferenced on device_thread as well. Here the weak pointer is used for something else, for producing and dereferencing on child thread for use in NotifyError. So this is a different use case and needs to be fixed (and the name of the factory changed as well). https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:121: DCHECK(decoder_task_runner_->BelongsToCurrentThread()); On 2015/06/15 07:55:28, wuchengli wrote: > This function can be called from poll thread. Change the function name to > PostNotifyError? I don't see this method called from anywhere in this CL... NotifyError is called from all threads though... https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:186: DVLOG(3) << __func__; What happens if a DecodeTask() that requires buffer change is called before the device finishes processing previous buffers/jobs? Won't we streamoff the queues, cancelling in-flight tasks before they are finished? I think you'll need to pend the buffer set change and handle it after all in-flight jobs are finished. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:273: format.fmt.pix.bytesperline = 0; Not needed (memset above). https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:279: reqbufs.type = V4L2_BUF_TYPE_VIDEO_OUTPUT; Could we use _MPLANE API? https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:317: uint32_t output_format_fourcc_ = V4L2_PIX_FMT_YUV420; Could we have a check that VideoFrame format is YUV420 as well? https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:537: memcpy(job_record->frame->data(media::VideoFrame::kYPlane), Please use libyuv:: copy methods to account for strides. Also, please do this before requeueing output buffer back and reinitializing things, just to make it better understandable/readable. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:563: if (!shm->Map(job_record->bitstream_buffer.size())) { Could we do this earlier? We could map before getting a free input buffer, immediately when we get this on Decode, potentially squeezing out a bit more parallelism. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:610: // Start up the device poll thread and schedule its first DevicePollTask(). On 2015/06/15 07:55:27, wuchengli wrote: > There is no "schedule its first DevicePollTask" here. Remove this comment. Why do we not schedule it from here though? https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:32: // Note: Initialize() are synchronous. On 2015/06/12 11:07:37, wuchengli wrote: > What do you mean it's synchronous? It posts a StartDevicePoll task. I believe this is because returning true from here means we have successfully initialized, as opposed to having an IPC NotifyInitializeDone in the old VDA/VEA model. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:33: bool Initialize(Client* client) override; Nit: // media::JpegDecodeAccelerator implementation. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:49: struct OutputRecord { InputRecord and OutputRecord are identical. Could we use a common class instead? https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:73: kInputBufferCount = 2, We always want to have inputcount == outputcount, so I'd just have one const here. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:111: uint32_t reset_buffer_flag_; On 2015/06/12 11:07:37, wuchengli wrote: > Two booleans is simpler than a bit mask. Just use > |recreate_input_buffers_pending_| and |recreate_output_buffers_pending_|. > "reset" is not clear about whether the buffers are destroyed. "recreate" should > be more clear. +1 to this. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:125: // Thread to communicate with the device on. On 2015/06/12 11:07:37, wuchengli wrote: > I don't understand. Maybe you mean "Thread to communicate with the device"? This sentence is grammatically correct actually. :) https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:136: std::queue<linked_ptr<JobRecord> > input_queue_; On 2015/06/12 11:07:37, wuchengli wrote: > The names of |input_queue_| and |running_jobs_| should be consistent. > s/input_queue_/input_jobs_/. How about job_input_queue_ as a compromise ? :) https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:137: std::queue<linked_ptr<JobRecord> > running_jobs_; On 2015/06/15 07:55:28, wuchengli wrote: > I don't think you need to use linked_ptr for |input_queue_| and |running_jobs_|. > It's cheap to copy BitstreamBuffer and media::VideoFrame is scoped_refptr. JobRecord is base::Bound, so we'd have to make it derive from RefCounted otherwise. Personally I feel this is simpler and saves some copying as a bonus. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:145: // Mapping of int index to an input buffer record. On 2015/06/12 11:07:37, wuchengli wrote: > This is not a map. Update the comment. s/input_buffer_map_/input_buffers/ We use it as a map though. The position in the vector maps v4l2_buffer.index -> InputRecord, but we can use a vector instead of std::map, because we know they are always 0..n. So we can do input_buffer_map[v4l2_buffer.index]. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:154: // Mapping of int index to an output buffer record. On 2015/06/12 11:07:37, wuchengli wrote: > This is not a map. s/output_buffer_map_/output_buffers_/. Update the comment. Same as input_buffer_map_. https://chromiumcodereview.appspot.com/1125263005/diff/60001/content/common/g... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:158: base::WeakPtrFactory<V4L2JpegDecodeAccelerator> device_weak_factory_; The weak this factory must be the last member of the class.
Patchset #5 (id:80001) has been deleted
all done. PTAL https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/gpu_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/gpu_jpeg_decode_accelerator.cc:302: if (device.get()) { On 2015/06/16 07:13:21, Pawel Osciak wrote: > Nit: I think just if (device) should work. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:5: #include <fcntl.h> On 2015/06/12 11:07:36, wuchengli wrote: > What's this for? removed. I also removed poll.h, sys/eventfd.h, sys/ioctl.h. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:12: #include "base/bind.h" On 2015/06/12 11:07:36, wuchengli wrote: > do we need this? We use base::Bind. We can remove bind_helpers.h and callback.h. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:14: #include "base/callback.h" On 2015/06/12 11:07:36, wuchengli wrote: > do we need this? Removed. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:43: V4L2JpegDecodeAccelerator::InputRecord::InputRecord() : at_device(false) { On 2015/06/15 07:55:27, wuchengli wrote: > Initialize |address| and |length|. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:68: : reset_buffer_flag_(false), On 2015/06/12 06:59:42, kcwu wrote: > s/false/0/ Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:78: device_weak_factory_(this) { On 2015/06/15 07:55:27, wuchengli wrote: > Initialize output_format_(0), client_(nullptr) Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:79: device_weak_ = device_weak_factory_.GetWeakPtr(); On 2015/06/15 07:55:27, wuchengli wrote: > This can be done in constructor member initializer list. Right? Not used anymore. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:85: // If the device thread is running, destroy using posted task. On 2015/06/12 11:07:36, wuchengli wrote: > s/device/decoder/ Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:85: // If the device thread is running, destroy using posted task. On 2015/06/15 07:55:27, wuchengli wrote: > On 2015/06/12 11:07:36, wuchengli wrote: > > s/device/decoder/ > This comment doesn't provide too much information. I think this can be removed. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:90: // Wait for tasks to finish/early-exit. On 2015/06/15 07:55:28, wuchengli wrote: > On 2015/06/12 11:07:36, wuchengli wrote: > > Where is the code related to early-exit? > This comment doesn't provide too much information. I think this can be removed. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:90: // Wait for tasks to finish/early-exit. On 2015/06/12 11:07:36, wuchengli wrote: > Where is the code related to early-exit? Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:93: DCHECK(!decoder_thread_.IsRunning()); On 2015/06/12 11:07:36, wuchengli wrote: > Remove. The code above ensures this. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:101: device_weak_factory_.InvalidateWeakPtrs(); On 2015/06/12 11:07:36, wuchengli wrote: > The invalidation and dereference of weak pointers have to be on the same thread. > Move this to ~V4L2JpegDecodeAccelerator (line 84). > > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:120: Error error) { On 2015/06/12 11:07:36, wuchengli wrote: > combine with the previous line? Does git cl format do this? Yes. git cl format did this. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:121: DCHECK(decoder_task_runner_->BelongsToCurrentThread()); On 2015/06/15 07:55:28, wuchengli wrote: > This function can be called from poll thread. Change the function name to > PostNotifyError? Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:121: DCHECK(decoder_task_runner_->BelongsToCurrentThread()); On 2015/06/16 07:13:21, Pawel Osciak wrote: > On 2015/06/15 07:55:28, wuchengli wrote: > > This function can be called from poll thread. Change the function name to > > PostNotifyError? > > I don't see this method called from anywhere in this CL... NotifyError is called > from all threads though... Fixed https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:124: device_weak_, bitstream_buffer_id, error)); On 2015/06/12 06:59:42, kcwu wrote: > s/device_weak_/device_weak_factory_.GetWeakPtr()/ Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:130: client_ = client; On 2015/06/12 11:07:36, wuchengli wrote: > Set client_ after other initialization succeeds (after line 145). Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:137: LOG(ERROR) << "Initialize(): ioctl() failed: VIDIOC_QUERYCAP" On 2015/06/12 11:07:36, wuchengli wrote: > Remove "ioctl() failed: ". Ioctl actually succeeds. It's capabilities check > failed. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:143: LOG(ERROR) << "Initialize(): encoder thread failed to start"; On 2015/06/15 05:58:37, wuchengli wrote: > s/encoder/decoder/ Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:164: scoped_ptr<JobRecord> job_record( On 2015/06/15 07:55:27, wuchengli wrote: > We don't need to use scoped_ptr. Copying BitstreamBuffer is cheap and > JobRecord::frame is a scoped_refptr. As discussed. keep scoped_ptr here. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:177: if (!reset_buffer_flag_) { On 2015/06/15 05:58:37, wuchengli wrote: > This is always set to false in ResetBuffers. Remove. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:185: bool V4L2JpegDecodeAccelerator::CheckBufferAttributes() { On 2015/06/15 05:58:37, wuchengli wrote: > On 2015/06/12 11:07:36, wuchengli wrote: > > This is a bad name. This function does more than checking attributes. Need > > refactor here. > CreateBuffersIfNecessary should be a better name. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:185: bool V4L2JpegDecodeAccelerator::CheckBufferAttributes() { On 2015/06/12 11:07:36, wuchengli wrote: > This is a bad name. This function does more than checking attributes. Need > refactor here. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:186: DVLOG(3) << __func__; On 2015/06/16 07:13:21, Pawel Osciak wrote: > What happens if a DecodeTask() that requires buffer change is called before the > device finishes processing previous buffers/jobs? Won't we streamoff the queues, > cancelling in-flight tasks before they are finished? > > I think you'll need to pend the buffer set change and handle it after all > in-flight jobs are finished. We will wait all in-flight jobs are finished. And then streamoff, recreate buffers, streamon, and enqueue buffer. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:199: if (job_record->frame->format() != output_format_ || On 2015/06/12 11:07:36, wuchengli wrote: > This can be removed because DCHECK of line 162 means the format shouldn't > change. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:199: if (job_record->frame->format() != output_format_ || On 2015/06/15 07:55:27, wuchengli wrote: > On 2015/06/12 11:07:36, wuchengli wrote: > > This can be removed because DCHECK of line 162 means the format shouldn't > > change. > |output_format_| can also be removed. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:211: reset_buffer_flag_ = reset_input_buffer | reset_output_buffer; On 2015/06/15 07:55:28, wuchengli wrote: > As we discussed, suppose there are two resolution changes in |input_queue_|. The > first resolution change is not done yet. The second resolution change may > overwrite the value of |reset_buffer_flag_|. So the check of > |reset_input_buffer| and |reset_output_buffer| should be done just before > Enqueue to avoid this. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:214: image_coded_size_ = job_record->frame->coded_size(); On 2015/06/15 07:55:28, wuchengli wrote: > Set |image_coded_size_| at the end of CreateOutputBuffers to make sure output > buffers and |image_coded_size_| will always match. Same for line 246. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:263: size_t reserve_size = job_record->bitstream_buffer.size() * 2; On 2015/06/15 07:55:28, wuchengli wrote: > Add a simple comment to explain why we multiple by 2. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:273: format.fmt.pix.bytesperline = 0; On 2015/06/16 07:13:21, Pawel Osciak wrote: > Not needed (memset above). Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:279: reqbufs.type = V4L2_BUF_TYPE_VIDEO_OUTPUT; On 2015/06/16 07:13:21, Pawel Osciak wrote: > Could we use _MPLANE API? s5p-jpeg doesn't support MPLANE. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:317: uint32_t output_format_fourcc_ = V4L2_PIX_FMT_YUV420; On 2015/06/16 07:13:21, Pawel Osciak wrote: > Could we have a check that VideoFrame format is YUV420 as well? We have it in Decode function. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:324: format.fmt.pix.pixelformat = output_format_fourcc_; On 2015/06/15 07:55:27, wuchengli wrote: > Just use V4L2_PIX_FMT_YUV420 here. Remove output_format_fourcc_. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:404: NotifyError(-1, media::JpegDecodeAccelerator::PLATFORM_FAILURE); On 2015/06/15 07:55:27, wuchengli wrote: > NotifyErrorFromDecoderThread Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:409: // touch encoder state from this thread. On 2015/06/15 05:58:37, wuchengli wrote: > s/encoder/decoder/ Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:454: while (!input_queue_.empty() && !free_input_buffers_.empty()) { On 2015/06/12 11:07:36, wuchengli wrote: > Can we check the resolution change here? If the size has changed, do not enqueue > it. I check resolution change before Enqueue() function. I'd like to keep Enqueue function simply and do enqueue directly. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:458: if (old_inputs_queued == 0 && input_buffer_queued_count_ != 0) { On 2015/06/12 11:07:36, wuchengli wrote: > Why do we need old_inputs_queued == 0 check? Removed. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:500: NotifyError(dqbuf.index, media::JpegDecodeAccelerator::PLATFORM_FAILURE); On 2015/06/15 07:55:27, wuchengli wrote: > NotifyErrorFromDecoderThread Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:523: NotifyError(dqbuf.index, media::JpegDecodeAccelerator::PLATFORM_FAILURE); On 2015/06/15 07:55:28, wuchengli wrote: > NotifyErrorFromDecoderThread Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:537: memcpy(job_record->frame->data(media::VideoFrame::kYPlane), On 2015/06/16 07:13:21, Pawel Osciak wrote: > Please use libyuv:: copy methods to account for strides. > Also, please do this before requeueing output buffer back and reinitializing > things, just to make it better understandable/readable. output_record.length is calculated from VideoFrame::AllocationSize. I think it is fine. I moved ResetBuffers to ServiceDeviceTask. So this is before requeueing output buffer and reinitialization. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:540: DVLOG(3) << "Processing finished, returning frame, ts=" On 2015/06/12 11:07:36, wuchengli wrote: > s/Processing/Decoding/ Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:543: DCHECK(client_); On 2015/06/12 11:07:36, wuchengli wrote: > This isn't very useful. If this DCHECK doesn't exist, the code just crashes in > the next line. The effect is almost the same. Remove. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:546: ResetBuffers(); On 2015/06/12 11:07:36, wuchengli wrote: > Move this out of this function. Moved to ServiceDeviceTask. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:546: ResetBuffers(); On 2015/06/15 05:58:37, wuchengli wrote: > On 2015/06/12 11:07:36, wuchengli wrote: > > Move this out of this function. > Do we need to do this in Dequeue or ServiceDeviceTask? It seems it's enough to > just do it in DecodeTask. DecodeTask only handles queue empty case. We still need this and I move this section to ServiceDeviceTask. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:563: if (!shm->Map(job_record->bitstream_buffer.size())) { On 2015/06/16 07:13:21, Pawel Osciak wrote: > Could we do this earlier? We could map before getting a free input buffer, > immediately when we get this on Decode, potentially squeezing out a bit more > parallelism. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:579: input_buffer_queued_count_++; On 2015/06/15 07:55:27, wuchengli wrote: > You can see we always manipulate |free_input_buffers_| and > |input_buffer_queued_count_| together. We can use a function to get > input_buffer_queued_count_. We can maintain one less variable. > int input_buffer_queued_count() { > return input_buffer_map_.size() - free_input_buffers_(); > } > > Same for |output_buffer_queued_count_|. We use input_buffer_queued_count to check queue empty in many places. I'd prefer to use two more variables to save one subtraction for each time. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:610: // Start up the device poll thread and schedule its first DevicePollTask(). On 2015/06/15 07:55:27, wuchengli wrote: > There is no "schedule its first DevicePollTask" here. Remove this comment. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:610: // Start up the device poll thread and schedule its first DevicePollTask(). On 2015/06/16 07:13:21, Pawel Osciak wrote: > On 2015/06/15 07:55:27, wuchengli wrote: > > There is no "schedule its first DevicePollTask" here. Remove this comment. > > Why do we not schedule it from here though? We only schedule DevicePollTask when queue has buffers. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:625: if (!device_->SetDevicePollInterrupt()) On 2015/06/12 11:07:36, wuchengli wrote: > VDA notifies error here. VEA and V4L2imageProcessor don't. I checked with Pawel. > SetDevicePollInterrupt is too simple to fail. Let's notify error here. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:647: if (!keep_input_queue) { On 2015/06/12 11:07:36, wuchengli wrote: > Remove |keep_input_queue| and move the code out of this function. > while (!input_queue_.empty()) input_queue_.pop(); Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:653: running_jobs_.pop(); On 2015/06/12 11:07:36, wuchengli wrote: > When the resolution changes, shouldn't we wait until all |running_jobs_| > finishes? Sure. For resolution change case, running_jobs should be empty. But for DestroyTask case, we just discard all jobs. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:32: // Note: Initialize() are synchronous. On 2015/06/12 11:07:37, wuchengli wrote: > What do you mean it's synchronous? It posts a StartDevicePoll task. Modified comments. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:33: bool Initialize(Client* client) override; On 2015/06/16 07:13:22, Pawel Osciak wrote: > Nit: > // media::JpegDecodeAccelerator implementation. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:45: bool at_device; On 2015/06/12 11:07:37, wuchengli wrote: > Explain what this means. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:49: struct OutputRecord { On 2015/06/16 07:13:22, Pawel Osciak wrote: > InputRecord and OutputRecord are identical. Could we use a common class instead? Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:54: bool at_device; On 2015/06/12 11:07:37, wuchengli wrote: > Explain what this means. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:73: kInputBufferCount = 2, On 2015/06/16 07:13:22, Pawel Osciak wrote: > We always want to have inputcount == outputcount, so I'd just have one const > here. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:86: bool CheckBufferAttributes(); On 2015/06/12 11:07:37, wuchengli wrote: > Add documentation for this. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:91: void ResetBuffers(); On 2015/06/12 11:07:37, wuchengli wrote: > Add documentation for this. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:95: void DestroyTask(); On 2015/06/12 11:07:37, wuchengli wrote: > Add documentation for this. > > I think DestroyTask shouldn't be grouped together with NotifyError and > NotifyErrorFromDecoderThread. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:100: // Attempt to start/stop device_poll_thread_. On 2015/06/12 11:07:37, wuchengli wrote: > s/Attempt to// Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:107: media::VideoFrame::Format output_format_; On 2015/06/12 11:07:37, wuchengli wrote: > Add a blank line if there's comment. Same for all other variables. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:108: // Record current image size for checking image size is changed or not. On 2015/06/12 11:07:37, wuchengli wrote: > // Current image size used for checking if the size is changed. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:110: // Set to true when input or output buffer have to re-allocate. On 2015/06/12 06:59:42, kcwu wrote: > s/true/non-zero/ Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:111: uint32_t reset_buffer_flag_; On 2015/06/12 11:07:37, wuchengli wrote: > Two booleans is simpler than a bit mask. Just use > |recreate_input_buffers_pending_| and |recreate_output_buffers_pending_|. > "reset" is not clear about whether the buffers are destroyed. "recreate" should > be more clear. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:125: // Thread to communicate with the device on. On 2015/06/12 11:07:37, wuchengli wrote: > I don't understand. Maybe you mean "Thread to communicate with the device"? Yes. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:134: // All the below members are to be accessed from decoder_thread_ only On 2015/06/12 11:07:37, wuchengli wrote: > s/to be// Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:136: std::queue<linked_ptr<JobRecord> > input_queue_; On 2015/06/12 11:07:37, wuchengli wrote: > The names of |input_queue_| and |running_jobs_| should be consistent. > s/input_queue_/input_jobs_/. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:137: std::queue<linked_ptr<JobRecord> > running_jobs_; On 2015/06/15 07:55:28, wuchengli wrote: > I don't think you need to use linked_ptr for |input_queue_| and |running_jobs_|. > It's cheap to copy BitstreamBuffer and media::VideoFrame is scoped_refptr. As discussed, I prefer to keep linked_ptr here because BitstreamBuffer size depends on base::SharedMemoryHandle class. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:145: // Mapping of int index to an input buffer record. On 2015/06/12 11:07:37, wuchengli wrote: > This is not a map. Update the comment. s/input_buffer_map_/input_buffers/ As Pawel said, I prefer to keep original name. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:152: // Output buffers ready to use; LIFO since we don't care about ordering. On 2015/06/12 11:07:37, wuchengli wrote: > Explain what the int stands for. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:154: // Mapping of int index to an output buffer record. On 2015/06/12 11:07:37, wuchengli wrote: > This is not a map. s/output_buffer_map_/output_buffers_/. Update the comment. same as input_buffer_map_. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:158: base::WeakPtrFactory<V4L2JpegDecodeAccelerator> device_weak_factory_; On 2015/06/12 11:07:37, wuchengli wrote: > s/device_weak_factory_/weak_factory_/. This is the ptr factory for itself. > Calling it "device" is strange. It's common to call it |weak_facotry_|. > > https://code.google.com/p/chromium/codesearch#search/&q=file:%5C.h%20WeakPtrF... Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:158: base::WeakPtrFactory<V4L2JpegDecodeAccelerator> device_weak_factory_; On 2015/06/16 07:13:22, Pawel Osciak wrote: > The weak this factory must be the last member of the class. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:165: base::WeakPtr<V4L2JpegDecodeAccelerator> device_weak_; On 2015/06/12 06:59:42, kcwu wrote: > Do we need to keep this? Just call device_weak_factory_.GetWeakPtr() instead. Done. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:165: base::WeakPtr<V4L2JpegDecodeAccelerator> device_weak_; On 2015/06/12 11:07:37, wuchengli wrote: > s/device_weak_/weak_this_/ deleted.
https://codereview.chromium.org/1125263005/diff/100001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/100001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:433: if (!recreate_input_buffers_pending_ && !recreate_output_buffers_pending_) { This check can be removed since we always call CreateBufferIfNecessary before Enqueue to make sure that current frame has correct input/output buffer size. I'll update it in next patch. https://codereview.chromium.org/1125263005/diff/100001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/100001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:80: void DecodeTask(scoped_ptr<JobRecord> job_record); I missed it. I'll add documentation for this in next patch.
https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:279: reqbufs.type = V4L2_BUF_TYPE_VIDEO_OUTPUT; On 2015/06/16 09:37:22, henryhsu wrote: > On 2015/06/16 07:13:21, Pawel Osciak wrote: > > Could we use _MPLANE API? > > s5p-jpeg doesn't support MPLANE. Pawel. Do we need MPLANE? The input is JPEG and doesn't need MPLANE. We use one shared memory for one output frame. So using MPLANE means memcpy? I think single plane is ok. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:579: input_buffer_queued_count_++; On 2015/06/16 09:37:23, henryhsu wrote: > On 2015/06/15 07:55:27, wuchengli wrote: > > You can see we always manipulate |free_input_buffers_| and > > |input_buffer_queued_count_| together. We can use a function to get > > input_buffer_queued_count_. We can maintain one less variable. > > int input_buffer_queued_count() { > > return input_buffer_map_.size() - free_input_buffers_(); > > } > > > > Same for |output_buffer_queued_count_|. > > We use input_buffer_queued_count to check queue empty in many places. I'd prefer > to use two more variables to save one subtraction for each time. The time saving is very small. The point is it's easy to forget to change them at the same time. It's also harder to see if the code is correct. For example, in line 355 at the end of DestroyInputBuffers, do we need to set input_buffer_queued_count_ = 0 after REQBUFS(0)? Are we relying input_buffer_queued_count_ = 0 before DestroyInputBuffers is called? https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:96: DCHECK(client_); This doesn't give any more information. The code has the same effect of crashing in the next line without DCHECK. Remove. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:168: (input_buffer_queued_count_ == 0 && output_buffer_queued_count_ == 0)) { Can we just check (input_buffer_queued_count_ == 0 && output_buffer_queued_count_ == 0)? If both streams are off, queued count should also be 0. Also, these checks should go to ShouldRecreateInputBuffers() and ShouldRecreateOutputBuffers(). See my comment in line 178. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:170: Enqueue(); Shouldn't we allow the input buffer to be enqueued as long as no buffer recreation is pending? I think Enqueue() should be split into two functions -- EnqueueInput and EnqueueOutput. CreateBufferIfNecessary(); if (!recreate_input_buffers_pending_ && !recreate_output_buffers_pending_) EnqueueInput(); EnqueueOutput(); https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:171: device_poll_task_runner_->PostTask( if (input_buffer_queued_count_ > 0 || output_buffer_queued_count_ > 0) { PostTask() } The code here should be the same as ServiceDeviceTask. Can we remove line 164-175 and just call ServiceDeviceTask() here? https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:178: void V4L2JpegDecodeAccelerator::CheckBufferAttributes() { This name is not good. "Check" is vaque. It's hard to know what this is without looking at the code of the function. Break this into two functions. recreate_input_buffers_pending_ = ShouldRecreateInputBuffers(); recreate_output_buffers_pending_ = ShouldRecreateOutputBuffers(); https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:201: bool V4L2JpegDecodeAccelerator::CreateBufferIfNecessary() { s/CreateBufferIfNecessary/CreateBuffersIfNecessary/ https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:208: We should return if input queued count or output queued count is not 0. We need to prevent the buffer from re-creating when not all output buffers have been dequeued? For example: - JDA gets a DecodeTask. One input buffer and one output buffer are queued. - JDA gets a DecodeTask with a different coded size. It is added to input_jobs_. - ServiceDeviceTask is called. One input buffer is dequeued. If I understand correctly, the buffers will be re-created even though the output buffer is not dequeued yet. if (input_buffer_queued_count_ != 0 || output_buffer_queued_output_ != 0) return true; https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:211: LOG(ERROR) << "Stop device poll thread failed when renew buffers."; s/renew/recreating/ https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:222: LOG(ERROR) << "Create Input buffer failed."; s/Input buffer/input buffers/ https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:226: LOG(ERROR) << "Create Output buffer failed."; s/Output buffer/output buffers/ https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:327: PLOG(ERROR) << "CreateOutputBuffers(): mmap() failed"; Please examine all LOG(ERROR) in this file and make sure it notifies an error (and only one) to the client. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:355: free_input_buffers_.clear(); input_buffer_queued_count_ = 0; https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:375: free_output_buffers_.clear(); output_buffer_queued_count_ = 0; https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:405: if (!input_jobs_.empty() && CreateBufferIfNecessary()) Why we shouldn't call enqueue if input_jobs_ is empty? What if Dequeue() DQBUF an output buffer and we need to QBUF it in Enqueue()? Also, move input_jobs_.empty() check to CreateBufferIfNecessary(). All the logic to decide if creating buffers is necessary should go there. Should the code be like this? if (CreateBufferIfNecessary()) return; if (!recreate_input_buffers_pending_ && !recreate_output_buffers_pending_) EnqueueInput(); EnqueueOutput(); https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:412: if (input_buffer_queued_count_ > 0 && output_buffer_queued_count_ > 0) { s/&&/||/. If any buffer is queued, we need to poll. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:437: if (input_buffer_queued_count_ != 0 && !input_streamon_) { Why can't we always start streaming if |input_streamon_|==false? Why do we need to wait until at least one buffer is queued? https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:517: << job_record->frame->timestamp().InMilliseconds(); timestamp is set to zero in gpu_jpeg_decode_accelerator.cc. No point of printing it. Right? Remove. Same for line 548. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:606: input_streamon_ = false; Move this line inside if (input_streamon_) https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:612: output_streamon_ = false; Move this line inside if (output_streamon_) https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:615: running_jobs_.pop(); When StopDevicePoll is called in CreateBufferIfNecessary, |running_jobs_| should be already empty. Also, |input_jobs_| is not emptied here. For consistency, move line 614-615 to line 85 in DestroyTask(). https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:623: input_buffer_queued_count_ = 0; Move this line to line 604 inside "if (input_streamon_)". https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:631: output_buffer_queued_count_ = 0; Move this line to line 610 inside "if (output_streamon_)" https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:45: // Set true during QBUF and DQBUF. |adress| will be accessed by hardware. s/adress/address/ https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:59: media::BitstreamBuffer bitstream_buffer; Document these three variables. Also mention |bitstream_buffer| is input and |frame| is output. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:60: scoped_refptr<media::VideoFrame> frame; s/frame/output_frame/ so it's more obvious what this is. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:61: scoped_ptr<base::SharedMemory> shm; Document this is mapped from |bitstream_buffer|. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:75: bool CreateBufferIfNecessary(); Document the return value -- False means an error has occurred. Otherwise, people will think true means buffers have been created. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:97: const size_t kBufferCount = 2; Document this is the number of input buffers and output buffers. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:102: // Set true when input or output buffers have to re-allocate. s/re-allocate/be re-allocated/ https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:127: // All the below members are accessed from |decoder_thread_| only All the below members except |weak_factory_| https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:152: // Weak factory for producing weak pointers on the decoder_thread_ s/decoder/child/
PTAL. https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/60001/content/common/gpu/medi... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:579: input_buffer_queued_count_++; On 2015/06/22 22:40:53, wuchengli wrote: > On 2015/06/16 09:37:23, henryhsu wrote: > > On 2015/06/15 07:55:27, wuchengli wrote: > > > You can see we always manipulate |free_input_buffers_| and > > > |input_buffer_queued_count_| together. We can use a function to get > > > input_buffer_queued_count_. We can maintain one less variable. > > > int input_buffer_queued_count() { > > > return input_buffer_map_.size() - free_input_buffers_(); > > > } > > > > > > Same for |output_buffer_queued_count_|. > > > > We use input_buffer_queued_count to check queue empty in many places. I'd > prefer > > to use two more variables to save one subtraction for each time. > The time saving is very small. The point is it's easy to forget to change them > at the same time. It's also harder to see if the code is correct. For example, > in line 355 at the end of DestroyInputBuffers, do we need to set > input_buffer_queued_count_ = 0 after REQBUFS(0)? Are we relying > input_buffer_queued_count_ = 0 before DestroyInputBuffers is called? Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:96: DCHECK(client_); On 2015/06/22 22:40:53, wuchengli wrote: > This doesn't give any more information. The code has the same effect of crashing > in the next line without DCHECK. Remove. Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:168: (input_buffer_queued_count_ == 0 && output_buffer_queued_count_ == 0)) { On 2015/06/22 22:40:54, wuchengli wrote: > Can we just check (input_buffer_queued_count_ == 0 && > output_buffer_queued_count_ == 0)? If both streams are off, queued count should > also be 0. > > Also, these checks should go to ShouldRecreateInputBuffers() and > ShouldRecreateOutputBuffers(). See my comment in line 178. These check will be moved to CreateBufferIfNecessary(). https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:170: Enqueue(); On 2015/06/22 22:40:53, wuchengli wrote: > Shouldn't we allow the input buffer to be enqueued as long as no buffer > recreation is pending? I think Enqueue() should be split into two functions -- > EnqueueInput and EnqueueOutput. > > CreateBufferIfNecessary(); > if (!recreate_input_buffers_pending_ && !recreate_output_buffers_pending_) > EnqueueInput(); > EnqueueOutput(); I agree to split Enqueue to EnqueueInput and EnqueueOutput because the number of input queue can be greater than the number of output queue. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:171: device_poll_task_runner_->PostTask( On 2015/06/22 22:40:53, wuchengli wrote: > if (input_buffer_queued_count_ > 0 || output_buffer_queued_count_ > 0) { > PostTask() > } > The code here should be the same as ServiceDeviceTask. Can we remove line > 164-175 and just call ServiceDeviceTask() here? Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:178: void V4L2JpegDecodeAccelerator::CheckBufferAttributes() { On 2015/06/22 22:40:54, wuchengli wrote: > This name is not good. "Check" is vaque. It's hard to know what this is without > looking at the code of the function. Break this into two functions. > recreate_input_buffers_pending_ = ShouldRecreateInputBuffers(); > recreate_output_buffers_pending_ = ShouldRecreateOutputBuffers(); Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:201: bool V4L2JpegDecodeAccelerator::CreateBufferIfNecessary() { On 2015/06/22 22:40:54, wuchengli wrote: > s/CreateBufferIfNecessary/CreateBuffersIfNecessary/ Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:208: On 2015/06/22 22:40:54, wuchengli wrote: > We should return if input queued count or output queued count is not 0. We need > to prevent the buffer from re-creating when not all output buffers have been > dequeued? For example: > - JDA gets a DecodeTask. One input buffer and one output buffer are queued. > - JDA gets a DecodeTask with a different coded size. It is added to input_jobs_. > - ServiceDeviceTask is called. One input buffer is dequeued. If I understand > correctly, the buffers will be re-created even though the output buffer is not > dequeued yet. > > if (input_buffer_queued_count_ != 0 || output_buffer_queued_output_ != 0) > return true; Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:211: LOG(ERROR) << "Stop device poll thread failed when renew buffers."; On 2015/06/22 22:40:54, wuchengli wrote: > s/renew/recreating/ Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:222: LOG(ERROR) << "Create Input buffer failed."; On 2015/06/22 22:40:53, wuchengli wrote: > s/Input buffer/input buffers/ Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:226: LOG(ERROR) << "Create Output buffer failed."; On 2015/06/22 22:40:53, wuchengli wrote: > s/Output buffer/output buffers/ Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:327: PLOG(ERROR) << "CreateOutputBuffers(): mmap() failed"; On 2015/06/22 22:40:54, wuchengli wrote: > Please examine all LOG(ERROR) in this file and make sure it notifies an error > (and only one) to the client. Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:355: free_input_buffers_.clear(); On 2015/06/22 22:40:54, wuchengli wrote: > input_buffer_queued_count_ = 0; StopDevicePoll function will do this. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:375: free_output_buffers_.clear(); On 2015/06/22 22:40:54, wuchengli wrote: > output_buffer_queued_count_ = 0; StopDevicePoll function will do this. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:405: if (!input_jobs_.empty() && CreateBufferIfNecessary()) On 2015/06/22 22:40:54, wuchengli wrote: > Why we shouldn't call enqueue if input_jobs_ is empty? What if Dequeue() DQBUF > an output buffer and we need to QBUF it in Enqueue()? > > Also, move input_jobs_.empty() check to CreateBufferIfNecessary(). All the logic > to decide if creating buffers is necessary should go there. > > Should the code be like this? > if (CreateBufferIfNecessary()) return; > if (!recreate_input_buffers_pending_ && !recreate_output_buffers_pending_) > EnqueueInput(); > EnqueueOutput(); Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:412: if (input_buffer_queued_count_ > 0 && output_buffer_queued_count_ > 0) { On 2015/06/22 22:40:53, wuchengli wrote: > s/&&/||/. If any buffer is queued, we need to poll. Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:437: if (input_buffer_queued_count_ != 0 && !input_streamon_) { On 2015/06/22 22:40:54, wuchengli wrote: > Why can't we always start streaming if |input_streamon_|==false? Why do we need > to wait until at least one buffer is queued? Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:517: << job_record->frame->timestamp().InMilliseconds(); On 2015/06/22 22:40:54, wuchengli wrote: > timestamp is set to zero in gpu_jpeg_decode_accelerator.cc. No point of printing > it. Right? Remove. Same for line 548. Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:606: input_streamon_ = false; On 2015/06/22 22:40:54, wuchengli wrote: > Move this line inside if (input_streamon_) Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:612: output_streamon_ = false; On 2015/06/22 22:40:54, wuchengli wrote: > Move this line inside if (output_streamon_) Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:615: running_jobs_.pop(); On 2015/06/22 22:40:54, wuchengli wrote: > When StopDevicePoll is called in CreateBufferIfNecessary, |running_jobs_| should > be already empty. Also, |input_jobs_| is not emptied here. For consistency, move > line 614-615 to line 85 in DestroyTask(). Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:623: input_buffer_queued_count_ = 0; On 2015/06/22 22:40:54, wuchengli wrote: > Move this line to line 604 inside "if (input_streamon_)". Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:631: output_buffer_queued_count_ = 0; On 2015/06/22 22:40:54, wuchengli wrote: > Move this line to line 610 inside "if (output_streamon_)" Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:45: // Set true during QBUF and DQBUF. |adress| will be accessed by hardware. On 2015/06/22 22:40:55, wuchengli wrote: > s/adress/address/ Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:59: media::BitstreamBuffer bitstream_buffer; On 2015/06/22 22:40:55, wuchengli wrote: > Document these three variables. Also mention |bitstream_buffer| is input and > |frame| is output. Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:60: scoped_refptr<media::VideoFrame> frame; On 2015/06/22 22:40:55, wuchengli wrote: > s/frame/output_frame/ so it's more obvious what this is. Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:61: scoped_ptr<base::SharedMemory> shm; On 2015/06/22 22:40:54, wuchengli wrote: > Document this is mapped from |bitstream_buffer|. Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:75: bool CreateBufferIfNecessary(); On 2015/06/22 22:40:55, wuchengli wrote: > Document the return value -- False means an error has occurred. Otherwise, > people will think true means buffers have been created. Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:97: const size_t kBufferCount = 2; On 2015/06/22 22:40:55, wuchengli wrote: > Document this is the number of input buffers and output buffers. Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:102: // Set true when input or output buffers have to re-allocate. On 2015/06/22 22:40:55, wuchengli wrote: > s/re-allocate/be re-allocated/ Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:127: // All the below members are accessed from |decoder_thread_| only On 2015/06/22 22:40:55, wuchengli wrote: > All the below members except |weak_factory_| Done. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:152: // Weak factory for producing weak pointers on the decoder_thread_ On 2015/06/22 22:40:55, wuchengli wrote: > s/decoder/child/ Done.
https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:355: free_input_buffers_.clear(); On 2015/06/23 10:08:17, henryhsu wrote: > On 2015/06/22 22:40:54, wuchengli wrote: > > input_buffer_queued_count_ = 0; > > StopDevicePoll function will do this. If the driver drops all the buffers after VIDIOC_REQBUFS, we need to set input_buffer_queued_count_ to 0 here. My question is: what will driver do when some buffers are queued in the driver and gets REQBUFS(0)? Does the driver release all the buffers? https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:437: if (input_buffer_queued_count_ != 0 && !input_streamon_) { On 2015/06/23 10:08:16, henryhsu wrote: > On 2015/06/22 22:40:54, wuchengli wrote: > > Why can't we always start streaming if |input_streamon_|==false? Why do we > need > > to wait until at least one buffer is queued? > > Done. Can you check with Pawel if it's OK to STREAMON when no buffer is queued? I want to make sure it follows codec API. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:35: : address(nullptr), length(0), at_device(false) { |at_device| is only used by DCHECK. It's fine if that's your intention. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:202: return false; This is not an error. return true? I think this check should go to ShouldRecreateInputBuffers and ShouldRecreateOutputBuffers. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:209: // If queues are not empty, we should wait the pending frames finished. s/wait the pending frames finished/wait until pending frames finish/ https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:417: if (!recreate_input_buffers_pending_ && !recreate_output_buffers_pending_) Add comments to explain why we should not enqueue any input when we need to recreate buffers. Also why it's ok for output. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:449: while (OutputBufferQueuedCount() < InputBufferQueuedCount() && I'm not sure this is always correct. For example, - Gets one DecodTask. - One input and one outputs are queued. (1/1) - One input is dequeued. (0/1) - Gets one DecodeTask. One input is queued. (1/1) - One input and one output is dequeued. (0/0) Now we'll never queue the second output. Is this correct? while (running_jobs_.size() - OutputBufferQueuedCount() > 0 && https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:60: // Input stream of image. Stream sounds like a video stream. Let's call it "Input image buffer". https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:82: // Check input buffer size to decide recreate input buffers. s/decide recreate/decide whether to recreate/ https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:84: // Check resolution to decide recreate output buffers. The function checks more than resolution. We don't need to mention the implementation. // Return true if output buffers should be re-created.
PTAL. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:355: free_input_buffers_.clear(); On 2015/06/25 08:29:53, wuchengli wrote: > On 2015/06/23 10:08:17, henryhsu wrote: > > On 2015/06/22 22:40:54, wuchengli wrote: > > > input_buffer_queued_count_ = 0; > > > > StopDevicePoll function will do this. > If the driver drops all the buffers after VIDIOC_REQBUFS, we need to set > input_buffer_queued_count_ to 0 here. My question is: what will driver do when > some buffers are queued in the driver and gets REQBUFS(0)? Does the driver > release all the buffers? REQBUFS(0) should be after STREAM_OFF. Therefore, we don't have to consider this case. Now input_buffer_queued_count_ is calculated by the size of free_input_buffers and input_buffer_map_. https://codereview.chromium.org/1125263005/diff/120001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:437: if (input_buffer_queued_count_ != 0 && !input_streamon_) { On 2015/06/25 08:29:53, wuchengli wrote: > On 2015/06/23 10:08:16, henryhsu wrote: > > On 2015/06/22 22:40:54, wuchengli wrote: > > > Why can't we always start streaming if |input_streamon_|==false? Why do we > > need > > > to wait until at least one buffer is queued? > > > > Done. > Can you check with Pawel if it's OK to STREAMON when no buffer is queued? I want > to make sure it follows codec API. Pawel said it depends on different kernel. This patch https://chromium-review.googlesource.com/#/c/218105/ allows stream on before qbuf. I think we should check stream_on in Enqueue function to be compatible for all kernel versions. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:35: : address(nullptr), length(0), at_device(false) { On 2015/06/25 08:29:53, wuchengli wrote: > |at_device| is only used by DCHECK. It's fine if that's your intention. When we enqueue a buffer to device, we will set at_device to true. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:202: return false; On 2015/06/25 08:29:53, wuchengli wrote: > This is not an error. return true? I think this check should go to > ShouldRecreateInputBuffers and ShouldRecreateOutputBuffers. Done. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:209: // If queues are not empty, we should wait the pending frames finished. On 2015/06/25 08:29:53, wuchengli wrote: > s/wait the pending frames finished/wait until pending frames finish/ Done. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:417: if (!recreate_input_buffers_pending_ && !recreate_output_buffers_pending_) On 2015/06/25 08:29:53, wuchengli wrote: > Add comments to explain why we should not enqueue any input when we need to > recreate buffers. Also why it's ok for output. Done. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:449: while (OutputBufferQueuedCount() < InputBufferQueuedCount() && On 2015/06/25 08:29:53, wuchengli wrote: > I'm not sure this is always correct. For example, > - Gets one DecodTask. > - One input and one outputs are queued. (1/1) > - One input is dequeued. (0/1) > - Gets one DecodeTask. One input is queued. (1/1) > - One input and one output is dequeued. (0/0) > Now we'll never queue the second output. > > Is this correct? > while (running_jobs_.size() - OutputBufferQueuedCount() > 0 && Hmm...seems like we can always enqueue output buffer if free_output_buffers_ is not empty. So we don't check the number of pending buffers. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:60: // Input stream of image. On 2015/06/25 08:29:54, wuchengli wrote: > Stream sounds like a video stream. Let's call it "Input image buffer". Done. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:82: // Check input buffer size to decide recreate input buffers. On 2015/06/25 08:29:54, wuchengli wrote: > s/decide recreate/decide whether to recreate/ Done. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:84: // Check resolution to decide recreate output buffers. On 2015/06/25 08:29:53, wuchengli wrote: > The function checks more than resolution. We don't need to mention the > implementation. > > // Return true if output buffers should be re-created. Done.
https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:35: : address(nullptr), length(0), at_device(false) { On 2015/06/26 03:39:32, henryhsu wrote: > On 2015/06/25 08:29:53, wuchengli wrote: > > |at_device| is only used by DCHECK. It's fine if that's your intention. > > When we enqueue a buffer to device, we will set at_device to true. |at_device| is only set to true and false in various places. It is not actually used except DCHECK. It means you can remove all |at_device| if you want. Having at_device is only for debugging purpose. It's fine if you want to keep it for debugging. https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:449: while (OutputBufferQueuedCount() < InputBufferQueuedCount() && On 2015/06/26 03:39:32, henryhsu wrote: > On 2015/06/25 08:29:53, wuchengli wrote: > > I'm not sure this is always correct. For example, > > - Gets one DecodTask. > > - One input and one outputs are queued. (1/1) > > - One input is dequeued. (0/1) > > - Gets one DecodeTask. One input is queued. (1/1) > > - One input and one output is dequeued. (0/0) > > Now we'll never queue the second output. > > > > Is this correct? > > while (running_jobs_.size() - OutputBufferQueuedCount() > 0 && > > Hmm...seems like we can always enqueue output buffer if free_output_buffers_ is > not empty. So we don't check the number of pending buffers. No. This means you'll queue more output buffers than the number of DecodeTask. Buffer will never be re-created because of line 212. You'll call extra Deqeueu() in line 417. https://codereview.chromium.org/1125263005/diff/160001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/160001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:420: // Stop enqueue input record when input/output buffers are required to s/Stop enqueue/Do not queue/ "Stop enqueuing" means an action has been done and queuing has stopped because of the action from this point. In fact, no action is done. The code simply not to queue an input record. https://codereview.chromium.org/1125263005/diff/160001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:422: // Output record can be enqueued in this case since the number of This is not the reason why output can be queued. The new input record will have new resolution. The new output record will have the same resolution as the last queued DecodeTask. // Output record can be enqueued because the output coded sizes of the frames currently in the pipeline are all the same. https://codereview.chromium.org/1125263005/diff/160001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:453: if (!input_streamon_ && InputBufferQueuedCount()) { Why this part is moved back here?
https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/140001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:449: while (OutputBufferQueuedCount() < InputBufferQueuedCount() && On 2015/06/26 23:42:48, wuchengli wrote: > On 2015/06/26 03:39:32, henryhsu wrote: > > On 2015/06/25 08:29:53, wuchengli wrote: > > > I'm not sure this is always correct. For example, > > > - Gets one DecodTask. > > > - One input and one outputs are queued. (1/1) > > > - One input is dequeued. (0/1) > > > - Gets one DecodeTask. One input is queued. (1/1) > > > - One input and one output is dequeued. (0/0) > > > Now we'll never queue the second output. > > > > > > Is this correct? > > > while (running_jobs_.size() - OutputBufferQueuedCount() > 0 && > > > > Hmm...seems like we can always enqueue output buffer if free_output_buffers_ > is > > not empty. So we don't check the number of pending buffers. > No. This means you'll queue more output buffers than the number of DecodeTask. > Buffer will never be re-created because of line 212. You'll call extra Deqeueu() > in line 417. You right, there is a bug in CreateBuffersIfNecessary(). I have already fixed it after I ran unittest. Will update in next patch. https://codereview.chromium.org/1125263005/diff/160001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/160001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:453: if (!input_streamon_ && InputBufferQueuedCount()) { On 2015/06/26 23:42:48, wuchengli wrote: > Why this part is moved back here? Pawel said it depends on different kernel. This patch https://chromium-review.googlesource.com/#/c/218105/ allows stream on before qbuf. I think we should check stream_on in Enqueue function to be compatible for all kernel versions.
https://codereview.chromium.org/1125263005/diff/160001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/160001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:420: // Stop enqueue input record when input/output buffers are required to On 2015/06/26 23:42:48, wuchengli wrote: > s/Stop enqueue/Do not queue/ > > "Stop enqueuing" means an action has been done and queuing has stopped because > of the action from this point. In fact, no action is done. The code simply not > to queue an input record. Done. https://codereview.chromium.org/1125263005/diff/160001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:422: // Output record can be enqueued in this case since the number of On 2015/06/26 23:42:48, wuchengli wrote: > This is not the reason why output can be queued. The new input record will have > new resolution. The new output record will have the same resolution as the last > queued DecodeTask. > > // Output record can be enqueued because the output coded sizes of the frames > currently in the pipeline are all the same. Done.
https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:461: while (!free_output_buffers_.empty()) { If we queue as many as output buffers we can, we will do unnecessary work in line 417 and 431. If there's no other disadvantage, we should queue only the number of output buffers we need (running_jobs_.size() - OutputBufferQueuedCount()).
https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:112: struct v4l2_capability caps; memset to clear it. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:216: if (!StopDevicePoll()) { We don't need to stop and re-start the poll thread when re-creating buffers. Refactor StopDevicePoll so we don't need to do it. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:394: if (!device_->Poll(true, &event_pending)) { add an error log here https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:408: // ServiceDeviceTask() should only ever be scheduled from DevicePollTask(), ServiceDeviceTask can also be called from DecodeTask now. Line 408-410 can be removed. Just say "If a DestroyTask() shuts poll thread down, we should early-out." https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:428: if (!device_->ClearDevicePollInterrupt()) return; As discussed, this can be removed. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:439: << InputBufferQueuedCount() << "/" InputBufferQueuedCount() = input_buffer_map_.size() - free_input_buffers_.size(). We don't need to print this. Same for OutputBufferQueuedCount(). https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:452: if (!input_streamon_ && InputBufferQueuedCount()) { Please document that in earlier kernel, we cannot STREAMON before QBUF. Same for line 465. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:488: PLOG(ERROR) << "ioctl() failed: VIDIOC_DQBUF"; Let's mention "input" here. Maybe "Input buffer VIDIOC_DQBUF failed". Same for line 511. Mention "output" there. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:559: DVLOG(3) << __func__ << ": enqueued frame id=" s/frame/bitstream buffer/ https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:597: if (decoder_thread_.IsRunning()) remove this line. All callers run on decoder thread.
https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:15: PLOG(ERROR) << __func__ << "(): ioctl() failed: " << type_name; \ Why no NotifyError here? https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:29: PLOG(ERROR) << __func__ << "(): ioctl() failed: " << #type; \ Why no NotifyError here?
all done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:15: PLOG(ERROR) << __func__ << "(): ioctl() failed: " << type_name; \ On 2015/06/30 05:34:38, wuchengli wrote: > Why no NotifyError here? Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:29: PLOG(ERROR) << __func__ << "(): ioctl() failed: " << #type; \ On 2015/06/30 05:34:38, wuchengli wrote: > Why no NotifyError here? Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:112: struct v4l2_capability caps; On 2015/06/30 04:50:04, wuchengli wrote: > memset to clear it. Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:216: if (!StopDevicePoll()) { On 2015/06/30 04:50:04, wuchengli wrote: > We don't need to stop and re-start the poll thread when re-creating buffers. > Refactor StopDevicePoll so we don't need to do it. Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:394: if (!device_->Poll(true, &event_pending)) { On 2015/06/30 04:50:04, wuchengli wrote: > add an error log here Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:408: // ServiceDeviceTask() should only ever be scheduled from DevicePollTask(), On 2015/06/30 04:50:04, wuchengli wrote: > ServiceDeviceTask can also be called from DecodeTask now. Line 408-410 can be > removed. Just say "If a DestroyTask() shuts poll thread down, we should > early-out." Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:428: if (!device_->ClearDevicePollInterrupt()) return; On 2015/06/30 04:50:04, wuchengli wrote: > As discussed, this can be removed. Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:439: << InputBufferQueuedCount() << "/" On 2015/06/30 04:50:04, wuchengli wrote: > InputBufferQueuedCount() = input_buffer_map_.size() - > free_input_buffers_.size(). We don't need to print this. Same for > OutputBufferQueuedCount(). Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:452: if (!input_streamon_ && InputBufferQueuedCount()) { On 2015/06/30 04:50:04, wuchengli wrote: > Please document that in earlier kernel, we cannot STREAMON before QBUF. Same for > line 465. Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:461: while (!free_output_buffers_.empty()) { On 2015/06/30 02:52:15, wuchengli wrote: > If we queue as many as output buffers we can, we will do unnecessary work in > line 417 and 431. If there's no other disadvantage, we should queue only the > number of output buffers we need (running_jobs_.size() - > OutputBufferQueuedCount()). Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:488: PLOG(ERROR) << "ioctl() failed: VIDIOC_DQBUF"; On 2015/06/30 04:50:04, wuchengli wrote: > Let's mention "input" here. Maybe "Input buffer VIDIOC_DQBUF failed". Same for > line 511. Mention "output" there. Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:559: DVLOG(3) << __func__ << ": enqueued frame id=" On 2015/06/30 04:50:04, wuchengli wrote: > s/frame/bitstream buffer/ Done. https://codereview.chromium.org/1125263005/diff/180001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:597: if (decoder_thread_.IsRunning()) On 2015/06/30 04:50:04, wuchengli wrote: > remove this line. All callers run on decoder thread. Done.
https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:149: DCHECK_EQ(video_frame->format(), media::VideoFrame::I420); NotifyError UNSUPPORTED_JPEG if the format is I420. See JDA.h. https://code.google.com/p/chromium/codesearch#chromium/src/media/video/jpeg_d... https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:218: // If input queue is not empty, we should wait until pending frames finish. input queue or output queue https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:260: IOCTL_OR_ERROR_RETURN_FALSE(-1, VIDIOC_S_FMT, &format); Move the id to the last parameter. Having -1 at the first affects the readability. It's more clear to see what ioctl is in the first parameter. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:487: PostNotifyError(dqbuf.index, This is not bitstream buffer id. Same for all other PostNotifyError. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:552: IOCTL_OR_ERROR_RETURN_FALSE(index, VIDIOC_QBUF, &qbuf); The id is bitstream buffer id, not v4l2 index. Same for all others.
https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:84: while (!input_jobs_.empty()) input_jobs_.pop(); Did git cl format shorten to one line? Personally, I'd prefer two lines. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:85: while (!running_jobs_.empty()) running_jobs_.pop(); ditto https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:335: void* address = device_->Mmap(NULL, buffer.length, PROT_READ | PROT_WRITE, Is only PROT_READ enough? https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:144: std::queue<linked_ptr<JobRecord> > input_jobs_; s/> >/>>/ https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:145: std::queue<linked_ptr<JobRecord> > running_jobs_; s/> >/>>/
all done. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:84: while (!input_jobs_.empty()) input_jobs_.pop(); On 2015/06/30 10:06:09, kcwu wrote: > Did git cl format shorten to one line? > Personally, I'd prefer two lines. Done. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:85: while (!running_jobs_.empty()) running_jobs_.pop(); On 2015/06/30 10:06:10, kcwu wrote: > ditto Done. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:149: DCHECK_EQ(video_frame->format(), media::VideoFrame::I420); On 2015/06/30 08:08:55, wuchengli wrote: > NotifyError UNSUPPORTED_JPEG if the format is I420. See JDA.h. > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/jpeg_d... Done. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:218: // If input queue is not empty, we should wait until pending frames finish. On 2015/06/30 08:08:55, wuchengli wrote: > input queue or output queue Done. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:260: IOCTL_OR_ERROR_RETURN_FALSE(-1, VIDIOC_S_FMT, &format); On 2015/06/30 08:08:55, wuchengli wrote: > Move the id to the last parameter. Having -1 at the first affects the > readability. It's more clear to see what ioctl is in the first parameter. Done. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:335: void* address = device_->Mmap(NULL, buffer.length, PROT_READ | PROT_WRITE, On 2015/06/30 10:06:09, kcwu wrote: > Is only PROT_READ enough? I tried, we need PROT_WRITE. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:487: PostNotifyError(dqbuf.index, On 2015/06/30 08:08:55, wuchengli wrote: > This is not bitstream buffer id. Same for all other PostNotifyError. Done. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:552: IOCTL_OR_ERROR_RETURN_FALSE(index, VIDIOC_QBUF, &qbuf); On 2015/06/30 08:08:55, wuchengli wrote: > The id is bitstream buffer id, not v4l2 index. Same for all others. Done. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:144: std::queue<linked_ptr<JobRecord> > input_jobs_; On 2015/06/30 10:06:10, kcwu wrote: > s/> >/>>/ Done. https://codereview.chromium.org/1125263005/diff/220001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.h:145: std::queue<linked_ptr<JobRecord> > running_jobs_; On 2015/06/30 10:06:10, kcwu wrote: > s/> >/>>/ Done.
https://codereview.chromium.org/1125263005/diff/240001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/240001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:38: : address(nullptr), length(0), buffer_id(-1), at_device(false) { use JDA::kInvalidBitstreamBufferId for -1 https://codereview.chromium.org/1125263005/diff/240001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:595: IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_QBUF, &qbuf, buffer_id); just kInvalidBitstreamBufferId since this error is not associated to given input
https://codereview.chromium.org/1125263005/diff/240001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/240001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:38: : address(nullptr), length(0), buffer_id(-1), at_device(false) { On 2015/07/01 08:25:31, kcwu wrote: > use JDA::kInvalidBitstreamBufferId for -1 Done. https://codereview.chromium.org/1125263005/diff/240001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:595: IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_QBUF, &qbuf, buffer_id); On 2015/07/01 08:25:31, kcwu wrote: > just kInvalidBitstreamBufferId since this error is not associated to given input Done. https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:532: running_jobs_.pop(); Maybe we have to check dqbuf.flag is set V4L2_BUF_FLAG_ERROR or not.
https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:454: if (!input_jobs_.empty() && !free_input_buffers_.empty() && We should still allow to queue multiple input if the resolution has not changed. Let's move line 430-431 here. Change this back to a while loop. while (!input_jobs_.empty() && !free_input_buffers_.empty()) { if (recreate_input_buffers_pending_ || recreate_output_buffers_pending_) break; if (!EnqueueInputRecord()) return; recreate_input_buffers_pending_ = ShouldRecreateInputBuffers(); recreate_output_buffers_pending_ = ShouldRecreateOutputBuffers(); } https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:468: if (running_jobs_.size() > OutputBufferQueuedCount() && We should still allow multiple outputs to be queued here. Change back to while loop. https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:523: } From v4l2 API, does V4L2_BUF_FLAG_ERROR mean jpeg cannot be decoded or parsed by the driver? If yes, we should check it here and NotifyError. From v4l2 API, can bytes_used be 0? If yes, we should check it and NotifyError. https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:648: ResetQueues(); Move this to DestroyTask. StopDevicePoll shouldn't do other things other than stopping device poll thread.
https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:532: running_jobs_.pop(); On 2015/07/01 11:32:04, henryhsu wrote: > Maybe we have to check dqbuf.flag is set V4L2_BUF_FLAG_ERROR or not. Yes. According to Pawel, please NotifyError if V4L2_BUF_FLAG_ERROR is set.
https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:454: if (!input_jobs_.empty() && !free_input_buffers_.empty() && On 2015/07/01 11:46:47, wuchengli wrote: > We should still allow to queue multiple input if the resolution has not changed. > > Let's move line 430-431 here. Change this back to a while loop. > while (!input_jobs_.empty() && !free_input_buffers_.empty()) { > if (recreate_input_buffers_pending_ || recreate_output_buffers_pending_) > break; > if (!EnqueueInputRecord()) return; > recreate_input_buffers_pending_ = ShouldRecreateInputBuffers(); > recreate_output_buffers_pending_ = ShouldRecreateOutputBuffers(); > } > Done. https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:468: if (running_jobs_.size() > OutputBufferQueuedCount() && On 2015/07/01 11:46:47, wuchengli wrote: > We should still allow multiple outputs to be queued here. Change back to while > loop. Done. https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:523: } On 2015/07/01 11:46:47, wuchengli wrote: > From v4l2 API, does V4L2_BUF_FLAG_ERROR mean jpeg cannot be decoded or parsed by > the driver? If yes, we should check it here and NotifyError. > > From v4l2 API, can bytes_used be 0? If yes, we should check it and NotifyError. We check flags instead of bytesused. Added to notify UNSUPPORTED_JPEG when flags has V4L2_BUF_FLAG_ERROR. https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:532: running_jobs_.pop(); On 2015/07/02 07:19:36, wuchengli wrote: > On 2015/07/01 11:32:04, henryhsu wrote: > > Maybe we have to check dqbuf.flag is set V4L2_BUF_FLAG_ERROR or not. > Yes. According to Pawel, please NotifyError if V4L2_BUF_FLAG_ERROR is set. Done. https://codereview.chromium.org/1125263005/diff/260001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:648: ResetQueues(); On 2015/07/01 11:46:47, wuchengli wrote: > Move this to DestroyTask. StopDevicePoll shouldn't do other things other than > stopping device poll thread. Done.
https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:426: if (InputBufferQueuedCount() > 0 || OutputBufferQueuedCount() > 0) Change to !running_jobs_.empty()? https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:435: (InputBufferQueuedCount() > 0 || OutputBufferQueuedCount() > 0)) { Remove this because !running_jobs_.empty() is enough? https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:512: if (dqbuf.flags & V4L2_BUF_FLAG_ERROR) { Please check with Pawel if both input and output buffers can have this error. https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:516: running_jobs_.pop(); As discussed, check with Pawel if we can still dequeue an output buffer if input buffer has this error. https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:522: while (OutputBufferQueuedCount() > 0) { Change to while(!running_jobs_.empty()) and DCHECK(OutputBufferQueuedCount() > 0)?
LGTM. Remember to update the code after Pawel gets the reply from v4l2 group.
henryhsu@chromium.org changed reviewers: + piman@chromium.org
piman@chromium.org: Please review changes in Hi piman, Please take a look. Thanks. https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:426: if (InputBufferQueuedCount() > 0 || OutputBufferQueuedCount() > 0) On 2015/07/06 09:39:45, wuchengli wrote: > Change to !running_jobs_.empty()? Done. https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:435: (InputBufferQueuedCount() > 0 || OutputBufferQueuedCount() > 0)) { On 2015/07/06 09:39:45, wuchengli wrote: > Remove this because !running_jobs_.empty() is enough? Done. https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:522: while (OutputBufferQueuedCount() > 0) { On 2015/07/06 09:39:45, wuchengli wrote: > Change to while(!running_jobs_.empty()) and DCHECK(OutputBufferQueuedCount() > > 0)? Done.
https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:113: weak_factory_.GetWeakPtr(), bitstream_buffer_id, error)); I'm don't think it is generally safe to call GetWeakPtr on another thread. What is safe though is to call GetWeakPtr on the child thread (e.g. the constructor), save the WeakPtr as a field, and use it here (to post the task to the child thread). https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:558: client_->VideoFrameReady(job_record->bitstream_buffer.id()); You can only call into client_ on the child thread. Post a task?
PTAL https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:113: weak_factory_.GetWeakPtr(), bitstream_buffer_id, error)); On 2015/07/08 00:18:07, piman (Very slow to review) wrote: > I'm don't think it is generally safe to call GetWeakPtr on another thread. What > is safe though is to call GetWeakPtr on the child thread (e.g. the constructor), > save the WeakPtr as a field, and use it here (to post the task to the child > thread). Done. https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:512: if (dqbuf.flags & V4L2_BUF_FLAG_ERROR) { On 2015/07/06 09:39:45, wuchengli wrote: > Please check with Pawel if both input and output buffers can have this error. As discussed, we check both input and output error flag. https://codereview.chromium.org/1125263005/diff/280001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:558: client_->VideoFrameReady(job_record->bitstream_buffer.id()); On 2015/07/08 00:18:07, piman (Very slow to review) wrote: > You can only call into client_ on the child thread. Post a task? Done.
https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:295: PostNotifyError(media::JpegDecodeAccelerator::kInvalidBitstreamBufferId, Do we need to add media::JpegDecodeAccelerator:: given V4L2JDA inherits JpegDecodeAccelerator? https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:520: // Return the finished buffer to the client via the job ready callback. Add comment to explain we cannot check OutputBufferQueuedCount() > 0 here because the frame may have had an error during input buffer dequeue. https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:521: while (!running_jobs_.empty()) { Can we do "running_jobs_.size() - InputBufferQueuedCount()" here to optimize it? https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:543: DCHECK(!running_jobs_.empty()); Remove. It's checked at line 521.
https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:295: PostNotifyError(media::JpegDecodeAccelerator::kInvalidBitstreamBufferId, On 2015/07/08 03:13:37, wuchengli wrote: > Do we need to add media::JpegDecodeAccelerator:: given V4L2JDA inherits > JpegDecodeAccelerator? You right. We don't need this since V4L2JDA has already inherited from JpegDecodeAccelerator. https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:520: // Return the finished buffer to the client via the job ready callback. On 2015/07/08 03:13:37, wuchengli wrote: > Add comment to explain we cannot check OutputBufferQueuedCount() > 0 here > because the frame may have had an error during input buffer dequeue. Done. https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:521: while (!running_jobs_.empty()) { On 2015/07/08 03:13:37, wuchengli wrote: > Can we do "running_jobs_.size() - InputBufferQueuedCount()" here to optimize it? This implies that input buffer is always dequeued before output buffer. I think V4L2 API doesn't specify this order. https://codereview.chromium.org/1125263005/diff/290010/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:543: DCHECK(!running_jobs_.empty()); On 2015/07/08 03:13:37, wuchengli wrote: > Remove. It's checked at line 521. Done.
LGTM at a high level, but I mostly only looked at the threading and lifetime, not closely at the V4L2JpegDecodeAccelerator implementation details.
lgtm https://codereview.chromium.org/1125263005/diff/330001/content/common/gpu/med... File content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/1125263005/diff/330001/content/common/gpu/med... content/common/gpu/media/v4l2_jpeg_decode_accelerator.cc:522: // OutputBufferQueueCount() to avoid pop |running_jobs_| twice for one frame. |running_jobs_| won't be popped twice because DQBUF will return EAGAIN. It's better to say We check the size of |running_jobs_| instead of OutputBufferQueueCount() to avoid calling DQBUF unnecessarily.
The CQ bit was checked by henryhsu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org, wuchengli@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1125263005/#ps350001 (title: "fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1125263005/350001
Message was sent while issue was closed.
Committed patchset #18 (id:350001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/840b38edfdb6bb8c7c82e6f207bf2d7f6f505249 Cr-Commit-Position: refs/heads/master@{#337990} |
