|
|
Created:
6 years, 9 months ago by sheu Modified:
6 years, 9 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionStart the VDA device poll thread in Initialize()
We may need to be returning completed input buffers from the device while we run
DecodeBufferInitial() to set up the decoder. Start the device thread then in
Initialize() so the device is polled for input buffers to return.
This is can be necessary in some cases where the decoder may run out of buffers
to enqueue inputs on while scanning for valid input starting positions.
BUG=None
TEST=local build, run on CrOS snow
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258097
Patch Set 1 : c0b94e10 Initial. #
Total comments: 2
Patch Set 2 : 2426734d Update. #
Total comments: 6
Patch Set 3 : c930ce4cc Comments. #Patch Set 4 : fed18955 offline discussion with posciak@ #Messages
Total messages: 21 (0 generated)
posciak@: PTAL. This should fix the stuck business for Tegra VDA.
https://chromiumcodereview.appspot.com/189993002/diff/1/content/common/gpu/me... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left): https://chromiumcodereview.appspot.com/189993002/diff/1/content/common/gpu/me... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416: NOTIFY_ERROR(PLATFORM_FAILURE); Could you please explain/add to CL description why we had to move this out of here? It's not clear to me. Initialize() does report PLATFORM_FAILURE too, so it's not because it's too early to do this from Initialize(), right?
On 2014/03/07 04:05:22, sheu wrote: > posciak@: PTAL. This should fix the stuck business for Tegra VDA. Please make sure to test with vdatest if you haven't and also MSE.
Updated. PTAL https://chromiumcodereview.appspot.com/189993002/diff/1/content/common/gpu/me... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left): https://chromiumcodereview.appspot.com/189993002/diff/1/content/common/gpu/me... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416: NOTIFY_ERROR(PLATFORM_FAILURE); On 2014/03/07 06:14:47, Pawel Osciak wrote: > Could you please explain/add to CL description why we had to move this out of > here? It's not clear to me. Initialize() does report PLATFORM_FAILURE too, so > it's not because it's too early to do this from Initialize(), right? D'oh. I was thinking of: https://chromiumcodereview.appspot.com/185403020/ after which Initialize() is synchronous and returns bool instead of erroring. Will fix.
https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left): https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416: NOTIFY_ERROR(PLATFORM_FAILURE); Could you instead just: if (state != kUninitialized) NOTIFY_ERROR() ?
https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left): https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416: NOTIFY_ERROR(PLATFORM_FAILURE); On 2014/03/09 02:34:50, Pawel Osciak wrote: > Could you instead just: > > if (state != kUninitialized) > NOTIFY_ERROR() > > ? Not sure what you mean here. We should error out also if thread start fails.
https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left): https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416: NOTIFY_ERROR(PLATFORM_FAILURE); On 2014/03/10 19:38:42, sheu wrote: > On 2014/03/09 02:34:50, Pawel Osciak wrote: > > Could you instead just: > > > > if (state != kUninitialized) > > NOTIFY_ERROR() > > > > ? > > Not sure what you mean here. We should error out also if thread start fails. Sorry, should have been more precise. The issue here is that you had to move NOTIFY_ERROR out of {Start,Stop}DevicePoll because you didn't want to NOTIFY_ERROR in Initialize(). What I'm suggesting is to keep NOTIFY_ERROR in those methods, but only call it if state is not kUnintialized (i.e. it's ok to NOTIFY_ERROR). Or maybe even better, check for it in the actual NOTIFY_ERROR macro?
https://codereview.chromium.org/189993002/diff/10001/content/common/gpu/media... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/189993002/diff/10001/content/common/gpu/media... content/common/gpu/media/v4l2_video_decode_accelerator.cc:289: It seems there is a race condition here. The DevicePollTask() can get scheduled before this thread creates the decoder_thread_ and access to decoder_thread_ in DevicePollTask() will seg fault. StartDevicePoll() if done after decoder_thread_ starts.
https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left): https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416: NOTIFY_ERROR(PLATFORM_FAILURE); On 2014/03/11 00:07:44, Pawel Osciak wrote: > On 2014/03/10 19:38:42, sheu wrote: > > On 2014/03/09 02:34:50, Pawel Osciak wrote: > > > Could you instead just: > > > > > > if (state != kUninitialized) > > > NOTIFY_ERROR() > > > > > > ? > > > > Not sure what you mean here. We should error out also if thread start fails. > > Sorry, should have been more precise. The issue here is that you had to move > NOTIFY_ERROR out of {Start,Stop}DevicePoll because you didn't want to > NOTIFY_ERROR in Initialize(). > > What I'm suggesting is to keep NOTIFY_ERROR in those methods, but only call it > if state is not kUnintialized (i.e. it's ok to NOTIFY_ERROR). > > Or maybe even better, check for it in the actual NOTIFY_ERROR macro? I'd be more inclined to check in the macro, but even then I'm not really inclined to conditionally do NOTIFY_ERROR. If a call is made in a location where state matters, I'd prefer that the caller take care of doing things that are appropriate for the state, especially since the state is part of decoder thread state and it would be a whole 'nother can of worms to make that state safe to query from the child thread as well. https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:289: On 2014/03/12 12:12:27, shivdasp wrote: > It seems there is a race condition here. The DevicePollTask() can get scheduled > before this thread creates the decoder_thread_ and access to decoder_thread_ in > DevicePollTask() will seg fault. > StartDevicePoll() if done after decoder_thread_ starts. You're completely correct here. Thanks. Fixed.
For the record, I did run vda_unittests, flash video, and html5 video (with/without MSE). The race just didn't come up. PTAL, in any case!
Updated, per discussion.
lgtm Thanks for updating.
Adding fischman@ for owners. PTAL, thanks!
LGTM % please update description to describe why the new functionality is interesting? (hinted at in your first reviewlog message, but not spelled out)
The CQ bit was checked by sheu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/189993002/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by sheu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/189993002/50001
Message was sent while issue was closed.
Change committed as 258097 |