Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
653663006:
Support configuration changes in VTVideoDecodeAccelerator. (Closed)
|
Created:
6 years, 2 months ago by sandersd (OOO until July 31) Modified:
6 years, 1 month ago Reviewers:
Pawel Osciak CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@vt_make_context_current Project:
chromium Visibility:
Public. |
DescriptionSupport configuration changes in VTVideoDecodeAccelerator.
This support is minimalist, and does not carefully verify SPS IDs, so its behavior is not guaranteed for streams with strange SPS/PPS ordering (especially after seeking).
Resolution change support is included and fully functional.
BUG=642453003
Committed: https://crrev.com/a896e504cd759295ec186d35f358f68de9d3e02c
Cr-Commit-Position: refs/heads/master@{#303278}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 16
Patch Set 3 : Address comments. #
Total comments: 16
Patch Set 4 : Improve comments. #
Total comments: 3
Patch Set 5 : Rename ProcessSizeChange. #
Messages
Total messages: 13 (2 generated)
sandersd@chromium.org changed reviewers: + posciak@chromium.org
https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:279: case media::H264NALU::kPPS: Does PPS actually change config? PPSes don't have to be accompanied by SPSes... https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:293: if (config_changed && last_sps_.size() != 0 && last_pps_.size() != 0) { Shouldn't we return an error if config_changed, but no sps or no pps? https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:325: // TODO(sandersd): Check that the slice's PPS matches the current PPS. That doesn't have to be the case... You could refer to another PPS from before. Is this a requirement of VT? https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:554: while (!decoded_frames_.empty() && Please document, this is getting pretty convoluted. https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:567: // TODO(sandersd): If GpuVideoDecoder didn't specifically check the size, this Could you explain this comment please? https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:104: int32_t ProcessDroppedFrames( Please document. https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:138: std::queue<int32_t> pending_bitstream_ids_; Could we please add documentation for members?
https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:279: case media::H264NALU::kPPS: On 2014/10/30 11:02:52, Pawel Osciak wrote: > Does PPS actually change config? PPSes don't have to be accompanied by SPSes... If the PPS changes, then VT needs to be reconfigured. I don't reset the SPS, under the assumption that the previous SPS is still correct. https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:293: if (config_changed && last_sps_.size() != 0 && last_pps_.size() != 0) { On 2014/10/30 11:02:52, Pawel Osciak wrote: > Shouldn't we return an error if config_changed, but no sps or no pps? Seems reasonable. Done. https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:325: // TODO(sandersd): Check that the slice's PPS matches the current PPS. On 2014/10/30 11:02:52, Pawel Osciak wrote: > That doesn't have to be the case... You could refer to another PPS from before. > Is this a requirement of VT? VT requires that we pack both the SPS and PPS into the configuration data (that is, we recreate an avcC record). This code just assumes that the most recent SPS and PPS are paired, and I'll change that when/if it is necessary. https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:554: while (!decoded_frames_.empty() && On 2014/10/30 11:02:52, Pawel Osciak wrote: > Please document, this is getting pretty convoluted. Done. https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:567: // TODO(sandersd): If GpuVideoDecoder didn't specifically check the size, this On 2014/10/30 11:02:52, Pawel Osciak wrote: > Could you explain this comment please? Done. https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:104: int32_t ProcessDroppedFrames( On 2014/10/30 11:02:52, Pawel Osciak wrote: > Please document. Done. https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:138: std::queue<int32_t> pending_bitstream_ids_; On 2014/10/30 11:02:52, Pawel Osciak wrote: > Could we please add documentation for members? Done.
https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/653663006/diff/70001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:142: // TODO(sandersd): A single map of structs holding picture data. This would still be great to have... https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:270: last_sps_.assign(nalu.data, nalu.data + nalu.size); You actually shouldn't have to store SPS/PPS. H264Parser keeps all current SPSes and PPSes cached and you can get them via GetSPS(id) GetPPS(id). The parser will also already verify for you that i.e. PPS refers to a valid SPS, so you don't have to check ordering or worry that the last PPS and SPS is the one you want. https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:330: // TODO(sandersd): Check that the slice's PPS matches the current PPS. A slice doesn't have to refer to the latest PPS afair. Otherwise it wouldn't make sense to allow more than one pps id in the stream. https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:563: decoded_frames_.front().image_buffer.get() == NULL) { Couldn't we just do this in SendPictures? You could check for decoded_frames_.front().image_buffer.get() == NULL and not get the surface then, just return everything like here? https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:615: ProcessSizeChange(); This also wouldn't have to be duplicated if we did everything in the while loop? https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:616: if (available_picture_ids_.empty()) Do you have this here to save on making context current if this is empty? Otherwise the while loop takes care of this. https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:106: // context. Parameters and return value are unclear. https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.h:153: // Queue of bitstreams currently being decoded. This is mostly needed to be Or waiting to be decoded...
https://codereview.chromium.org/653663006/diff/70001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/653663006/diff/70001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.h:142: // TODO(sandersd): A single map of structs holding picture data. On 2014/11/02 23:09:52, Pawel Osciak wrote: > This would still be great to have... Acknowledged. https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:270: last_sps_.assign(nalu.data, nalu.data + nalu.size); On 2014/11/02 23:09:52, Pawel Osciak wrote: > You actually shouldn't have to store SPS/PPS. H264Parser keeps all current SPSes > and PPSes cached and you can get them via GetSPS(id) GetPPS(id). The parser will > also already verify for you that i.e. PPS refers to a valid SPS, so you don't > have to check ordering or worry that the last PPS and SPS is the one you want. But GetSPS()/GetPPS() return the parsed structure, not the original bytes, which are what need to go in the avcC record. https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:330: // TODO(sandersd): Check that the slice's PPS matches the current PPS. On 2014/11/02 23:09:52, Pawel Osciak wrote: > A slice doesn't have to refer to the latest PPS afair. Otherwise it wouldn't > make sense to allow more than one pps id in the stream. Acknowledged. https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:563: decoded_frames_.front().image_buffer.get() == NULL) { On 2014/11/02 23:09:52, Pawel Osciak wrote: > Couldn't we just do this in SendPictures? You could check for > decoded_frames_.front().image_buffer.get() == NULL > and not get the surface then, just return everything like here? That's true. The purpose of separating them is that it makes the size change logic much simpler, without waiting for picture buffers that we don't actually need. (It also avoids unnecessary size changes and GL context switches, but I don't consider that important.) https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:615: ProcessSizeChange(); On 2014/11/02 23:09:52, Pawel Osciak wrote: > This also wouldn't have to be duplicated if we did everything in the while loop? It would also incorrectly wait for a picture buffer before starting the size change. I admit that this may not matter in practice. https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:616: if (available_picture_ids_.empty()) On 2014/11/02 23:09:52, Pawel Osciak wrote: > Do you have this here to save on making context current if this is empty? > Otherwise the while loop takes care of this. That's correct. This function may get called many times before successfully sending a frame, but it may not be a necessary optimization. https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.h:106: // context. On 2014/11/02 23:09:52, Pawel Osciak wrote: > Parameters and return value are unclear. Done. https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.h:153: // Queue of bitstreams currently being decoded. This is mostly needed to be On 2014/11/02 23:09:52, Pawel Osciak wrote: > Or waiting to be decoded... Done.
On 2014/11/03 21:14:25, sandersd wrote: > https://codereview.chromium.org/653663006/diff/70001/content/common/gpu/media... > File content/common/gpu/media/vt_video_decode_accelerator.h (right): > > https://codereview.chromium.org/653663006/diff/70001/content/common/gpu/media... > content/common/gpu/media/vt_video_decode_accelerator.h:142: // TODO(sandersd): A > single map of structs holding picture data. > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > This would still be great to have... > > Acknowledged. > > https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... > File content/common/gpu/media/vt_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... > content/common/gpu/media/vt_video_decode_accelerator.cc:270: > last_sps_.assign(nalu.data, nalu.data + nalu.size); > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > You actually shouldn't have to store SPS/PPS. H264Parser keeps all current > SPSes > > and PPSes cached and you can get them via GetSPS(id) GetPPS(id). The parser > will > > also already verify for you that i.e. PPS refers to a valid SPS, so you don't > > have to check ordering or worry that the last PPS and SPS is the one you want. > > But GetSPS()/GetPPS() return the parsed structure, not the original bytes, which > are what need to go in the avcC record. > > https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... > content/common/gpu/media/vt_video_decode_accelerator.cc:330: // TODO(sandersd): > Check that the slice's PPS matches the current PPS. > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > A slice doesn't have to refer to the latest PPS afair. Otherwise it wouldn't > > make sense to allow more than one pps id in the stream. > > Acknowledged. > > https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... > content/common/gpu/media/vt_video_decode_accelerator.cc:563: > decoded_frames_.front().image_buffer.get() == NULL) { > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > Couldn't we just do this in SendPictures? You could check for > > decoded_frames_.front().image_buffer.get() == NULL > > and not get the surface then, just return everything like here? > > That's true. The purpose of separating them is that it makes the size change > logic much simpler, without waiting for picture buffers that we don't actually > need. (It also avoids unnecessary size changes and GL context switches, but I > don't consider that important.) > > https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... > content/common/gpu/media/vt_video_decode_accelerator.cc:615: > ProcessSizeChange(); > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > This also wouldn't have to be duplicated if we did everything in the while > loop? > > It would also incorrectly wait for a picture buffer before starting the size > change. I admit that this may not matter in practice. > > https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... > content/common/gpu/media/vt_video_decode_accelerator.cc:616: if > (available_picture_ids_.empty()) > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > Do you have this here to save on making context current if this is empty? > > Otherwise the while loop takes care of this. > > That's correct. This function may get called many times before successfully > sending a frame, but it may not be a necessary optimization. > > https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... > File content/common/gpu/media/vt_video_decode_accelerator.h (right): > > https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... > content/common/gpu/media/vt_video_decode_accelerator.h:106: // context. > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > Parameters and return value are unclear. > > Done. > > https://codereview.chromium.org/653663006/diff/90001/content/common/gpu/media... > content/common/gpu/media/vt_video_decode_accelerator.h:153: // Queue of > bitstreams currently being decoded. This is mostly needed to be > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > Or waiting to be decoded... > > Done. Ping?
lgtm % comments https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:616: if (available_picture_ids_.empty()) On 2014/11/03 21:14:25, sandersd wrote: > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > Do you have this here to save on making context current if this is empty? > > Otherwise the while loop takes care of this. > > That's correct. This function may get called many times before successfully > sending a frame, but it may not be a necessary optimization. I'm not a big fan of this kind of optimizations, but I won't press too much. I know MCC might be long. But ideally we shouldn't be coming here unless we knew we could output a picture... https://chromiumcodereview.appspot.com/653663006/diff/110001/content/common/g... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/653663006/diff/110001/content/common/g... content/common/gpu/media/vt_video_decode_accelerator.cc:628: DCHECK(!decoded_frames_.empty()); Can this ever fail given l612 and l668? https://chromiumcodereview.appspot.com/653663006/diff/110001/content/common/g... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/653663006/diff/110001/content/common/g... content/common/gpu/media/vt_video_decode_accelerator.h:114: void ProcessSizeChange(); ProcessSizeChangeIfNeeded() ?
https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/653663006/diff/90001/content/common/gp... content/common/gpu/media/vt_video_decode_accelerator.cc:616: if (available_picture_ids_.empty()) On 2014/11/07 10:09:38, Pawel Osciak wrote: > On 2014/11/03 21:14:25, sandersd wrote: > > On 2014/11/02 23:09:52, Pawel Osciak wrote: > > > Do you have this here to save on making context current if this is empty? > > > Otherwise the while loop takes care of this. > > > > That's correct. This function may get called many times before successfully > > sending a frame, but it may not be a necessary optimization. > > I'm not a big fan of this kind of optimizations, but I won't press too much. I > know MCC might be long. But ideally we shouldn't be coming here unless we knew > we could output a picture... The next CL splits this method in two; I'll leave it how it is here for now. https://chromiumcodereview.appspot.com/653663006/diff/110001/content/common/g... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/653663006/diff/110001/content/common/g... content/common/gpu/media/vt_video_decode_accelerator.cc:628: DCHECK(!decoded_frames_.empty()); On 2014/11/07 10:09:38, Pawel Osciak wrote: > Can this ever fail given l612 and l668? No, I just make it explicit because the next line assumes it.
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/653663006/130001
Message was sent while issue was closed.
Committed patchset #5 (id:130001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a896e504cd759295ec186d35f358f68de9d3e02c Cr-Commit-Position: refs/heads/master@{#303278} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
