Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
114853002:
media: Enabling direct rendering for VP9 (Closed)
|
Created:
7 years ago by vignesh Modified:
6 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionmedia: Enabling direct rendering for VP9
Passing in our own buffers to libvpx for VP9 decoding so that the
copying of frames after decoding is eliminated.
Note: This depends on a few changes in libvpx. Do NOT commit this until a libvpx roll happens that contains the necessary changes.
test=media_unittests
BUG=321856
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252380
Patch Set 1 #Patch Set 2 : adding some comments. #
Total comments: 10
Patch Set 3 : comments #
Total comments: 8
Patch Set 4 : addressing comments #
Total comments: 10
Patch Set 5 : adding MemoryPool class #Patch Set 6 : adding LRU codec ctl #
Total comments: 8
Patch Set 7 : addressing comments #Patch Set 8 : fixing the case when videoframes are destroyed out of order #
Total comments: 6
Patch Set 9 : addressing comments and adding unit tests #Patch Set 10 : get/release model for frame bufers. rebase. #
Total comments: 20
Patch Set 11 : addressing comments #
Total comments: 6
Patch Set 12 : nit fixes #Patch Set 13 : rebase and minor nit changes due to change in library #Patch Set 14 : fixing trybot failure #
Messages
Total messages: 39 (0 generated)
Sorry I thought I published my comments on Friday. https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:34: #include "third_party/libvpx/source/libvpx/vpx/vpx_external_frame_buffer.h" This include should be before vp8dx https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:164: if (config.codec() == kCodecVP9) { You are going to have to add a check of the ABI here for the correct version of libvpx. Otherwise this will most likely break use system libvpx. https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:165: frame_buffers_ = new vpx_codec_frame_buffer[kVP9MaxFrameBuffers](); parenthesis are not needed. https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.h (right): https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:52: vpx_codec_frame_buffer *fb); '*' should be next to the type in this file (I know in libvpx it is the other way) https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:64: // decoding. 8 (libvpx reference buffers) + 4 (gitter buffers) + 1 (work jitter
https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:34: #include "third_party/libvpx/source/libvpx/vpx/vpx_external_frame_buffer.h" On 2013/12/16 19:38:43, fgalligan1 wrote: > This include should be before vp8dx Done. https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:164: if (config.codec() == kCodecVP9) { On 2013/12/16 19:38:43, fgalligan1 wrote: > You are going to have to add a check of the ABI here for the correct version of > libvpx. Otherwise this will most likely break use system libvpx. Done. https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:165: frame_buffers_ = new vpx_codec_frame_buffer[kVP9MaxFrameBuffers](); On 2013/12/16 19:38:43, fgalligan1 wrote: > parenthesis are not needed. new operator without paranthesis will not zero initialize the memory. https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.h (right): https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:52: vpx_codec_frame_buffer *fb); On 2013/12/16 19:38:43, fgalligan1 wrote: > '*' should be next to the type in this file (I know in libvpx it is the other > way) Done. https://codereview.chromium.org/114853002/diff/20001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:64: // decoding. 8 (libvpx reference buffers) + 4 (gitter buffers) + 1 (work On 2013/12/16 19:38:43, fgalligan1 wrote: > jitter whoops. done.
https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.h (right): https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:51: static int32 ReallocVP9FrameBuffer(void* user_priv, size_t new_size, nit: I think we use int other places. https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:65: // buffer) = 13. These have to be static const int. https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:66: const int kVP9MaxFrameBuffers = 13; I think you can move this to the .cc file. Or are you planning on accessing this value somewhere else? I think it would be good if you include media/base/limits.h, so this can be 9 (ref buffers + work buffer) + kMaxVideoFrames; https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:69: const int kVP9MinABIVersion = 6; Move this to the .cc and I would change the name to something like kLibvpxMinAbiExternalBuffers.
On 2013/12/17 01:16:53, fgalligan1 wrote: > https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... > File media/filters/vpx_video_decoder.h (right): > > https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... > media/filters/vpx_video_decoder.h:51: static int32 ReallocVP9FrameBuffer(void* > user_priv, size_t new_size, > nit: I think we use int other places. > > https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... > media/filters/vpx_video_decoder.h:65: // buffer) = 13. > These have to be static const int. > > https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... > media/filters/vpx_video_decoder.h:66: const int kVP9MaxFrameBuffers = 13; > I think you can move this to the .cc file. Or are you planning on accessing this > value somewhere else? > > I think it would be good if you include media/base/limits.h, so this can be 9 > (ref buffers + work buffer) + kMaxVideoFrames; > > https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... > media/filters/vpx_video_decoder.h:69: const int kVP9MinABIVersion = 6; > Move this to the .cc and I would change the name to something like > kLibvpxMinAbiExternalBuffers. Also Andrew said we might need someone else to review as he is swamped.
Adding acolwell@ as reviewer as Andrew is busy. Please add someone else if you think they are better suited to review this. https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.h (right): https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:51: static int32 ReallocVP9FrameBuffer(void* user_priv, size_t new_size, On 2013/12/17 01:16:54, fgalligan1 wrote: > nit: I think we use int other places. i tried int first. clang was not happy as libvpx expects a function with this signature. https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:65: // buffer) = 13. On 2013/12/17 01:16:54, fgalligan1 wrote: > These have to be static const int. Done. https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:66: const int kVP9MaxFrameBuffers = 13; On 2013/12/17 01:16:54, fgalligan1 wrote: > I think you can move this to the .cc file. Or are you planning on accessing this > value somewhere else? > > I think it would be good if you include media/base/limits.h, so this can be 9 > (ref buffers + work buffer) + kMaxVideoFrames; > Done. https://codereview.chromium.org/114853002/diff/40001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:69: const int kVP9MinABIVersion = 6; On 2013/12/17 01:16:54, fgalligan1 wrote: > Move this to the .cc and I would change the name to something like > kLibvpxMinAbiExternalBuffers. Done.
https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:75: static const int kVP9MaxFrameBuffers = 9 + limits::kMaxVideoFrames; Shouldn't libvpx advertise the minimum required number of frame buffers it needs instead of us hardcoding it here? https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:143: delete[] fb->data; This is not safe. libvpx only knows when it no longer needs this reference. It has no clue about whether a VideoFrame is still around that contains pointers into this data. https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:200: if (frame_buffers_) { This is not safe. It assumes that all VideoFrames have been destroyed when the CloseDecoder() method has been called. This is definitely not true when MediaSource is used and there are config changes. Frames will likely outlive the decoder instance. You need to provide a closure to WrapExternalYuvData so that you can determine when all the frames have been released. https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:392: if (!vpx_codec_alpha_ && Why isn't alpha included in the 0-copy code path? https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.h (right): https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:97: vpx_codec_frame_buffer* frame_buffers_; nit: Use scoped_vector<vpx_codec_frame_buffer>
acolwell@, thanks for the patch that you sent in email. PS5 is mostly based on that. PTAL. https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:75: static const int kVP9MaxFrameBuffers = 9 + limits::kMaxVideoFrames; On 2013/12/17 19:07:03, acolwell wrote: > Shouldn't libvpx advertise the minimum required number of frame buffers it needs > instead of us hardcoding it here? Done. https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:143: delete[] fb->data; On 2013/12/17 19:07:03, acolwell wrote: > This is not safe. libvpx only knows when it no longer needs this reference. It > has no clue about whether a VideoFrame is still around that contains pointers > into this data. The way libvpx designed to do this is under the following assumption: * libvpx needs 9 buffers (8 reference buffers + 1 work buffer) for decoding VP9. * the application passes in 9 + x buffers to libvpx, where x is the number of buffers (frames) that the application expects to hold on to after decoding. libvpx ensures that it won't call Realloc (or overwrite) on the x most recent frames. So assuming that's how chromium works (it does not hold on to more than kMaxVideoFrames), this behavior is correct. If not, we will have to fix it. https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:200: if (frame_buffers_) { On 2013/12/17 19:07:03, acolwell wrote: > This is not safe. It assumes that all VideoFrames have been destroyed when the > CloseDecoder() method has been called. This is definitely not true when > MediaSource is used and there are config changes. Frames will likely outlive the > decoder instance. You need to provide a closure to WrapExternalYuvData so that > you can determine when all the frames have been released. Addressing this issue by introducing MemoryPool class as you suggested. https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:392: if (!vpx_codec_alpha_ && On 2013/12/17 19:07:03, acolwell wrote: > Why isn't alpha included in the 0-copy code path? libvpx takes in external buffers only for VP9 as of now. So we have to do a copy for VP8 (as alpha is VP8 only). https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.h (right): https://codereview.chromium.org/114853002/diff/60001/media/filters/vpx_video_... media/filters/vpx_video_decoder.h:97: vpx_codec_frame_buffer* frame_buffers_; On 2013/12/17 19:07:03, acolwell wrote: > nit: Use scoped_vector<vpx_codec_frame_buffer> not applicable anymore. Done.
Looks pretty good. Just a few more comments. https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (left): https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:364: if (!vpx_codec_alpha_) You should probably leave this in here since the memory_pool is only allocated for VP9 w/ the right ABI and this code would break if we ever decided to use this decoder for VP8. https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:108: vpx_codec_frame_buffer frame_buffers_[kVP9MaxFrameBuffers]; Add DISALLOW_COPY_AND_ASSIGN(MemoryPool). https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:217: VPX_DECODER_ABI_VERSION >= kLibvpxMinABIExternalBuffers) { Is this really needed? Can this code even build w/ a lower ABI version. If so, it seems like a COMPILER_ASSERT() by the #includes is what you want instead of this runtime check. https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:441: vpx_image->stride[VPX_PLANE_Y], It would be nice if vpx_image contained a vpx_codec_frame_buffer* that points to the buffer that is backing this image. It would also be nice if vpx_codec_frame_buffer had an "in use by libvpx" field and an "in use by external code" field so that these two pieces of code can clearly communicate when the frame buffer data can safely be destroyed. This may allow you to remove the LRU assumption. libvpx just looks for a frame buffer that isn't in use by libvpx or external code.
On 2013/12/19 15:26:12, acolwell wrote: > Looks pretty good. Just a few more comments. > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > File media/filters/vpx_video_decoder.cc (left): > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > media/filters/vpx_video_decoder.cc:364: if (!vpx_codec_alpha_) > You should probably leave this in here since the memory_pool is only allocated > for VP9 w/ the right ABI and this code would break if we ever decided to use > this decoder for VP8. > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > File media/filters/vpx_video_decoder.cc (right): > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > media/filters/vpx_video_decoder.cc:108: vpx_codec_frame_buffer > frame_buffers_[kVP9MaxFrameBuffers]; > Add DISALLOW_COPY_AND_ASSIGN(MemoryPool). > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > media/filters/vpx_video_decoder.cc:217: VPX_DECODER_ABI_VERSION >= > kLibvpxMinABIExternalBuffers) { > Is this really needed? Can this code even build w/ a lower ABI version. If so, > it seems like a COMPILER_ASSERT() by the #includes is what you want instead of > this runtime check. > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > media/filters/vpx_video_decoder.cc:441: vpx_image->stride[VPX_PLANE_Y], > It would be nice if vpx_image contained a vpx_codec_frame_buffer* that points to > the buffer that is backing this image. It would also be nice if > vpx_codec_frame_buffer had an "in use by libvpx" field and an "in use by > external code" field so that these two pieces of code can clearly communicate > when the frame buffer data can safely be destroyed. This may allow you to remove > the LRU assumption. libvpx just looks for a frame buffer that isn't in use by > libvpx or external code. We talked yesterday about vpx_codec_frame_buffer some, which would have been our second choice to address the video frames outliving the decoder issue. I think we had it as our second choice more because it would involve changing the struct. We will meet with more parties that are involved with the API and come to a decision if we want to go with what we have or add explicit signaling on which fb was used. As for the explicit signaling of which frames are in use by the application and which frames are in use by libvpx. We struggled with which way to go in libvpx. We went with the LRU in the end because it made the API simpler. Our implementation can still be a little smarter as libvpx knows how many references the encoder has used. And we were planning on changing the implementation soon, when we are going to do some work on mobile. I know the implicit signaling of the application jitter buffers is fragile, but even in the explicit case libvpx cannot give out more than jitter buffer to the application in case libvpx gets a frame that adds a new reference. In that case libvpx may run out and have to return error or try_again, same as the implicit case of the application breaking the implicit jitter buffer number. I also don't know if there will be any value to the application knowing which buffers libvpx has in use, once we move to a better LRU. Anyway we will meet again today and talk about this, because if we change it I would want to do it now rather than later.
On 2013/12/19 17:56:26, fgalligan1 wrote: > On 2013/12/19 15:26:12, acolwell wrote: > > Looks pretty good. Just a few more comments. > > > > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > > File media/filters/vpx_video_decoder.cc (left): > > > > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > > media/filters/vpx_video_decoder.cc:364: if (!vpx_codec_alpha_) > > You should probably leave this in here since the memory_pool is only allocated > > for VP9 w/ the right ABI and this code would break if we ever decided to use > > this decoder for VP8. > > > > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > > File media/filters/vpx_video_decoder.cc (right): > > > > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > > media/filters/vpx_video_decoder.cc:108: vpx_codec_frame_buffer > > frame_buffers_[kVP9MaxFrameBuffers]; > > Add DISALLOW_COPY_AND_ASSIGN(MemoryPool). > > > > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > > media/filters/vpx_video_decoder.cc:217: VPX_DECODER_ABI_VERSION >= > > kLibvpxMinABIExternalBuffers) { > > Is this really needed? Can this code even build w/ a lower ABI version. If so, > > it seems like a COMPILER_ASSERT() by the #includes is what you want instead of > > this runtime check. > > > > > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > > media/filters/vpx_video_decoder.cc:441: vpx_image->stride[VPX_PLANE_Y], > > It would be nice if vpx_image contained a vpx_codec_frame_buffer* that points > to > > the buffer that is backing this image. It would also be nice if > > vpx_codec_frame_buffer had an "in use by libvpx" field and an "in use by > > external code" field so that these two pieces of code can clearly communicate > > when the frame buffer data can safely be destroyed. This may allow you to > remove > > the LRU assumption. libvpx just looks for a frame buffer that isn't in use by > > libvpx or external code. > > We talked yesterday about vpx_codec_frame_buffer some, which would have been our > second choice to address the video frames outliving the decoder issue. I think > we had it as our second choice more because it would involve changing the > struct. We will meet with more parties that are involved with the API and come > to a decision if we want to go with what we have or add explicit signaling on > which fb was used. > > As for the explicit signaling of which frames are in use by the application and > which frames are in use by libvpx. We struggled with which way to go in libvpx. > We went with the LRU in the end because it made the API simpler. Our > implementation can still be a little smarter as libvpx knows how many references > the encoder has used. And we were planning on changing the implementation soon, > when we are going to do some work on mobile. I know the implicit signaling of > the application jitter buffers is fragile, but even in the explicit case libvpx > cannot give out more than jitter buffer to the application in case libvpx gets a > frame that adds a new reference. In that case libvpx may run out and have to > return error or try_again, same as the implicit case of the application breaking > the implicit jitter buffer number. I also don't know if there will be any value > to the application knowing which buffers libvpx has in use, once we move to a > better LRU. Anyway we will meet again today and talk about this, because if we > change it I would want to do it now rather than later. We talked about it and at this point we are going to keep the api the same. If we need to add the fb output to the ximage we can do that later fairly easy. As for the application/libvpx signaling, we don't really see a benefit after we switch to a smarter LRU. Basically in all cases the jitter buffer should be full as well as the number of buffers libvpx is using (I.e. 5). In any case where those buffers are not full most likely the application will have decode performance issues. That being said we are planning to make an incompatible release (2.0) sometime later this year. If we find non-critical shortcomings in the current API we can make changes then.
https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (left): https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:364: if (!vpx_codec_alpha_) On 2013/12/19 15:26:12, acolwell wrote: > You should probably leave this in here since the memory_pool is only allocated > for VP9 w/ the right ABI and this code would break if we ever decided to use > this decoder for VP8. Done. https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:108: vpx_codec_frame_buffer frame_buffers_[kVP9MaxFrameBuffers]; On 2013/12/19 15:26:12, acolwell wrote: > Add DISALLOW_COPY_AND_ASSIGN(MemoryPool). Done. https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:217: VPX_DECODER_ABI_VERSION >= kLibvpxMinABIExternalBuffers) { On 2013/12/19 15:26:12, acolwell wrote: > Is this really needed? Can this code even build w/ a lower ABI version. If so, > it seems like a COMPILER_ASSERT() by the #includes is what you want instead of > this runtime check. We don't need this. Getting rid of it. https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:441: vpx_image->stride[VPX_PLANE_Y], On 2013/12/19 15:26:12, acolwell wrote: > It would be nice if vpx_image contained a vpx_codec_frame_buffer* that points to > the buffer that is backing this image. It would also be nice if > vpx_codec_frame_buffer had an "in use by libvpx" field and an "in use by > external code" field so that these two pieces of code can clearly communicate > when the frame buffer data can safely be destroyed. This may allow you to remove > the LRU assumption. libvpx just looks for a frame buffer that isn't in use by > libvpx or external code. frank's comments address this.
On 2013/12/19 20:27:20, vignesh wrote: > https://codereview.chromium.org/114853002/diff/100001/media/filters/vpx_video... > media/filters/vpx_video_decoder.cc:441: vpx_image->stride[VPX_PLANE_Y], > On 2013/12/19 15:26:12, acolwell wrote: > > It would be nice if vpx_image contained a vpx_codec_frame_buffer* that points > to > > the buffer that is backing this image. It would also be nice if > > vpx_codec_frame_buffer had an "in use by libvpx" field and an "in use by > > external code" field so that these two pieces of code can clearly communicate > > when the frame buffer data can safely be destroyed. This may allow you to > remove > > the LRU assumption. libvpx just looks for a frame buffer that isn't in use by > > libvpx or external code. > > frank's comments address this. I don't believe the current code is sufficient. LRU assumes that the frames are released in order. This is definitely not the case if the compositor grabs the reference for a frame and then while the composite is happening, we determine that we are behind and need to drop some frames. This will cause the VideoFrames to get destroyed out of order and successive decodes cannot assume that the oldest frame is not still referenced. If you still want to use LRU, then you'll have to add code that prevents decodes until you know that the "oldest" VideoFrame according to libvpx has been destroyed.
acolwell@, I have addressed the case when videoframes are destroyed out of order. PTAL. I have tested this manually, working on adding some tests as well.
lgtm % minor comments. https://codereview.chromium.org/114853002/diff/140001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/140001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:128: sizeof(vpx_codec_frame_buffer) * arraysize(frame_buffers_)); nit: I believe you should be able to just use sizeof(frame_buffers_) here. https://codereview.chromium.org/114853002/diff/140001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:160: if (decoder_->pending_buffer_ && IsSafeToCallDecoder()) { You'll need a decoder_ null check and a way for the decoder to clear decoder_ when the decoder clears its reference to the pool (ie when it creates a new pool or when the decoder destructor runs. If you don't you could run into use-after-free issues when frames, that live longer than the decoder, get destroyed. https://codereview.chromium.org/114853002/diff/140001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:282: memory_pool_ = NULL; I think right before this line is where you need to call a method that clears memory_pool_->decoder_
Aaron, I have added the unittests. PTAL. https://codereview.chromium.org/114853002/diff/140001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/140001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:128: sizeof(vpx_codec_frame_buffer) * arraysize(frame_buffers_)); On 2013/12/20 17:26:44, acolwell wrote: > nit: I believe you should be able to just use sizeof(frame_buffers_) here. I tried that before i used this, it did not work. https://codereview.chromium.org/114853002/diff/140001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:160: if (decoder_->pending_buffer_ && IsSafeToCallDecoder()) { On 2013/12/20 17:26:44, acolwell wrote: > You'll need a decoder_ null check and a way for the decoder to clear decoder_ > when the decoder clears its reference to the pool (ie when it creates a new pool > or when the decoder destructor runs. If you don't you could run into > use-after-free issues when frames, that live longer than the decoder, get > destroyed. nice catch. Done. https://codereview.chromium.org/114853002/diff/140001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:282: memory_pool_ = NULL; On 2013/12/20 17:26:44, acolwell wrote: > I think right before this line is where you need to call a method that clears > memory_pool_->decoder_ Done.
so.. I have some bad news for you. During a lunch conversation about your CL, Ami and I realized that LRU won't ultimately work. In the presence of dropped frames you can always lockup the pipeline. WebMediaPlayerImpl always holds onto 1 frame so that it can use it to repaint the element anytime the compositor needs to. If we get into a dropped frame situation where all the other frames get dropped, the pipeline will deadlock because: 1. The VpxDecoder needs the frame back to proceed with decoding 2. WebMediaPlayerImpl needs a new frame before it is able to release its reference to the frame VpxDecoder needs to complete the next decode. Therefore, I am requesting again that a mechanism be added to vpx_codec_frame_buffer that allows external code to tell libvpx that a particular buffer is still in use by the caller.
Unfortunately this means not lgtm.
On 2013/12/20 21:28:57, acolwell wrote: > so.. I have some bad news for you. During a lunch conversation about your CL, > Ami and I realized that LRU won't ultimately work. In the presence of dropped > frames you can always lockup the pipeline. WebMediaPlayerImpl always holds onto > 1 frame so that it can use it to repaint the element anytime the compositor > needs to. If we get into a dropped frame situation where all the other frames > get dropped, the pipeline will deadlock because: > 1. The VpxDecoder needs the frame back to proceed with decoding > 2. WebMediaPlayerImpl needs a new frame before it is able to release its > reference to the frame VpxDecoder needs to complete the next decode. > > Therefore, I am requesting again that a mechanism be added to > vpx_codec_frame_buffer that allows external code to tell libvpx that a > particular buffer is still in use by the caller. OK, we will change the API.
On 2013/12/21 00:14:34, fgalligan1 wrote: > On 2013/12/20 21:28:57, acolwell wrote: > > so.. I have some bad news for you. During a lunch conversation about your CL, > > Ami and I realized that LRU won't ultimately work. In the presence of dropped > > frames you can always lockup the pipeline. WebMediaPlayerImpl always holds > onto > > 1 frame so that it can use it to repaint the element anytime the compositor > > needs to. If we get into a dropped frame situation where all the other frames > > get dropped, the pipeline will deadlock because: > > 1. The VpxDecoder needs the frame back to proceed with decoding > > 2. WebMediaPlayerImpl needs a new frame before it is able to release its > > reference to the frame VpxDecoder needs to complete the next decode. > > > > Therefore, I am requesting again that a mechanism be added to > > vpx_codec_frame_buffer that allows external code to tell libvpx that a > > particular buffer is still in use by the caller. > > OK, we will change the API. Thank you. I appreciate it. :)
On 2013/12/21 00:24:38, acolwell wrote: > On 2013/12/21 00:14:34, fgalligan1 wrote: > > On 2013/12/20 21:28:57, acolwell wrote: > > > so.. I have some bad news for you. During a lunch conversation about your > CL, > > > Ami and I realized that LRU won't ultimately work. In the presence of > dropped > > > frames you can always lockup the pipeline. WebMediaPlayerImpl always holds > > onto > > > 1 frame so that it can use it to repaint the element anytime the compositor > > > needs to. If we get into a dropped frame situation where all the other > frames > > > get dropped, the pipeline will deadlock because: > > > 1. The VpxDecoder needs the frame back to proceed with decoding > > > 2. WebMediaPlayerImpl needs a new frame before it is able to release its > > > reference to the frame VpxDecoder needs to complete the next decode. > > > > > > Therefore, I am requesting again that a mechanism be added to > > > vpx_codec_frame_buffer that allows external code to tell libvpx that a > > > particular buffer is still in use by the caller. > > > > OK, we will change the API. > > Thank you. I appreciate it. :) Aaron, Frank has made the necessary changes to libvpx to implement the get/release model for external frame buffers. I have made the changes to chromium accordingly. PTAL now. :)
Looks pretty good. Here are my initial comments. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:83: struct VP9FrameBuffer { nit: Make this decl private inside the MemoryPool since nothing outside the pool really needs to be aware of it. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:103: // Callback that will be called by libvpx when the frame buffer is no more nit: s/more/longer/ https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:147: if (user_priv == NULL || fb == NULL) nit: These look like they should be DCHECKs since I think it's likely a bug in libvpx if these are NULL. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:152: Put the rest of this method in a private method and just call that method here so you can get rid of all the memory_pool references. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:155: for (i = 0; i < memory_pool->frame_buffers_.size(); ++i) { nit: s/i = 0// since you initialize it on the line above. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:171: delete[] memory_pool->frame_buffers_[i]->data; Just use std::vector<uint8> for data so you can replace this and the 4 lines below with frame_buffers_[i]->resize(min_size). https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:190: --frame_buffer->ref_cnt; Is this method and GetVP9FrameBuffer() ALWAYS called on the same thread as vpx_codec_decode()? If not, then this isn't thread-safe. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:194: base::Closure VpxVideoDecoder::MemoryPool::frame_callback( nit: This isn't a trivial accessor so it should have a CamelCase name like CreateFrameCallback(). https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:218: memory_pool_(NULL) { nit: You don't need this for a scoped_refptr. It is already initialized to null. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:516: memory_pool_->frame_callback( nit: Change the signature of this method so you don't need the cast here. It's a little cleaner to have the cast inside the method since the MemoryPool is really the one that "knows" this is a VP9FrameBuffer*.
Have addressed all the comments. PTAL. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:83: struct VP9FrameBuffer { On 2014/02/04 00:48:35, acolwell wrote: > nit: Make this decl private inside the MemoryPool since nothing outside the pool > really needs to be aware of it. Done. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:103: // Callback that will be called by libvpx when the frame buffer is no more On 2014/02/04 00:48:35, acolwell wrote: > nit: s/more/longer/ Done. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:147: if (user_priv == NULL || fb == NULL) On 2014/02/04 00:48:35, acolwell wrote: > nit: These look like they should be DCHECKs since I think it's likely a bug in > libvpx if these are NULL. Done. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:152: On 2014/02/04 00:48:35, acolwell wrote: > Put the rest of this method in a private method and just call that method here > so you can get rid of all the memory_pool references. Done. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:155: for (i = 0; i < memory_pool->frame_buffers_.size(); ++i) { On 2014/02/04 00:48:35, acolwell wrote: > nit: s/i = 0// since you initialize it on the line above. Done. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:171: delete[] memory_pool->frame_buffers_[i]->data; On 2014/02/04 00:48:35, acolwell wrote: > Just use std::vector<uint8> for data so you can replace this and the 4 lines > below with frame_buffers_[i]->resize(min_size). Done. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:190: --frame_buffer->ref_cnt; On 2014/02/04 00:48:35, acolwell wrote: > Is this method and GetVP9FrameBuffer() ALWAYS called on the same thread as > vpx_codec_decode()? If not, then this isn't thread-safe. yes, both the methods are always called on the same thread as vpx_codec_decode. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:194: base::Closure VpxVideoDecoder::MemoryPool::frame_callback( On 2014/02/04 00:48:35, acolwell wrote: > nit: This isn't a trivial accessor so it should have a CamelCase name like > CreateFrameCallback(). Done. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:218: memory_pool_(NULL) { On 2014/02/04 00:48:35, acolwell wrote: > nit: You don't need this for a scoped_refptr. It is already initialized to null. Done. https://codereview.chromium.org/114853002/diff/290001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:516: memory_pool_->frame_callback( On 2014/02/04 00:48:35, acolwell wrote: > nit: Change the signature of this method so you don't need the cast here. It's a > little cleaner to have the cast inside the method since the MemoryPool is really > the one that "knows" this is a VP9FrameBuffer*. Done.
lgtm % nits https://codereview.chromium.org/114853002/diff/350001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/350001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:133: for (size_t i = 0; i < frame_buffers_.size(); ++i) { nit: Use STLDeleteElements() from base/stl_util.h here instead. https://codereview.chromium.org/114853002/diff/350001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:164: DCHECK(user_priv != NULL); nit: Remove the != NULL here and below. https://codereview.chromium.org/114853002/diff/350001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:307: if (memory_pool_) nit: remove condition since blindly clearing this isn't a big deal.
thanks for the review Aaron. have addressed the nits. I will wait until the libvpx roll happens before submitting this. https://codereview.chromium.org/114853002/diff/350001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/114853002/diff/350001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:133: for (size_t i = 0; i < frame_buffers_.size(); ++i) { On 2014/02/04 17:47:50, acolwell wrote: > nit: Use STLDeleteElements() from base/stl_util.h here instead. Done. https://codereview.chromium.org/114853002/diff/350001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:164: DCHECK(user_priv != NULL); On 2014/02/04 17:47:50, acolwell wrote: > nit: Remove the != NULL here and below. Done. https://codereview.chromium.org/114853002/diff/350001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:307: if (memory_pool_) On 2014/02/04 17:47:50, acolwell wrote: > nit: remove condition since blindly clearing this isn't a big deal. Done.
I can't seem to find the external buffers implmentation, but can you elaborate for posterity on why you didn't take an approach similar to how FFmpegVideoDecoder works? I.e., allocate a VideoFrame and then point libvpx's internal buffers at the inner memory for that VideoFrame?
On 2014/02/04 20:25:12, DaleCurtis wrote: > I can't seem to find the external buffers implmentation, It's a WIP on the libvpx side: CL here: https://gerrit.chromium.org/gerrit/#/c/68554/ > but can you elaborate > for posterity on why you didn't take an approach similar to how > FFmpegVideoDecoder works? > > I.e., allocate a VideoFrame and then point libvpx's internal buffers at the > inner memory for that VideoFrame? My understanding is not 100%, so the explanation may not be accurate. I'll leave it to fgalligan@ to address this properly. From my understanding, the way libvpx is structured is, it has internal buffers where there is no notion of planes (which it uses as reference buffers). So it expects a single block of contiguous memory where it does the decoding. It then transfers it to a structure which has to notion of planes (which is what ultimately comes out of vpx_codec_dec call). Changing that would require a lot of compatibility breaking changes in libvpx.
libvpx has planes underneath, which we do expect to be contiguous. Even if we were to change the code to work on independent planes, I don't see what it buys us by sending the characteristics of how we want the YUV plane allocated vs asking for one pointer with a size. On Tue, Feb 4, 2014 at 1:45 PM, <vigneshv@chromium.org> wrote: > On 2014/02/04 20:25:12, DaleCurtis wrote: > >> I can't seem to find the external buffers implmentation, >> > > It's a WIP on the libvpx side: CL here: > https://gerrit.chromium.org/gerrit/#/c/68554/ > > > but can you elaborate >> for posterity on why you didn't take an approach similar to how >> FFmpegVideoDecoder works? >> > > I.e., allocate a VideoFrame and then point libvpx's internal buffers at >> the >> inner memory for that VideoFrame? >> > > My understanding is not 100%, so the explanation may not be accurate. I'll > leave > it to fgalligan@ to address this properly. > > From my understanding, the way libvpx is structured is, it has internal > buffers > where there is no notion of planes (which it uses as reference buffers). > So it > expects a single block of contiguous memory where it does the decoding. It > then > transfers it to a structure which has to notion of planes (which is what > ultimately comes out of vpx_codec_dec call). Changing that would require a > lot > of compatibility breaking changes in libvpx. > > https://codereview.chromium.org/114853002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Does the transfer into the planar format require a copy?
On 2014/02/05 02:11:39, DaleCurtis wrote: > Does the transfer into the planar format require a copy? No, we just set some pointer and stride values.
rebase and minor changes due to renames in libvpx. functionally still the same. The libvpx roll has landed few minutes ago: https://codereview.chromium.org/168363002 Will land this tomorrow to make sure libvpx roll is stable.
The CQ bit was checked by vigneshv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/114853002/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
The CQ bit was checked by vigneshv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/114853002/760001
Message was sent while issue was closed.
Change committed as 252380
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/175013002/ by rmcilroy@chromium.org. The reason for reverting is: Broke media_perftest on Window's bots. See: https://code.google.com/p/chromium/issues/detail?id=345629 for details.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
