Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1750213002:
Fix Android black frames from MSE config changes. (Closed)
|
Created:
4 years, 9 months ago by chcunningham Modified:
4 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Android black frames during MSE config changes.
The core of the fix/issue is in AVDA. We were previously dismissing
PictureBuffers upon receiving FORMAT_CHANGED from MediaCodec. For
the deferred copy strategy, this had the effect of unhooking the
Texture from PictureBuffers that may already be on the way to the
compositor. With no texture, the compositor would paint black.
With this change, FORMAT_CHANGED will no longer dismiss PictureBuffers.
Instead we update the size of the existing PictureBuffers as they are
used for with the new format. For the copy path we must also update
the size of the 2D texture associated with the PictureBuffer.
BUG=587994
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/338ac3d5778afc85b741bafc778d2301eb272bfa
Cr-Commit-Position: refs/heads/master@{#380305}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Feedback: Adding UpdatePictureBufferSize to strategy #
Total comments: 4
Patch Set 3 : Add logging to copy path #Patch Set 4 : Fix the copy path. Cleanup logging. #Patch Set 5 : Add size_changed field to Picture/IPC #Patch Set 6 : Rebase #Patch Set 7 : Fix lingering Picture instantiations #Patch Set 8 : Fix cros build #
Total comments: 17
Patch Set 9 : Address the easy feedback, awaiting high level design sign off #
Total comments: 18
Patch Set 10 : Address feedback #Patch Set 11 : Rebase #
Total comments: 6
Patch Set 12 : Address final comments #Patch Set 13 : Rebase #Messages
Total messages: 46 (15 generated)
chcunningham@chromium.org changed reviewers: + dalecurtis@chromium.org, liberato@chromium.org
I need to run for tonight - but this seems to be working well. Will add more to desc and do some more manual testing. LMK your thoughts.
almost there. just needs a little more bookkeeping, i think. -fl https://codereview.chromium.org/1750213002/diff/1/content/common/gpu/media/an... File content/common/gpu/media/android_video_decode_accelerator.cc (left): https://codereview.chromium.org/1750213002/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:577: if (!output_picture_buffers_.empty()) { i prefer that you ignore this comment, but i leave it in case the info is useful. for the copying strategy, there is another option. we could be asymmetric about when we dismiss picture buffers between strategies. in particular, one chould continue to dismiss picture buffers when FORMAT_CHANGED shows up for the copying strategy, but not for the deferred strategy. all the frames that have been sent to the pipeline as VideoFrames, but not returned yet, will be kept alive automatically by GVD in the copying case. i'm not sure there's an advantage to doing this over the method described below, but it's good to have options. https://codereview.chromium.org/1750213002/diff/1/content/common/gpu/media/an... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:704: // meta-data and is not associated with OES texture it refers to. whether it's an external texture or not depends on the backing strategy. for the copying strategy, it won't be. PictureBuffer textures are 2D, not external, when we're using the copying strategy. also, for the copying strategy, updating just the metadata won't be enough. we have to bind a big enough 2D texture to hold the copy. however, if we dismiss / re-acquire picture buffers in the copying case (as suggested in another comment as the 'asymmetric' case), then this will happen automatically and no action is needed. if you decide to do it that way, then having a size mismatch here is a DCHECK when using the copying strategy. if, instead, you want to keep deferred / copying strategies symmetric and never dismiss picture buffers on FORMAT_CHANGED, then one can still fix up the textures here for the copying strategy. one could strategy::UpdatePictureBufferSize(picture_buffer, new_size). deferred would do nothing. copying would issue a glTexImage2D, and also update the gpu::gles2::Texture object to reflect the new size. the code @709 would stay here (same for both copying and deferred), as would the additional code to get GVD's PictureBuffer size in sync. this is described @709. https://codereview.chromium.org/1750213002/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:709: i->second.set_size(size_); i don't think that this is enough to get the size metadata updated everywhere. the PictureBuffers are created by the GVD in response to ProvidePictureBuffers call from us. each is assigned an integer handle. after that, we only ever use that integer handle to refer to them; we never send a PictureBuffer back to GVD via IPC. instead, we send back a Picture object (not PictureBuffer) on every frame that references which PictureBuffer contains the image data. this CL will end up changing (a) the size in our local PictureBuffer structure, and (b) the visible size of the Picture in GVD. it doesn't update the PictureBuffer size in GVD, as far as i can tell. see https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_... . it can probably be done via a flag on Picture that tells the GVD to copy the size back into its copy of the PictureBuffer.
Thanks Frank
https://codereview.chromium.org/1750213002/diff/1/content/common/gpu/media/an... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/1/content/common/gpu/media/an... content/common/gpu/media/android_video_decode_accelerator.cc:704: // meta-data and is not associated with OES texture it refers to. On 2016/03/01 17:13:08, liberato wrote: > whether it's an external texture or not depends on the backing strategy. for > the copying strategy, it won't be. PictureBuffer textures are 2D, not external, > when we're using the copying strategy. > > also, for the copying strategy, updating just the metadata won't be enough. we > have to bind a big enough 2D texture to hold the copy. however, if we dismiss / > re-acquire picture buffers in the copying case (as suggested in another comment > as the 'asymmetric' case), then this will happen automatically and no action is > needed. > > if you decide to do it that way, then having a size mismatch here is a DCHECK > when using the copying strategy. > > if, instead, you want to keep deferred / copying strategies symmetric and never > dismiss picture buffers on FORMAT_CHANGED, then one can still fix up the > textures here for the copying strategy. one could > strategy::UpdatePictureBufferSize(picture_buffer, new_size). deferred would do > nothing. copying would issue a glTexImage2D, and also update the > gpu::gles2::Texture object to reflect the new size. > > the code @709 would stay here (same for both copying and deferred), as would the > additional code to get GVD's PictureBuffer size in sync. this is described @709. I like this symmetric proposal better. Patch coming shortly.
Description was changed from ========== Fix Android black frames from MSE config changes. BUG=587994 ========== to ========== Fix Android black frames from MSE config changes. BUG=587994 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Description was changed from ========== Fix Android black frames from MSE config changes. BUG=587994 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Fix Android black frames from MSE config changes. BUG=587994 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Hitting some issues with this approach. Things look good for the zero-copy case, but using the copy strategy is busted. Things look good for the initial config, but when the resolution grows the video appears zoomed in as if the new size isn't fully propogated and the scaling is happening assuming the old size. Screenshot: https://drive.google.com/a/google.com/file/d/0B4uYjpy7GKPMYVhyYnBLU3N6MW8/vie... When the config changes to resolution below the initial size, we see something like a picture-in-picture, where the smaller image is the decoded frame and the larger frame is from the previous config. Screenshot: https://drive.google.com/a/google.com/file/d/0B4uYjpy7GKPMQzFYblVac3U0X00/vie... Hopefully I just have a simple bug. But if it turns out we're overlooking some aspect of updating the Chromium Texture I may start to be convinced that we are better off just requesting new PictureBuffers, at least in the case of the copy strategy. The setup for the Chromium 2d Texture when we initially request PictureBuffers pretty complicated. It would be easy for us to miss a piece of state that needs updating. Even if we do it perfectly, it would be easy for them to change things in a way that breaks us, especially with current test coverage (NONE).
https://codereview.chromium.org/1750213002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/android_copying_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/20001/content/common/gpu/medi... content/common/gpu/media/android_copying_backing_strategy.cc:138: copier_->DoCopyTextureWithTransform( the problem might be here. have you checked that this thing is using the right sizes? in CopyTextureCHROMIUMResourceManager::DoCopyTextureInternal, are you getting the right src and dest sizes / x,y / offsets? (you might want to log only if target==external. that thing is used a lot :) ) the changes you made elsewhere look fine. https://codereview.chromium.org/1750213002/diff/20001/media/filters/gpu_video... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/1750213002/diff/20001/media/filters/gpu_video... media/filters/gpu_video_decoder.cc:477: if (pb.size() != picture.visible_rect().size()) { i don't think that you want to do this unconditionally. the picture should have a flag (like allow_overlay) that permits it. it's not causing the current problem, of course.
Thanks Frank https://codereview.chromium.org/1750213002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/android_copying_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/20001/content/common/gpu/medi... content/common/gpu/media/android_copying_backing_strategy.cc:138: copier_->DoCopyTextureWithTransform( On 2016/03/02 16:50:36, liberato wrote: > the problem might be here. have you checked that this thing is using the right > sizes? in CopyTextureCHROMIUMResourceManager::DoCopyTextureInternal, are you > getting the right src and dest sizes / x,y / offsets? (you might want to log > only if target==external. that thing is used a lot :) ) > > the changes you made elsewhere look fine. Added this log just now. I sadly cant check it until I come back to office, but after a quick code review I'm worried this may not be the source of our trouble. We call this once we've gotten decoded frames back. The decoded frames should have the size described by the most recent media codec format_change (which is when we update AVDA's size_ member). IIUC this size is coming directly from AVDA (as the state_provider_) so I expect to find everything in sync. I also expect to find the x/y and offset values to all be 0, since they're just defaulted to zero here: https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... We'll know for sure when I get a chance to run the test again. https://codereview.chromium.org/1750213002/diff/20001/media/filters/gpu_video... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/1750213002/diff/20001/media/filters/gpu_video... media/filters/gpu_video_decoder.cc:477: if (pb.size() != picture.visible_rect().size()) { On 2016/03/02 16:50:36, liberato wrote: > i don't think that you want to do this unconditionally. the picture should have > a flag (like allow_overlay) that permits it. > > it's not causing the current problem, of course. Ack, happy to add a flag. In our chat earlier I think we reached consensus that pb.size() != picture.visible_rect().size()... as visible rect is a slightly different concept from pb size. In the case of AVDA, this visible rect is coming directly from the size_ member, so perhaps its not a concern for AVDA... but I'm still feeling a little unsure if copying size from visible rect -> pb size is semantically correct for VDA's in general (even if guarded with a flag). WDYT?
On 2016/03/02 21:46:19, chcunningham wrote: > Thanks Frank > > https://codereview.chromium.org/1750213002/diff/20001/content/common/gpu/medi... > File content/common/gpu/media/android_copying_backing_strategy.cc (right): > > https://codereview.chromium.org/1750213002/diff/20001/content/common/gpu/medi... > content/common/gpu/media/android_copying_backing_strategy.cc:138: > copier_->DoCopyTextureWithTransform( > On 2016/03/02 16:50:36, liberato wrote: > > the problem might be here. have you checked that this thing is using the > right > > sizes? in CopyTextureCHROMIUMResourceManager::DoCopyTextureInternal, are you > > getting the right src and dest sizes / x,y / offsets? (you might want to log > > only if target==external. that thing is used a lot :) ) > > > > the changes you made elsewhere look fine. > > Added this log just now. I sadly cant check it until I come back to office, but > after a quick code review I'm worried this may not be the source of our trouble. > We call this once we've gotten decoded frames back. The decoded frames should > have the size described by the most recent media codec format_change (which is > when we update AVDA's size_ member). IIUC this size is coming directly from AVDA > (as the state_provider_) so I expect to find everything in sync. I also expect > to find the x/y and offset values to all be 0, since they're just defaulted to > zero here: > > https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer... > > We'll know for sure when I get a chance to run the test again. > > https://codereview.chromium.org/1750213002/diff/20001/media/filters/gpu_video... > File media/filters/gpu_video_decoder.cc (right): > > https://codereview.chromium.org/1750213002/diff/20001/media/filters/gpu_video... > media/filters/gpu_video_decoder.cc:477: if (pb.size() != > picture.visible_rect().size()) { > On 2016/03/02 16:50:36, liberato wrote: > > i don't think that you want to do this unconditionally. the picture should > have > > a flag (like allow_overlay) that permits it. > > > > it's not causing the current problem, of course. > > Ack, happy to add a flag. > > In our chat earlier I think we reached consensus that pb.size() != > picture.visible_rect().size()... as visible rect is a slightly different concept > from pb size. In the case of AVDA, this visible rect is coming directly from the > size_ member, so perhaps its not a concern for AVDA... but I'm still feeling a > little unsure if copying size from visible rect -> pb size is semantically > We'll know for sure when I get a chance to run the test again. yeah, it's entirely possible that the copier isn't the problem. i suggested it since it'll tell us if the image is leaving AVDA correctly. if it turns out that the copy is not broken, then we can look at the other end of the video pipeline (gl_renderer), and see what texture coordinates it's generating before / after the format changes. if those look wrong, then we go back up the stack to find out why. if they're right, then it's probably something in the command decoder state / Texture that we missed. > correct for VDA's in general (even if guarded with a flag). WDYT? i agree that the flag is very AVDA-specific. doing it in other cases would be very questionable; we definitely want the Picture to tell us that the coded size has been changed. i agree that it's a little weird that the flag means "set coded size from visible rect", rather than "set coded size from this extra gfx::Size() that i put into the Picture". but, i don't think that there's much reason to IPC across a second rectangle for AVDA. so i agree in principle that it's weird, but in practice, it's probably fine. we can always get some more opinions, too. at worst, we just dismiss the buffers. thanks -fl
chcunningham@chromium.org changed reviewers: + bbudge@chromium.org, jbauman@chromium.org
Ready for another round of review. Huge thanks to Frank for patiently walking me through GL. +jbauman for content/common/gpu +bbudge for pepper
Description was changed from ========== Fix Android black frames from MSE config changes. BUG=587994 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Fix Android black frames during MSE config changes. The core of the fix/issue is in AVDA. We were previously dismissing PictureBuffers upon receiving FORMAT_CHANGED from MediaCodec. For the deferred copy strategy, this had the effect of unhooking the Texture from PictureBuffers that may already be on the way to the compositor. With no texture, the compositor would paint black. With this change, FORMAT_CHANGED will no longer dismiss PictureBuffers. Instead we update the size of the existing PictureBuffers as they are used for with the new format. For the copy path we must also update the size of the 2D texture associated with the PictureBuffer. BUG=587994 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
good stuff. gave it just a quick read through. i'll go over it in more detail tomorrow. thanks -fl https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/cli... File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/cli... content/common/gpu/client/gpu_video_decode_accelerator_host.cc:248: bool size_changed) { another option is to start sending 'int32_t flags'. Picture would hold the int32_t, and accessors (e.g., allow_overlay()) would check the right bit instead. it's okay like this too. https://codereview.chromium.org/1750213002/diff/140001/media/video/picture.h File media/video/picture.h (right): https://codereview.chromium.org/1750213002/diff/140001/media/video/picture.h#... media/video/picture.h:67: bool size_changed); since this flag has a very specific, uncommon use, maybe remove it from the constructor and add a setter for it. that'll also prevent most of the files in this CL from changing.
https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... File content/common/gpu/media/android_copying_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... content/common/gpu/media/android_copying_backing_strategy.cc:190: gfx::Rect(new_size.width(), new_size.height())); i think that gfx::Rect(new_size) works. https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:251: // This strategy uses OES textures which do not have a notion of size. We s/OES/external . OES is just a prefix that describes who wrote the extension (khronos group, in this case). maybe something more like this: "This strategy uses EGL images, so those manage the texture size for us. We simply update..." https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:734: strategy_->UpdatePictureBufferSize(&i->second, size_); an alternate idea is to add a "const gfx::Size*" to UseCodecBuffer... which is non-NULL if and only if we'd call UpdatePictureBufferSize. not sure if it's better or worse than what you have. i mention it only because it could be viewed as part of UseCodecBuffer's job. UpdatePictureSize would become a private method on the copying strategy, and could be inlined into UseCodecBuffer in the deferred strategy. then again, you'd have to also send in the Picture. that would be unwieldy, unless you want to move the Picture-setting code here. either way seems fine.
content/renderer/pepper LGTM
So one other option is to do what DXVA does and avoid destroying the old picture buffers from the previous size until they've actually been sent for reuse by the client. Would that work on android? It would at least help keep consistency between the implementations on different platforms.
https://codereview.chromium.org/1750213002/diff/140001/media/filters/gpu_vide... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/1750213002/diff/140001/media/filters/gpu_vide... media/filters/gpu_video_decoder.cc:489: pb.set_size(picture.visible_rect().size()); Hmm, should it be using the coded size instead? I'm not sure if that actually matters, though. https://codereview.chromium.org/1750213002/diff/140001/media/filters/gpu_vide... media/filters/gpu_video_decoder.cc:494: // Some of the VDAs like DXVA, AVDA, and VTVDA don't support and thus don't Is this comment still true with AVDA?
On 2016/03/07 23:46:08, jbauman wrote: > So one other option is to do what DXVA does and avoid destroying the old picture > buffers from the previous size until they've actually been sent for reuse by the > client. Would that work on android? It would at least help keep consistency > between the implementations on different platforms. We considered this option but decided to not do it because it requires additional code complexity with no improvement to user experience. See our discussion in these comments 18 - 22 https://bugs.chromium.org/p/chromium/issues/detail?id=587994#c18
Thanks guys! Addressed the low hanging fruit. John LMK if you're happy with the immediate flushing (vs DXVA approach). If its ok my next CL will address Frank's other comments. https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/cli... File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/cli... content/common/gpu/client/gpu_video_decode_accelerator_host.cc:248: bool size_changed) { On 2016/03/05 08:51:00, liberato wrote: > another option is to start sending 'int32_t flags'. Picture would hold the > int32_t, and accessors (e.g., allow_overlay()) would check the right bit > instead. > > it's okay like this too. I like this idea. This will involve changing a lot of files so let me see if John signs off on the high level approach first.. https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... File content/common/gpu/media/android_copying_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... content/common/gpu/media/android_copying_backing_strategy.cc:190: gfx::Rect(new_size.width(), new_size.height())); On 2016/03/07 16:39:16, liberato wrote: > i think that gfx::Rect(new_size) works. Done. https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:251: // This strategy uses OES textures which do not have a notion of size. We On 2016/03/07 16:39:17, liberato wrote: > s/OES/external . OES is just a prefix that describes who wrote the extension > (khronos group, in this case). > > maybe something more like this: "This strategy uses EGL images, so those manage > the texture size for us. We simply update..." Done. https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:734: strategy_->UpdatePictureBufferSize(&i->second, size_); "then again, you'd have to also send in the Picture. that would be unwieldy," Little confused here. Both UseCodecBufferForPictureBuffer and UpdatePictureBuffer size take in a PictureBuffer. I we make UpdatePBSize private, and call in the top of UseCodecBufferForPictureBuffer, we should be good without ever needing a Picture right? Picture gets constructed below with the new size below. https://codereview.chromium.org/1750213002/diff/140001/media/filters/gpu_vide... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/1750213002/diff/140001/media/filters/gpu_vide... media/filters/gpu_video_decoder.cc:489: pb.set_size(picture.visible_rect().size()); On 2016/03/07 23:53:50, jbauman wrote: > Hmm, should it be using the coded size instead? I'm not sure if that actually > matters, though. I had the same concern, but Frank convinced me this should be ok. Coded size is definitely distinct from visible size, but for AVDA this is always the same. See Frank's reply earlier: "i agree that the flag is very AVDA-specific..." https://codereview.chromium.org/1750213002/diff/140001/media/filters/gpu_vide... media/filters/gpu_video_decoder.cc:494: // Some of the VDAs like DXVA, AVDA, and VTVDA don't support and thus don't On 2016/03/07 23:53:50, jbauman wrote: > Is this comment still true with AVDA? No longer true for AVDA. I've updated this comment. https://codereview.chromium.org/1750213002/diff/140001/media/video/picture.h File media/video/picture.h (right): https://codereview.chromium.org/1750213002/diff/140001/media/video/picture.h#... media/video/picture.h:67: bool size_changed); On 2016/03/05 08:51:00, liberato wrote: > since this flag has a very specific, uncommon use, maybe remove it from the > constructor and add a setter for it. that'll also prevent most of the files in > this CL from changing. I think I prefer your int flags suggestion.
On 2016/03/08 03:19:36, chcunningham wrote: > Thanks guys! Addressed the low hanging fruit. > > John LMK if you're happy with the immediate flushing (vs DXVA approach). If its > ok my next CL will address Frank's other comments. > Ok, I guess this seems reasonable.
https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... File content/common/gpu/media/android_copying_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_copying_backing_strategy.cc:172: // 2) Update the GL texture via glTextImage2D. This step assumes the caller glTexImage2D unless there's an ASCII art GL command I don't know about :) https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_copying_backing_strategy.cc:183: RETURN_IF_NULL(texture_ref); Should this function return an error if this occurs? https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_copying_backing_strategy.cc:186: RETURN_IF_NULL(texture_manager); Ditto? https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:251: // This strategy uses EGL images, so those manage the texture size for us. We s/so those/which/ https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:52: // Override definition from avda_return_on_failure.h to s/state_provider_/this/. Just use RETURN_ON_FAILURE directly instead of redefining the macro, that's just going to cause confusion. https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:609: picturebuffers_requested_ = true; Should this value be double checked to prevent multiple in flight RequestPictureBuffers? https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:723: OutputBufferMap::iterator i = output_picture_buffers_.find(picture_buffer_id); This is a fine place for auto if you like. https://codereview.chromium.org/1750213002/diff/160001/media/filters/gpu_vide... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/1750213002/diff/160001/media/filters/gpu_vide... media/filters/gpu_video_decoder.cc:485: DCHECK(pb.size() != picture.visible_rect().size()); DCHECK_NE ? https://codereview.chromium.org/1750213002/diff/160001/media/video/picture.h File media/video/picture.h (right): https://codereview.chromium.org/1750213002/diff/160001/media/video/picture.h#... media/video/picture.h:38: void set_size(gfx::Size size) { size_ = size; } const& ?
Thanks y'all. All comments should be addressed https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/cli... File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/cli... content/common/gpu/client/gpu_video_decode_accelerator_host.cc:248: bool size_changed) { Decided not to make it int32. I started to feel bad for callers of allow_overlay having to now call flags and bitmask. Then I stumbled upon this in WebRTC call thats not even in this CL yet and decided I'd reached a breaking point https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... Basically, I don't want bring down the visibility of allow_overlay() just because size_changed() is a little awkward. I've done your other proposal: make size_changed be just a setter and leave the constructor as is (with size_changed defaulting to false). https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... File content/common/gpu/media/android_copying_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_copying_backing_strategy.cc:172: // 2) Update the GL texture via glTextImage2D. This step assumes the caller On 2016/03/08 18:29:03, DaleCurtis wrote: > glTexImage2D unless there's an ASCII art GL command I don't know about :) Done. :) https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_copying_backing_strategy.cc:183: RETURN_IF_NULL(texture_ref); On 2016/03/08 18:29:03, DaleCurtis wrote: > Should this function return an error if this occurs? I think its should be ok as-is. This macro This macro will post an error an error to the state_provider_ if null is returned. IIUC this our pattern for reporting the errors from AVDA strategies. https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_copying_backing_strategy.cc:186: RETURN_IF_NULL(texture_manager); On 2016/03/08 18:29:03, DaleCurtis wrote: > Ditto? Ditto https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... File content/common/gpu/media/android_deferred_rendering_backing_strategy.cc (right): https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_deferred_rendering_backing_strategy.cc:251: // This strategy uses EGL images, so those manage the texture size for us. We On 2016/03/08 18:29:03, DaleCurtis wrote: > s/so those/which/ Done. https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:52: // Override definition from avda_return_on_failure.h to s/state_provider_/this/. On 2016/03/08 18:29:03, DaleCurtis wrote: > Just use RETURN_ON_FAILURE directly instead of redefining the macro, that's just > going to cause confusion. Done. https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:609: picturebuffers_requested_ = true; On 2016/03/08 18:29:03, DaleCurtis wrote: > Should this value be double checked to prevent multiple in flight > RequestPictureBuffers? Done. https://codereview.chromium.org/1750213002/diff/160001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:723: OutputBufferMap::iterator i = output_picture_buffers_.find(picture_buffer_id); On 2016/03/08 18:29:03, DaleCurtis wrote: > This is a fine place for auto if you like. Done. https://codereview.chromium.org/1750213002/diff/160001/media/filters/gpu_vide... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/1750213002/diff/160001/media/filters/gpu_vide... media/filters/gpu_video_decoder.cc:485: DCHECK(pb.size() != picture.visible_rect().size()); On 2016/03/08 18:29:03, DaleCurtis wrote: > DCHECK_NE ? Doesn't work for gfx::Size because they haven't defined the << operator :(. https://codereview.chromium.org/1750213002/diff/160001/media/video/picture.h File media/video/picture.h (right): https://codereview.chromium.org/1750213002/diff/160001/media/video/picture.h#... media/video/picture.h:38: void set_size(gfx::Size size) { size_ = size; } On 2016/03/08 18:29:03, DaleCurtis wrote: > const& ? Done.
https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:734: strategy_->UpdatePictureBufferSize(&i->second, size_); On 2016/03/08 03:19:36, chcunningham wrote: > "then again, you'd have to also send in the Picture. that would be unwieldy," > > Little confused here. Both UseCodecBufferForPictureBuffer and > UpdatePictureBuffer size take in a PictureBuffer. I we make UpdatePBSize > private, and call in the top of UseCodecBufferForPictureBuffer, we should be > good without ever needing a Picture right? Picture gets constructed below with > the new size below. We talked over chat - decided not to do this because its not a clear win for readability.
lgtm assuming frank is happy.
lgtm. nice! -fl https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... File content/common/gpu/media/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/140001/content/common/gpu/med... content/common/gpu/media/android_video_decode_accelerator.cc:734: strategy_->UpdatePictureBufferSize(&i->second, size_); On 2016/03/08 03:19:36, chcunningham wrote: > "then again, you'd have to also send in the Picture. that would be unwieldy," > > Little confused here. Both UseCodecBufferForPictureBuffer and > UpdatePictureBuffer size take in a PictureBuffer. I we make UpdatePBSize > private, and call in the top of UseCodecBufferForPictureBuffer, we should be > good without ever needing a Picture right? Picture gets constructed below with > the new size below. discussed offline. you're right, i misread the signature as 'Picture'. https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... File content/common/gpu/media/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... content/common/gpu/media/dxva_video_decode_accelerator_win.cc:1750: media::Picture picture(picture_buffer_id, input_buffer_id, gfx::Rect(0, 0), perhaps remove this file from the CL. https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... File content/common/gpu/media/fake_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... content/common/gpu/media/fake_video_decode_accelerator.cc:181: buffer_id, bitstream_id, gfx::Rect(frame_buffer_size_), false); perhaps remove this file from the CL. https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_video_decode_accelerator.cc:412: client_->PictureReady(media::Picture(output_id, input_id, gfx::Rect(0, 0), perhaps remove this file from the CL.
lgtm
Thanks everyone https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... File content/common/gpu/media/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... content/common/gpu/media/dxva_video_decode_accelerator_win.cc:1750: media::Picture picture(picture_buffer_id, input_buffer_id, gfx::Rect(0, 0), On 2016/03/09 15:32:06, liberato wrote: > perhaps remove this file from the CL. Done. https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... File content/common/gpu/media/fake_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... content/common/gpu/media/fake_video_decode_accelerator.cc:181: buffer_id, bitstream_id, gfx::Rect(frame_buffer_size_), false); On 2016/03/09 15:32:06, liberato wrote: > perhaps remove this file from the CL. Done. https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right): https://codereview.chromium.org/1750213002/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_video_decode_accelerator.cc:412: client_->PictureReady(media::Picture(output_id, input_id, gfx::Rect(0, 0), On 2016/03/09 15:32:06, liberato wrote: > perhaps remove this file from the CL. Done.
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, dalecurtis@chromium.org, jbauman@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1750213002/#ps220001 (title: "Address final comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750213002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750213002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #13 (id:240001) has been deleted
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, dalecurtis@chromium.org, jbauman@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1750213002/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750213002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750213002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750213002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750213002/260001
Message was sent while issue was closed.
Description was changed from ========== Fix Android black frames during MSE config changes. The core of the fix/issue is in AVDA. We were previously dismissing PictureBuffers upon receiving FORMAT_CHANGED from MediaCodec. For the deferred copy strategy, this had the effect of unhooking the Texture from PictureBuffers that may already be on the way to the compositor. With no texture, the compositor would paint black. With this change, FORMAT_CHANGED will no longer dismiss PictureBuffers. Instead we update the size of the existing PictureBuffers as they are used for with the new format. For the copy path we must also update the size of the 2D texture associated with the PictureBuffer. BUG=587994 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Fix Android black frames during MSE config changes. The core of the fix/issue is in AVDA. We were previously dismissing PictureBuffers upon receiving FORMAT_CHANGED from MediaCodec. For the deferred copy strategy, this had the effect of unhooking the Texture from PictureBuffers that may already be on the way to the compositor. With no texture, the compositor would paint black. With this change, FORMAT_CHANGED will no longer dismiss PictureBuffers. Instead we update the size of the existing PictureBuffers as they are used for with the new format. For the copy path we must also update the size of the 2D texture associated with the PictureBuffer. BUG=587994 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Fix Android black frames during MSE config changes. The core of the fix/issue is in AVDA. We were previously dismissing PictureBuffers upon receiving FORMAT_CHANGED from MediaCodec. For the deferred copy strategy, this had the effect of unhooking the Texture from PictureBuffers that may already be on the way to the compositor. With no texture, the compositor would paint black. With this change, FORMAT_CHANGED will no longer dismiss PictureBuffers. Instead we update the size of the existing PictureBuffers as they are used for with the new format. For the copy path we must also update the size of the 2D texture associated with the PictureBuffer. BUG=587994 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Fix Android black frames during MSE config changes. The core of the fix/issue is in AVDA. We were previously dismissing PictureBuffers upon receiving FORMAT_CHANGED from MediaCodec. For the deferred copy strategy, this had the effect of unhooking the Texture from PictureBuffers that may already be on the way to the compositor. With no texture, the compositor would paint black. With this change, FORMAT_CHANGED will no longer dismiss PictureBuffers. Instead we update the size of the existing PictureBuffers as they are used for with the new format. For the copy path we must also update the size of the 2D texture associated with the PictureBuffer. BUG=587994 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/338ac3d5778afc85b741bafc778d2301eb272bfa Cr-Commit-Position: refs/heads/master@{#380305} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/338ac3d5778afc85b741bafc778d2301eb272bfa Cr-Commit-Position: refs/heads/master@{#380305} |
