New issue
Advanced search Search tips

Issue 909547 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocked on:
issue 881875



Sign in to add a comment

Use GUARDED_BY in VaapiWrapper

Project Member Reported by hiroh@chromium.org, Nov 28

Issue description

GUARDED_BY is available to use in chromium code base and recommended to use [1].
VaapiWrapper is a heavy user to lock, va_lock*.
We should statically check the correctness of lock status on each function.
NOTE: GUARDED_BY apparently doesn't work with base::AutoUnlock(). We should think we are able to get rid of base::AutoUnlock in VaapiWrapper.

[1] https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer#!msg/chromium-dev/mG5sfyU1GGk/uo9ht2PsAgAJ
 
In fact, I found the lock is not acquired in CreateContext() if it is called by VaapiWrapper caller, though the lock is acquired if it is called from CreateSurfaces(). :( I am creating a CL to fix this.
Labels: -Type-Bug Type-Feature
Miguel, I assign you wondering whether you have bandwidth for this.
Good idea Hiro! Only just discovered the existence of the 'GUARDED_BY' annotation, it's amazing!

This change looks straightforward enough. If you want I'll take it up!
Why would we need to get rid of base::AutoUnlock? The GUARDED_BY macro checks whether variables are only accessed when the lock is acquired, it doesn't check whether the lock is released in all other cases. So the right thing the GUARDED_BY macro should do is ignore the use of 'AutoUnlock'.

I quickly added some GUARDED_BY macros, to see what would happen in: http://crrev.com/c/1353063. As expected it throws out tons of disturbing warnings.

I guess the important thing we want here, is that all calls to the underlying API are serialized. As we can't use the macro on functions we need to add it to variables that are used in these function calls.

va_display_: All access to this var should definitely be protected by a lock, as it's used in most API calls. It throws out tons of warnings, e.g. in vaCreateConfig().
va_config_id_: Similar to the above
va_surface_ids_: Same as above
va_surface_format_: Not sure if this one need to be protected, maybe to be on the safe side.

After Hiro's CL a lot of these warnings will disappear? I'll wait for the CL to get submitted and see if I can fix the remaining errors.
Thanks david for quick experiment. Let's fix together next week by pair-programming.
Cc: acourbot@chromium.org
+acourbot@, who may be interested in GUARDED_BY introduction in vaapi wrapper.
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/04fbdd9f175a350b7a674ab0610c3963428e0e1d

commit 04fbdd9f175a350b7a674ab0610c3963428e0e1d
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Thu Nov 29 08:31:34 2018

media/gpu/vaapi/vaapiWrapper: Acquire lock in CreateContext() and DestroySurfaces()

CreateContext() doesn't acquire lock if it is called by VaapiWrapper's caller.
Lock should be acquired always in CreateContext(). This change also removes
DestroySurfaces_Locked() and instead DestroySurfaces() is called everywhere.
CreateSurfaces() and DestroySurfaces() are renamed to CreateContextAndSurfaces()
and DestroyContextAndSurfaces() to specify what they are doing.

Bug:  909547 , 909561
Test: VDA, VEA, JDA, JEA unittest on eve
Change-Id: Ib04386307c2e83d83e0afe079a1868ac02eff086
Reviewed-on: https://chromium-review.googlesource.com/c/1353056
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612101}
[modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_jpeg_decode_accelerator.cc
[modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_jpeg_decode_accelerator_unittest.cc
[modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_jpeg_encode_accelerator.cc
[modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
[modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_video_encode_accelerator.cc
[modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_wrapper.cc
[modify] https://crrev.com/04fbdd9f175a350b7a674ab0610c3963428e0e1d/media/gpu/vaapi/vaapi_wrapper.h

Blockedon: 881875
https://bugs.chromium.org/p/chromium/issues/detail?id=881875#c15

I see that a pattern e.g.

  Bla input_buffers_ GUARDED_BY(lock_);

and 

  lock_.AssertAcquired();
  // ...
  input_buffers_.pop();

causes

... : error: reading variable 'input_buffers_' requires holding mutex 'lock_' [-Werror,-Wthread-safety-analysis]
  input_buffers_.pop();
I suspect that you need to add EXCLUSIVE_LOCKS_REQUIRED(lock_) annotation to the function calling |lock_.AssertAcquired()| - similarly how this was done in r593981 - see the changes that r593981 made in plugin_list.h and see the call to lock_.AssertAcquired() in PluginList::RemoveExtraPluginPathLocked.

OTOH, I am only guessing that the suggestion above will help - I don't have enough info to be confident in that suggestion (e.g. I don't know where |input_buffers_.pop();| is).
Cc: pwnall@chromium.org
mcasas@, so - which bug should we use going forward to discuss the issue you've run into?  You made the same comment in https://crbug.com/881875#c15 and  https://crbug.com/909547#c8  and now you have two people separately giving back the same advice... :-/
lukasza@, I just tried your suggestion and it was spot on, the
EXCLUSIVE_LOCKS_REQUIRED() annotation made it understand that
the |input_buffers_| was going to be protected. Thanks !!
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99e2d9e3cdba89a15ebd4890b2de37336963a0b0

commit 99e2d9e3cdba89a15ebd4890b2de37336963a0b0
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Thu Dec 13 03:38:03 2018

Vaapi: Use GUARDED_BY() static annotations on decoder

This CL introduces GUARDED_BY() static annotation on VaVDA; not all
members variables need to be guarded, so it adds on a number of them
a comment about on which thread it is used.

Bug:  909547 
Change-Id: I437a82320795aebc56f77cd1254bb7c8c2f2f5da
Reviewed-on: https://chromium-review.googlesource.com/c/1368590
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616196}
[modify] https://crrev.com/99e2d9e3cdba89a15ebd4890b2de37336963a0b0/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/99e2d9e3cdba89a15ebd4890b2de37336963a0b0/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/99e2d9e3cdba89a15ebd4890b2de37336963a0b0/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Dec 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/018fa575f326ce8a1d9173b3705f40f0771713f3

commit 018fa575f326ce8a1d9173b3705f40f0771713f3
Author: David Staessens <dstaessens@chromium.org>
Date: Thu Dec 13 04:34:05 2018

media/gpu/vaapi: Use GUARDED_BY annotation in the Vaapi Wrapper.

This CL adds the necessary checks to make sure that all access to the underlying
VAAPI library is thread-safe.

TEST=ran VDA and JDA tests on Nocturne

BUG= 909547 

Change-Id: Ibf1ee6a8f2d07e31c1142eb7629e9c0e903d00a8
Reviewed-on: https://chromium-review.googlesource.com/c/1353063
Commit-Queue: David Staessens <dstaessens@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616215}
[modify] https://crrev.com/018fa575f326ce8a1d9173b3705f40f0771713f3/media/gpu/vaapi/vaapi_jpeg_decode_accelerator_unittest.cc
[modify] https://crrev.com/018fa575f326ce8a1d9173b3705f40f0771713f3/media/gpu/vaapi/vaapi_wrapper.cc
[modify] https://crrev.com/018fa575f326ce8a1d9173b3705f40f0771713f3/media/gpu/vaapi/vaapi_wrapper.h

Project Member

Comment 14 by bugdroid1@chromium.org, Dec 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/57d51cc17e0e2c806d1b03eb6ce0905ed25f4cda

commit 57d51cc17e0e2c806d1b03eb6ce0905ed25f4cda
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Fri Dec 14 00:34:41 2018

Revert "Vaapi: Use GUARDED_BY() static annotations on decoder"

This reverts commit 99e2d9e3cdba89a15ebd4890b2de37336963a0b0.

Reason for revert: This breaks Video Playback on intel devices

Original change's description:
> Vaapi: Use GUARDED_BY() static annotations on decoder
>
> This CL introduces GUARDED_BY() static annotation on VaVDA; not all
> members variables need to be guarded, so it adds on a number of them
> a comment about on which thread it is used.
>
> Bug:  909547 
> Change-Id: I437a82320795aebc56f77cd1254bb7c8c2f2f5da
> Reviewed-on: https://chromium-review.googlesource.com/c/1368590
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616196}

TBR=mcasas@chromium.org,hiroh@chromium.org,dstaessens@chromium.org

Change-Id: I2e6496e9148f12d37f746a7d0b1e0e26cee70341
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  909547 ,  914954 
Reviewed-on: https://chromium-review.googlesource.com/c/1377470
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616522}
[modify] https://crrev.com/57d51cc17e0e2c806d1b03eb6ce0905ed25f4cda/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/57d51cc17e0e2c806d1b03eb6ce0905ed25f4cda/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/57d51cc17e0e2c806d1b03eb6ce0905ed25f4cda/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

Miguel, I reverted crrev.com/c/1368590 because it causes video playback breakage on intel devices. The problem is reproducible to play video on crosvideo.appspot.com. Could you take a look when you reland it?

Thanks.
Running the video_decode_accelerator_unittests seems to run into a deadlock:
[ RUN      ] ResourceExhaustion/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1
[13010:13014:1212/235931.839574:198195839583:FATAL:lock_impl_posix.cc(103)] Check failed: rv == 0 (35 vs. 0). Resource deadlock avoided. 
#0 0x555b4aa7d328 base::debug::StackTrace::StackTrace()
#1 0x555b4a84719c base::debug::StackTrace::StackTrace()
#2 0x555b4a87e07a logging::LogMessage::~LogMessage()
#3 0x555b4aaa06ea base::internal::LockImpl::Lock()
#4 0x555b4825be73 base::Lock::Acquire()
#5 0x555b4825be03 base::AutoLock::AutoLock()
#6 0x555b4a2a5eab media::VaapiVideoDecodeAccelerator::OutputPicture()
#7 0x555b4a2be952 _ZN4base8internal13FunctorTraitsIMN5media27VaapiVideoDecodeAcceleratorEFvRK13scoped_refptrINS2_9VASurfaceEEiN3gfx4RectERKNS2_15VideoColorSpaceEEvE6InvokeISF_NS_7WeakPtrIS3_EEJS6_iSA_SB_EEEvT_OT0_DpOT1_
#8 0x555b4a2be847 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIMN5media27VaapiVideoDecodeAcceleratorEFvRK13scoped_refptrINS4_9VASurfaceEEiN3gfx4RectERKNS4_15VideoColorSpaceEENS_7WeakPtrIS5_EEJS8_iSC_SD_EEEvOT_OT0_DpOT1_
#9 0x555b4a2be77d _ZN4base8internal7InvokerINS0_9BindStateIMN5media27VaapiVideoDecodeAcceleratorEFvRK13scoped_refptrINS3_9VASurfaceEEiN3gfx4RectERKNS3_15VideoColorSpaceEEJNS_7WeakPtrIS4_EES7_iSB_SC_EEEFvvEE7RunImplISG_NSt3__15tupleIJSI_S7_iSB_SC_EEEJLm0ELm1ELm2ELm3ELm4EEEEvOT_OT0_NSN_16integer_sequenceImJXspT1_EEEE
#10 0x555b4a2be569 _ZN4base8internal7InvokerINS0_9BindStateIMN5media27VaapiVideoDecodeAcceleratorEFvRK13scoped_refptrINS3_9VASurfaceEEiN3gfx4RectERKNS3_15VideoColorSpaceEEJNS_7WeakPtrIS4_EES7_iSB_SC_EEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#11 0x555b482db92c _ZNO4base12OnceCallbackIFvvEE3RunEv
#12 0x555b4a2a6f01 media::VaapiVideoDecodeAccelerator::TryOutputPicture()
#13 0x555b4a2aeff3 media::VaapiVideoDecodeAccelerator::SurfaceReady()
#14 0x555b4a2bdcf8 _ZN4base8internal13FunctorTraitsIMN5media27VaapiVideoDecodeAcceleratorEFvRK13scoped_refptrINS2_9VASurfaceEEiRKN3gfx4RectERKNS2_15VideoColorSpaceEEvE6InvokeISH_RKNS_7WeakPtrIS3_EEJS8_RKiSC_SF_EEEvT_OT0_DpOT1_
#15 0x555b4a2bdc27 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN5media27VaapiVideoDecodeAcceleratorEFvRK13scoped_refptrINS4_9VASurfaceEEiRKN3gfx4RectERKNS4_15VideoColorSpaceEERKNS_7WeakPtrIS5_EEJSA_RKiSE_SH_EEEvOT_OT0_DpOT1_
#16 0x555b4a2bdb5d _ZN4base8internal7InvokerINS0_9BindStateIMN5media27VaapiVideoDecodeAcceleratorEFvRK13scoped_refptrINS3_9VASurfaceEEiRKN3gfx4RectERKNS3_15VideoColorSpaceEEJNS_7WeakPtrIS4_EES7_iSB_SE_EEEFvvEE7RunImplIRKSI_RKNSt3__15tupleIJSK_S7_iSB_SE_EEEJLm0ELm1ELm2ELm3ELm4EEEEvOT_OT0_NSR_16integer_sequenceImJXspT1_EEEE
#17 0x555b4a2bd93c _ZN4base8internal7InvokerINS0_9BindStateIMN5media27VaapiVideoDecodeAcceleratorEFvRK13scoped_refptrINS3_9VASurfaceEEiRKN3gfx4RectERKNS3_15VideoColorSpaceEEJNS_7WeakPtrIS4_EES7_iSB_SE_EEEFvvEE3RunEPNS0_13BindStateBaseE
#18 0x555b482db92c _ZNO4base12OnceCallbackIFvvEE3RunEv
#19 0x555b4aadcf38 base::debug::TaskAnnotator::RunTask()
#20 0x555b4a89261a base::MessageLoopImpl::RunTask()
#21 0x555b4a8929ce base::MessageLoopImpl::DeferOrRunPendingTask()
#22 0x555b4a8933b9 base::MessageLoopImpl::DoWork()
#23 0x555b4aab6085 base::MessagePumpLibevent::Run()
#24 0x555b4a891d43 base::MessageLoopImpl::Run()
#25 0x555b4a90761d base::RunLoop::Run()
#26 0x555b4a9cd508 base::Thread::Run()
#27 0x555b4a9ce0e3 base::Thread::ThreadMain()
#28 0x555b4aaab186 base::(anonymous namespace)::ThreadFunc()
#29 0x7cfe606804fe <unknown>
#30 0x7cfe5fb7fbef clone

[12698:12701:1212/235932.014532:198196014546:ERROR:kill_posix.cc(84)] Unable to terminate process group 13010: No such process (3)
[31/40] ResourceExhaustion/VideoDecodeAcceleratorParamTest.TestSimpleDecode/1 (CRASHED)
[13022:13022:1212/235932.214757:198196214859:ERROR:icu_util.cc(172)] Invalid file descriptor to ICU data received.
Note: Google Test filter = ResourceExhaustion/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0

It seems fixed after the revert.
Created crrev.com/c/1377479 that fixes the deadlocks.
Project Member

Comment 18 by bugdroid1@chromium.org, Dec 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8ad3f6a5f474abbbb395e89eb25cf76ed1f3c5c

commit b8ad3f6a5f474abbbb395e89eb25cf76ed1f3c5c
Author: David Staessens <dstaessens@chromium.org>
Date: Wed Dec 19 10:14:13 2018

Re-land "Vaapi: Use GUARDED_BY() static annotations on decoder"

Deadlocks in the original code are fixed now.

TEST=ran VDA tests on Nocturne

Bug= 909547 

> Vaapi: Use GUARDED_BY() static annotations on decoder
>
> This CL introduces GUARDED_BY() static annotation on VaVDA; not all
> members variables need to be guarded, so it adds on a number of them
> a comment about on which thread it is used.

> Bug:  909547 
> Change-Id: I437a82320795aebc56f77cd1254bb7c8c2f2f5da
> Reviewed-on: https://chromium-review.googlesource.com/c/1368590
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616196}

Change-Id: If37bfd2d1f24b9b833bf8b291363b5c19861a6c7
Reviewed-on: https://chromium-review.googlesource.com/c/1377479
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617781}
[modify] https://crrev.com/b8ad3f6a5f474abbbb395e89eb25cf76ed1f3c5c/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/b8ad3f6a5f474abbbb395e89eb25cf76ed1f3c5c/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/b8ad3f6a5f474abbbb395e89eb25cf76ed1f3c5c/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Dec 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cb5feb064ec4a02a8591e59c61e44920d7455a47

commit cb5feb064ec4a02a8591e59c61e44920d7455a47
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Thu Dec 20 02:44:26 2018

Revert "Re-land "Vaapi: Use GUARDED_BY() static annotations on decoder""

This reverts commit b8ad3f6a5f474abbbb395e89eb25cf76ed1f3c5c.

Reason for revert: This causes inifite-loop in IMPORT mode.

Original change's description:
> Re-land "Vaapi: Use GUARDED_BY() static annotations on decoder"
> 
> Deadlocks in the original code are fixed now.
> 
> TEST=ran VDA tests on Nocturne
> 
> Bug= 909547 
> 
> > Vaapi: Use GUARDED_BY() static annotations on decoder
> >
> > This CL introduces GUARDED_BY() static annotation on VaVDA; not all
> > members variables need to be guarded, so it adds on a number of them
> > a comment about on which thread it is used.
> 
> > Bug:  909547 
> > Change-Id: I437a82320795aebc56f77cd1254bb7c8c2f2f5da
> > Reviewed-on: https://chromium-review.googlesource.com/c/1368590
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > Commit-Queue: Miguel Casas <mcasas@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#616196}
> 
> Change-Id: If37bfd2d1f24b9b833bf8b291363b5c19861a6c7
> Reviewed-on: https://chromium-review.googlesource.com/c/1377479
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#617781}

TBR=mcasas@chromium.org,hiroh@chromium.org,dstaessens@chromium.org

Change-Id: Ic4b1e7e8de8403173f71a415094ed23cab51f5e4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/1385689
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618085}
[modify] https://crrev.com/cb5feb064ec4a02a8591e59c61e44920d7455a47/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/cb5feb064ec4a02a8591e59c61e44920d7455a47/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/cb5feb064ec4a02a8591e59c61e44920d7455a47/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Dec 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de13353df3f9dbc253062fb35c184f967ba721b3

commit de13353df3f9dbc253062fb35c184f967ba721b3
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Thu Dec 20 04:41:42 2018

Revert "RELAND: Vaapi decode: split |decoder_|s GetRequiredNumOfPictures()"

This reverts commit f3a251c4ae81ebb23a95131e040c217d022b3a93 (crrev.com/c/1379274)
Reason for revert: This causes GTS breakage

TBR=mcasas@chromium.org

Bug:  909547 
Bug: b:121169667, b:121003733
Test: GtsExoPlayerTestCases

Change-Id: I949843abf926488e152f215a26391b3d08ba4be5
Reviewed-on: https://chromium-review.googlesource.com/c/1385692
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618112}
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/accelerated_video_decoder.h
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/h264_decoder.cc
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/h264_decoder.h
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/vp8_decoder.cc
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/vp8_decoder.h
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/vp9_decoder.cc
[modify] https://crrev.com/de13353df3f9dbc253062fb35c184f967ba721b3/media/gpu/vp9_decoder.h

Project Member

Comment 21 by bugdroid1@chromium.org, Dec 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c66ed12a80d9dce342314fbc03240cb44cd3f0b

commit 5c66ed12a80d9dce342314fbc03240cb44cd3f0b
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Thu Dec 20 17:26:21 2018

Reland "Re-land "Vaapi: Use GUARDED_BY() static annotations on decoder""

This reverts commit cb5feb064ec4a02a8591e59c61e44920d7455a47.

Bug:  909547 
Test: CTS and GTS
Test: VDA unittest with --test_import

Original change's description:
> Revert "Re-land "Vaapi: Use GUARDED_BY() static annotations on decoder""
>
> This reverts commit b8ad3f6a5f474abbbb395e89eb25cf76ed1f3c5c.
>
> Reason for revert: This causes inifite-loop in IMPORT mode.
>
> Original change's description:
> > Re-land "Vaapi: Use GUARDED_BY() static annotations on decoder"
> >
> > Deadlocks in the original code are fixed now.
> >
> > TEST=ran VDA tests on Nocturne
> >
> > Bug= 909547 
> >
> > > Vaapi: Use GUARDED_BY() static annotations on decoder
> > >
> > > This CL introduces GUARDED_BY() static annotation on VaVDA; not all
> > > members variables need to be guarded, so it adds on a number of them
> > > a comment about on which thread it is used.
> >
> > > Bug:  909547 
> > > Change-Id: I437a82320795aebc56f77cd1254bb7c8c2f2f5da
> > > Reviewed-on: https://chromium-review.googlesource.com/c/1368590
> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > Commit-Queue: Miguel Casas <mcasas@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#616196}
> >
> > Change-Id: If37bfd2d1f24b9b833bf8b291363b5c19861a6c7
> > Reviewed-on: https://chromium-review.googlesource.com/c/1377479
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#617781}
>
> TBR=mcasas@chromium.org,hiroh@chromium.org,dstaessens@chromium.org
>
> Change-Id: Ic4b1e7e8de8403173f71a415094ed23cab51f5e4
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/1385689
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#618085}

Change-Id: I96f758b3f99032db219ffce1d432c348e6a30e90
Reviewed-on: https://chromium-review.googlesource.com/c/1385728
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618257}
[modify] https://crrev.com/5c66ed12a80d9dce342314fbc03240cb44cd3f0b/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/5c66ed12a80d9dce342314fbc03240cb44cd3f0b/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/5c66ed12a80d9dce342314fbc03240cb44cd3f0b/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

Components: Internals>GPU>Video OS>Kernel>Video
Labels: GPU-Intel
Owner: dstaessens@chromium.org
Reassigning to dstaessens@ who did most of the above CLs,
dstaessens@, can we mark this bug as Fixed now ?
Status: Fixed (was: Assigned)
While there is always room for improvement, I think we can close this one now :-)

Sign in to add a comment