Should use weak_this when calling ImageProcessor.Process() in v4l2 VDA/VEA. |
|||
Issue descriptionIn V4L2VideoEncodeAccelerator and V4L2VideoDecodeAccelerator, when they call image processor's Process(), they set FrameReadyCb with Unretained(this). This is wrong as the callback is eventually posted to child thread, which outlives v4l2VEA and v4l2VDA.
,
Dec 27
,
Dec 27
I uploaded WIP CL, but totally WIP. Please hold on review. https://chromium-review.googlesource.com/c/chromium/src/+/1391649
,
Jan 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15a6476487da46bb668f369c485827d8a6132e4a commit 15a6476487da46bb668f369c485827d8a6132e4a Author: Hirokazu Honda <hiroh@chromium.org> Date: Tue Jan 15 05:37:08 2019 media/gpu: Use CancelableTaskTracker for LibYUV/V4L2ImageProcessor::Reset() This introduces "real" Reset() behavior, meaning ImageProcessor doesn't perform pixel format conversion for pending buffers when Reset() is called. Originally, ImageProcessor performs pixel format conversions but doesn't call FrameReadyCB for the output frames by weak pointer of the ImageProcessor. By using CalcelableTaskTracker, we don't need to stop the internal thread of ImageProcessor in Reset(). Bug: 917310 Test: VDA and VEA unittest on hana, peach_pit and cheza Change-Id: Ia13e35120effa62d7b5c95619aaee3122baa9a47 Reviewed-on: https://chromium-review.googlesource.com/c/1399767 Commit-Queue: Hirokazu Honda <hiroh@chromium.org> Reviewed-by: Alexandre Courbot <acourbot@chromium.org> Cr-Commit-Position: refs/heads/master@{#622747} [modify] https://crrev.com/15a6476487da46bb668f369c485827d8a6132e4a/media/gpu/libyuv_image_processor.cc [modify] https://crrev.com/15a6476487da46bb668f369c485827d8a6132e4a/media/gpu/libyuv_image_processor.h [modify] https://crrev.com/15a6476487da46bb668f369c485827d8a6132e4a/media/gpu/v4l2/v4l2_image_processor.cc [modify] https://crrev.com/15a6476487da46bb668f369c485827d8a6132e4a/media/gpu/v4l2/v4l2_image_processor.h [modify] https://crrev.com/15a6476487da46bb668f369c485827d8a6132e4a/media/gpu/v4l2/v4l2_video_decode_accelerator.cc
,
Jan 17
(5 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e56d4a201abe324dd428da56e9170375ca9ff7e3 commit e56d4a201abe324dd428da56e9170375ca9ff7e3 Author: Hirokazu Honda <hiroh@chromium.org> Date: Thu Jan 17 12:07:06 2019 media/gpu: Remove weak pointer of ImageProcessor class The passed callbacks to ImageProcessor, FrameReadyCB and ErrorCB, are usually the client functions. ImageProcessor class judges whether or not they are called by its lifetime using the weak pointer of itself. Since the lifetime of ImageProcessor is usually the same as the client, this way should be fine, but not ideal. It would rather be decided by the existence of the client. This also introduces "real" Reset() behavior as ImageProcessor doesn't perform pixel format conversion for pending buffers when Reset() is called. Originally, ImageProcessor performs the pixel format conversion but not call FrameReadyCB. Bug: 917310 Test: VDA and VEA unittest on hana, peach_pit, cheza Change-Id: I9c92e9cbb8f1dd8f04db154849e7b1495bf2f0cd Reviewed-on: https://chromium-review.googlesource.com/c/1391649 Commit-Queue: Hirokazu Honda <hiroh@chromium.org> Reviewed-by: Alexandre Courbot <acourbot@chromium.org> Cr-Commit-Position: refs/heads/master@{#623655} [modify] https://crrev.com/e56d4a201abe324dd428da56e9170375ca9ff7e3/media/gpu/image_processor.cc [modify] https://crrev.com/e56d4a201abe324dd428da56e9170375ca9ff7e3/media/gpu/image_processor.h [modify] https://crrev.com/e56d4a201abe324dd428da56e9170375ca9ff7e3/media/gpu/libyuv_image_processor.cc [modify] https://crrev.com/e56d4a201abe324dd428da56e9170375ca9ff7e3/media/gpu/libyuv_image_processor.h [modify] https://crrev.com/e56d4a201abe324dd428da56e9170375ca9ff7e3/media/gpu/v4l2/v4l2_image_processor.cc [modify] https://crrev.com/e56d4a201abe324dd428da56e9170375ca9ff7e3/media/gpu/v4l2/v4l2_image_processor.h [modify] https://crrev.com/e56d4a201abe324dd428da56e9170375ca9ff7e3/media/gpu/v4l2/v4l2_video_decode_accelerator.cc [modify] https://crrev.com/e56d4a201abe324dd428da56e9170375ca9ff7e3/media/gpu/v4l2/v4l2_video_encode_accelerator.cc
,
Jan 18
(5 days ago)
,
Yesterday
(47 hours ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a2062b754caeca0f1ce708b686ee91a0fff2bad4 commit a2062b754caeca0f1ce708b686ee91a0fff2bad4 Author: Alexandre Courbot <acourbot@chromium.org> Date: Mon Jan 21 07:35:09 2019 media/gpu/v4l2: move picture clearing back to the correct thread CL crrev.com/c/1391649 changed the thread on which picture clearing takes place from the child thread to the decode thread. This can result in a picture being sent before an EGLImage is created for it, since the later event happens on the child thread and is scheduled before the picture clearing. Moving picture clearing to the decoder thread removes any order guarantee that we had before. Revert this part to what it was before crrev.com/c/1391649 since it seems to be a typo. Bug: 917310 Bug: 920908 Test: VDA passing in debug mode on nyan_big Change-Id: Ic44c37f6aaa6a966c2f11a5e7363975229ec04a8 Reviewed-on: https://chromium-review.googlesource.com/c/1424629 Commit-Queue: Alexandre Courbot <acourbot@chromium.org> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> Cr-Commit-Position: refs/heads/master@{#624526} [modify] https://crrev.com/a2062b754caeca0f1ce708b686ee91a0fff2bad4/media/gpu/v4l2/v4l2_video_decode_accelerator.cc |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Dec 26