Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10843058:
VAVDA: Destroy vaapi buffers after we're done drawing. (Closed)
|
Created:
8 years, 4 months ago by marcheu Modified:
8 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, feature-media-reviews_chromium.org Base URL:
https://git.chromium.org/git/chromium/src@master Visibility:
Public. |
DescriptionVAVDA: Cleanups and fixes.
This CL does some cleanups and fixes:
- Destroy vaapi buffers after we're done drawing.If we do this before we're done drawing, the memory can be
reclaimed by other threads in the meantime and result in
interesting use-after-free bugs.
- Don't leak buffers when we go through the error path.
- Don't leak buffers when we Reset().
- Fix a small indentation.
BUG=chromium-os:33170, chromium:139755
TEST=by hand
Change-Id: Icd44bd52c5b6c8f32d904ad2980d19a090f01e38
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150010
Patch Set 1 #
Total comments: 6
Patch Set 2 : Patch 2 #Patch Set 3 : Patch 3 #
Total comments: 5
Patch Set 4 : Patch 4, adresses nits. #
Total comments: 1
Patch Set 5 : Also plug Destroy() leaks. #
Total comments: 3
Patch Set 6 : Fix locking v1 #Patch Set 7 : Did the locking in a separate CL. #
Total comments: 1
Patch Set 8 : Take Ami's comments into account. #
Total comments: 2
Patch Set 9 : More cleanups. #Patch Set 10 : Cleanup const placement and stale comment. #Patch Set 11 : Fix character inversion. #
Total comments: 1
Patch Set 12 : Fix MakeCurrent #
Messages
Total messages: 24 (0 generated)
LGTM Is the separation of the buffer sets purely aesthetic or is it doing something I'm missing?
https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... content/common/gpu/media/vaapi_h264_decoder.cc:1035: VA_SUCCESS_OR_RETURN(va_res, "vaRenderPicture for va_bufs failed", false); Can't we have a leak here if we return early because of error? https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... content/common/gpu/media/vaapi_h264_decoder.cc:1050: VA_SUCCESS_OR_RETURN(va_res, "vaRenderPicture for slices failed", false); Same here? https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... content/common/gpu/media/vaapi_h264_decoder.cc:1057: VA_SUCCESS_OR_RETURN(va_res, "vaEndPicture failed", false); And here https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... content/common/gpu/media/vaapi_h264_decoder.cc:1062: VA_SUCCESS_OR_RETURN(va_res, "vaDestroyBuffer for va_bufs failed", false); And here https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... content/common/gpu/media/vaapi_h264_decoder.cc:1067: VA_SUCCESS_OR_RETURN(va_res, "vaDestroyBuffer for slices failed", false); And here
lgtm https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... content/common/gpu/media/vaapi_h264_decoder.cc:1035: VA_SUCCESS_OR_RETURN(va_res, "vaRenderPicture for va_bufs failed", false); On 2012/08/02 23:36:42, piman wrote: > Can't we have a leak here if we return early because of error? TL;DR: yes, and everywhere else you mentioned too. This will cause us to return false, which will be propagated all the way up to VaapiVideoDecodeAccelerator class, which will get kDecodeError and will notify the VDA client of the error and Cleanup() itself, which will result in Destroy()ing this class as well, which frees all VA surfaces, and calls vaDestroyConfig() and vaTerminate(). vaTerminate() will free the buffers allocated with vaCreateBuffer by destroying their buffer_heap (and other heaps too) in i965_Terminate.
On 2012/08/03 02:13:20, posciak wrote: > lgtm > > https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... > File content/common/gpu/media/vaapi_h264_decoder.cc (right): > > https://chromiumcodereview.appspot.com/10843058/diff/1/content/common/gpu/med... > content/common/gpu/media/vaapi_h264_decoder.cc:1035: > VA_SUCCESS_OR_RETURN(va_res, "vaRenderPicture for va_bufs failed", false); > On 2012/08/02 23:36:42, piman wrote: > > Can't we have a leak here if we return early because of error? > > TL;DR: yes, and everywhere else you mentioned too. > Er, I of course meant no, there will be no leak.
New version which takes care of all the leaks I could find, please let me know what you think.
some minor nits. https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:34: #define VA_ERROR_RETURN(va_res, err_msg, ret) \ TBH, I see the value in the macro that logs things, but I don't see the value in folding the return into here. It'd be better to be explicit at the calling sites: VA_LOG_ERROR(va_res, "error"); return false; https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:383: while(!pending_slice_bufs_.empty()) { nit: space before ( https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:389: while(!pending_va_bufs_.empty()) { nit: space before ( https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:1032: va_res = VAAPI_DestroyBuffer(va_display_, va_buffers[i]); nit: declare va_res here.
https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:1086: pending_slice_bufs_.pop(); Perhaps we should wrap each buffer into a class that calls vaDestroyBuffer() on its destruction and make pending_*_bufs scoped containers (ScopedVectors?) that would call those destructors on clear(). Then we would not pop from here at all, but just clear() the whole container at the end of this function. In all other cases, including intermediate failures in this function, they would clean themselves up when the containers get destroyed in the destructor. Then the only other place we'd have to have an explicit clear of this container would be at Reset(), which we already have (not really needed in Flush(), because it'd either finish the current picture cleaning up or failing and destroying, or not have anything in flight at all).
On 2012/08/03 04:15:02, posciak wrote: > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > File content/common/gpu/media/vaapi_h264_decoder.cc (right): > > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > content/common/gpu/media/vaapi_h264_decoder.cc:1086: pending_slice_bufs_.pop(); > Perhaps we should wrap each buffer into a class that calls vaDestroyBuffer() on > its destruction and make pending_*_bufs scoped containers (ScopedVectors?) that > would call those destructors on clear(). Then we would not pop from here at all, > but just clear() the whole container at the end of this function. In all other > cases, including intermediate failures in this function, they would clean > themselves up when the containers get destroyed in the destructor. Then the only > other place we'd have to have an explicit clear of this container would be at > Reset(), which we already have (not really needed in Flush(), because it'd > either finish the current picture cleaning up or failing and destroying, or not > have anything in flight at all). Ami is also suggesting we could do it the Right (but Harder) Way and add a deleter template parameter to ScopedVector.
On 2012/08/03 16:48:26, posciak wrote: > On 2012/08/03 04:15:02, posciak wrote: > > > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > > File content/common/gpu/media/vaapi_h264_decoder.cc (right): > > > > > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > > content/common/gpu/media/vaapi_h264_decoder.cc:1086: > pending_slice_bufs_.pop(); > > Perhaps we should wrap each buffer into a class that calls vaDestroyBuffer() > on > > its destruction and make pending_*_bufs scoped containers (ScopedVectors?) > that > > would call those destructors on clear(). Then we would not pop from here at > all, > > but just clear() the whole container at the end of this function. In all other > > cases, including intermediate failures in this function, they would clean > > themselves up when the containers get destroyed in the destructor. Then the > only > > other place we'd have to have an explicit clear of this container would be at > > Reset(), which we already have (not really needed in Flush(), because it'd > > either finish the current picture cleaning up or failing and destroying, or > not > > have anything in flight at all). > > Ami is also suggesting we could do it the Right (but Harder) Way and add a > deleter template parameter to ScopedVector. Yes for that specific leak it would fix it, but the other ones definitely need something else, since the objects have to outlive the scope.
On 2012/08/03 04:15:02, posciak wrote: > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > File content/common/gpu/media/vaapi_h264_decoder.cc (right): > > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > content/common/gpu/media/vaapi_h264_decoder.cc:1086: pending_slice_bufs_.pop(); > Perhaps we should wrap each buffer into a class that calls vaDestroyBuffer() on > its destruction and make pending_*_bufs scoped containers (ScopedVectors?) that > would call those destructors on clear(). Then we would not pop from here at all, > but just clear() the whole container at the end of this function. Doing this will affect the behavior of the decoder in the error case, which I didn't want to do. But if you know it's the right thing, we could do that instead. In all other > cases, including intermediate failures in this function, they would clean > themselves up when the containers get destroyed in the destructor. Then the only > other place we'd have to have an explicit clear of this container would be at > Reset(), which we already have (not really needed in Flush(), because it'd > either finish the current picture cleaning up or failing and destroying, or not > have anything in flight at all).
On 2012/08/03 17:25:13, marcheu wrote: > On 2012/08/03 04:15:02, posciak wrote: > > > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > > File content/common/gpu/media/vaapi_h264_decoder.cc (right): > > > > > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > > content/common/gpu/media/vaapi_h264_decoder.cc:1086: > pending_slice_bufs_.pop(); > > Perhaps we should wrap each buffer into a class that calls vaDestroyBuffer() > on > > its destruction and make pending_*_bufs scoped containers (ScopedVectors?) > that > > would call those destructors on clear(). Then we would not pop from here at > all, > > but just clear() the whole container at the end of this function. > > Doing this will affect the behavior of the decoder in the error case, which I > didn't want to do. But if you know it's the right thing, we could do that > instead. > Not sure what you mean here... What kind of error case and what affect how? > In all other > > cases, including intermediate failures in this function, they would clean > > themselves up when the containers get destroyed in the destructor. Then the > only > > other place we'd have to have an explicit clear of this container would be at > > Reset(), which we already have (not really needed in Flush(), because it'd > > either finish the current picture cleaning up or failing and destroying, or > not > > have anything in flight at all).
On 2012/08/03 17:23:49, marcheu wrote: > On 2012/08/03 16:48:26, posciak wrote: > > On 2012/08/03 04:15:02, posciak wrote: > > > > > > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > > > File content/common/gpu/media/vaapi_h264_decoder.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10843058/diff/3002/content/common/gpu/... > > > content/common/gpu/media/vaapi_h264_decoder.cc:1086: > > pending_slice_bufs_.pop(); > > > Perhaps we should wrap each buffer into a class that calls vaDestroyBuffer() > > on > > > its destruction and make pending_*_bufs scoped containers (ScopedVectors?) > > that > > > would call those destructors on clear(). Then we would not pop from here at > > all, > > > but just clear() the whole container at the end of this function. In all > other > > > cases, including intermediate failures in this function, they would clean > > > themselves up when the containers get destroyed in the destructor. Then the > > only > > > other place we'd have to have an explicit clear of this container would be > at > > > Reset(), which we already have (not really needed in Flush(), because it'd > > > either finish the current picture cleaning up or failing and destroying, or > > not > > > have anything in flight at all). > > > > Ami is also suggesting we could do it the Right (but Harder) Way and add a > > deleter template parameter to ScopedVector. > > Yes for that specific leak it would fix it, but the other ones definitely need > something else, since the objects have to outlive the scope. I meant having them in class scope, not function scope.
The Destroy() path needs to clean up the pending_*_buffers as well. FTR: this is a quickfix for an immediate bug, we will post a proper scoped solution later. https://chromiumcodereview.appspot.com/10843058/diff/2005/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/2005/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:1058: VA_SUCCESS_OR_RETURN(va_res, "vaBeginPicture failed", false); Actually, here we've already created va buffers, so they still have to be deleted. That means you need to delete the pending_*_buffers in Destroy() as well, like you do in Reset(). Same thing applies at line 1072, where the other queue of pending buffers is not cleaned up.
On 2012/08/03 18:06:39, posciak wrote: > The Destroy() path needs to clean up the pending_*_buffers as well. > FTR: this is a quickfix for an immediate bug, we will post a proper scoped > solution later. > > https://chromiumcodereview.appspot.com/10843058/diff/2005/content/common/gpu/... > File content/common/gpu/media/vaapi_h264_decoder.cc (right): > > https://chromiumcodereview.appspot.com/10843058/diff/2005/content/common/gpu/... > content/common/gpu/media/vaapi_h264_decoder.cc:1058: > VA_SUCCESS_OR_RETURN(va_res, "vaBeginPicture failed", false); > Actually, here we've already created va buffers, so they still have to be > deleted. That means you need to delete the pending_*_buffers in Destroy() as > well, like you do in Reset(). Same thing applies at line 1072, where the other > queue of pending buffers is not cleaned up. Ok, I added that; please take another look. Hopefully we make it before 22 branches :)
https://chromiumcodereview.appspot.com/10843058/diff/10002/content/common/gpu... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/10002/content/common/gpu... content/common/gpu/media/vaapi_h264_decoder.cc:34: #define VA_LOG_ERROR(va_res, err_msg) \ It would be easier to see how these macros differ if the above two used this one for their logging. https://chromiumcodereview.appspot.com/10843058/diff/10002/content/common/gpu... content/common/gpu/media/vaapi_h264_decoder.cc:615: if (va_res != VA_STATUS_SUCCESS) { FWIW, this could have been a simpler diff by using the comma operator in the return value macro field: VA_SUCCESS_OR_RETURN( va_res, "vaCreateContext failed", (VAAPI_DestroySurfaces(va_display_, va_surface_ids_, GetRequiredNumOfPictures())), false)); https://chromiumcodereview.appspot.com/10843058/diff/10002/content/common/gpu... content/common/gpu/media/vaapi_h264_decoder.cc:1068: for (size_t i = 0; i < num_va_buffers && i < kMaxVABuffers; ++i) { ScopedClosureRunner va_buffers_deleter( base::Bind(&VaapiH264Decoder::DestroyBuffers, this, va_buffers, num_va_buffers)); lets you delete lines 1077, 1096, and 1110. Doing the same under the decl of slice_buffers would allow you to remove the rest of the DestroyBuffers calls below, and re-collapse back to VA_SUCCESS_OR_RETURN.
lgtm https://chromiumcodereview.appspot.com/10843058/diff/14003/content/common/gpu... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/14003/content/common/gpu... content/common/gpu/media/vaapi_h264_decoder.cc:429: DestroyPendingBuffers(); This should only be required in one of the three above states, not this, but shouldn't hurt to have it here.
https://chromiumcodereview.appspot.com/10843058/diff/10003/content/common/gpu... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/10003/content/common/gpu... content/common/gpu/media/vaapi_h264_decoder.cc:612: GetRequiredNumOfPictures()), false)); Let's not go overboard here, this is really unreadable IMO - a () expression with side effects that's executed conditionally from inside a macro? How about instead: if (va_res != VA_STATUS_SUCCESS) { DVLOG(1) << ... VAAPI_DestroySurfaces(va_display_, va_surface_ids_, GetRequiredNumOfPictures()); return false; }
https://chromiumcodereview.appspot.com/10843058/diff/10003/content/common/gpu... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/10003/content/common/gpu... content/common/gpu/media/vaapi_h264_decoder.cc:612: GetRequiredNumOfPictures()), false)); On 2012/08/03 22:29:46, piman wrote: > Let's not go overboard here, this is really unreadable IMO - a () expression > with side effects that's executed conditionally from inside a macro? > > How about instead: > if (va_res != VA_STATUS_SUCCESS) { > DVLOG(1) << ... > VAAPI_DestroySurfaces(va_display_, va_surface_ids_, > GetRequiredNumOfPictures()); > return false; > } Hah :) That's what was there when I suggested the comma operator. I actually think this is *more* readable this way, but I don't think it's worth arguing over (I made my original suggestion as a "FWIW" but I should have named it a "nit"). Do what y'all like.
More cleanups, let me know!
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/marcheu@chromium.org/10843058/9003
lgtm with an informative comment. https://chromiumcodereview.appspot.com/10843058/diff/9003/content/common/gpu/... File content/common/gpu/media/vaapi_h264_decoder.cc (right): https://chromiumcodereview.appspot.com/10843058/diff/9003/content/common/gpu/... content/common/gpu/media/vaapi_h264_decoder.cc:611: DVLOG(1) << "Error creating a decoding surface (binding to texture?)"; FWIW this is not really binding to textures yet... it's done later on in the constructor of DecodeSurface.
Change committed as 150010 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
