Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2345123002:
Attach color space information to hardware decoded NV12 video frames. (Closed)
|
Created:
4 years, 3 months ago by hubbe Modified:
4 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAttach color space information to hardware decoded NV12 video frames.
This extracts the color information from the h264 stream and attaches
it to the video frame. Currently this is only done if the result is
NV12 as we don't have the code yet to do it for RGB frames.
BUG=622133, 647725
Committed: https://crrev.com/b213e899246050d799ccb6df0e40837b619da8fb
Committed: https://crrev.com/f33dc1134c8c48a11280e8f62c96d749c4caf4a0
Cr-Original-Commit-Position: refs/heads/master@{#422942}
Cr-Commit-Position: refs/heads/master@{#423776}
Patch Set 1 #Patch Set 2 : forgot to actually set the color space on the video frame #Patch Set 3 : fix owners file #Patch Set 4 : compile fixes for several platforms #
Total comments: 18
Patch Set 5 : comments addressed + compile fixes #
Total comments: 16
Patch Set 6 : comments addressed #Patch Set 7 : compile fix #
Total comments: 1
Patch Set 8 : compile fix #
Total comments: 10
Patch Set 9 : comment fixed + compile fix #Patch Set 10 : revert OWNERS #
Total comments: 2
Patch Set 11 : ipc cleanup #Patch Set 12 : minor bugfix #Messages
Total messages: 93 (59 generated)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hubbe@chromium.org changed reviewers: + sandersd@chromium.org
This code is probably full of Wrong, looking for constructive criticism for how to make it Less Wrong.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Just comments on the parser for tonight. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:819: // Default color space information. I'd prefer that these were in an else block. Note that ParseVUIParameters() is not called if there is no VUI. It may be better to provide |video_signal_type_present_flag| instead. Then you can do what I did for |visible_rect| and provide helper methods that do the interpretation and use Chrome types. Other alternatives exist, you would just be the first to need them. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:820: sps->full_range = true; Should default to false. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... File media/filters/h264_parser.h (right): https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.h:159: bool full_range; Please name these the same as the spec does.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
posciak@chromium.org changed reviewers: + posciak@chromium.org
https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:819: // Default color space information. On 2016/09/16 00:55:20, sandersd wrote: > I'd prefer that these were in an else block. > > Note that ParseVUIParameters() is not called if there is no VUI. It may be > better to provide |video_signal_type_present_flag| instead. Then you can do what > I did for |visible_rect| and provide helper methods that do the interpretation > and use Chrome types. > > Other alternatives exist, you would just be the first to need them. +1. Also, in that case I'd also include video_format for consistency, even if not used right now. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:828: READ_BOOL_OR_RETURN(&sps->full_range); // video_full_range_flag s/full_range/video_full_range_flag/ If possible, please keep names exactly as in the spec, makes it easier to search in the code and look up in the spec. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:832: READ_BITS_OR_RETURN(8, &sps->primary_idc); s/primary_idc/colour_primaries/ https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:833: READ_BITS_OR_RETURN(8, &sps->transfer_idc); transfer_characteristics https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:834: READ_BITS_OR_RETURN(8, &sps->matrix_idc); matrix_coefficients https://codereview.chromium.org/2345123002/diff/60001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2345123002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:904: // TODO(hubbe): Insert the correct color space. Could we perhaps include a crbug number as well please?
Description was changed from ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133 ========== to ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 ==========
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:819: // Default color space information. On 2016/09/16 00:55:20, sandersd wrote: > I'd prefer that these were in an else block. > > Note that ParseVUIParameters() is not called if there is no VUI. It may be > better to provide |video_signal_type_present_flag| instead. Then you can do what > I did for |visible_rect| and provide helper methods that do the interpretation > and use Chrome types. > > Other alternatives exist, you would just be the first to need them. Done. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:819: // Default color space information. On 2016/09/16 04:37:39, Pawel Osciak wrote: > On 2016/09/16 00:55:20, sandersd wrote: > > I'd prefer that these were in an else block. > > > > Note that ParseVUIParameters() is not called if there is no VUI. It may be > > better to provide |video_signal_type_present_flag| instead. Then you can do > what > > I did for |visible_rect| and provide helper methods that do the interpretation > > and use Chrome types. > > > > Other alternatives exist, you would just be the first to need them. > > +1. Also, in that case I'd also include video_format for consistency, even if > not used right now. Done. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:820: sps->full_range = true; On 2016/09/16 00:55:20, sandersd wrote: > Should default to false. Done. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:828: READ_BOOL_OR_RETURN(&sps->full_range); // video_full_range_flag On 2016/09/16 04:37:39, Pawel Osciak wrote: > s/full_range/video_full_range_flag/ > > If possible, please keep names exactly as in the spec, makes it easier to search > in the code and look up in the spec. Done. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:832: READ_BITS_OR_RETURN(8, &sps->primary_idc); On 2016/09/16 04:37:39, Pawel Osciak wrote: > s/primary_idc/colour_primaries/ Done. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:833: READ_BITS_OR_RETURN(8, &sps->transfer_idc); On 2016/09/16 04:37:39, Pawel Osciak wrote: > transfer_characteristics Done. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:834: READ_BITS_OR_RETURN(8, &sps->matrix_idc); On 2016/09/16 04:37:39, Pawel Osciak wrote: > matrix_coefficients Done. https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... File media/filters/h264_parser.h (right): https://codereview.chromium.org/2345123002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.h:159: bool full_range; On 2016/09/16 00:55:20, sandersd wrote: > Please name these the same as the spec does. Done. https://codereview.chromium.org/2345123002/diff/60001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2345123002/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:904: // TODO(hubbe): Insert the correct color space. On 2016/09/16 04:37:39, Pawel Osciak wrote: > Could we perhaps include a crbug number as well please? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
I'll go over the rest of the code after lunch. https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... media/filters/h264_parser.cc:125: gfx::ColorSpace H264SPS::GetColorSpace() const { Nit: Comment where in the spec this comes from (E.2.1, "VUI parameters semantics"). https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... media/filters/h264_parser.cc:128: static_cast<gfx::ColorSpace::PrimaryID>(colour_primaries), This is either unspecified (C++<17) or undefined (C++17) when colour_primaries is outside the range of gfx::ColorSpace::PrimaryID. Since that's easy to arrange, you'll want to check more explicitly.
https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... media/filters/h264_parser.cc:128: static_cast<gfx::ColorSpace::PrimaryID>(colour_primaries), On 2016/09/16 18:50:26, sandersd wrote: > This is either unspecified (C++<17) or undefined (C++17) when colour_primaries > is outside the range of gfx::ColorSpace::PrimaryID. Since that's easy to > arrange, you'll want to check more explicitly. I'll retract that since all of these enums specify their underlying type, and are at least as large as the syntax elements allow. However, I'd still rather the results are guaranteed to be valid enum values.
On 2016/09/16 19:31:47, sandersd wrote: > https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... > File media/filters/h264_parser.cc (right): > > https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... > media/filters/h264_parser.cc:128: > static_cast<gfx::ColorSpace::PrimaryID>(colour_primaries), > On 2016/09/16 18:50:26, sandersd wrote: > > This is either unspecified (C++<17) or undefined (C++17) when colour_primaries > > is outside the range of gfx::ColorSpace::PrimaryID. Since that's easy to > > arrange, you'll want to check more explicitly. > > I'll retract that since all of these enums specify their underlying type, and > are at least as large as the syntax elements allow. However, I'd still rather > the results are guaranteed to be valid enum values. It's a good idea, but probably best to do in a separate CL.
On 2016/09/16 19:39:25, hubbe wrote: > On 2016/09/16 19:31:47, sandersd wrote: > > > https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... > > File media/filters/h264_parser.cc (right): > > > > > https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... > > media/filters/h264_parser.cc:128: > > static_cast<gfx::ColorSpace::PrimaryID>(colour_primaries), > > On 2016/09/16 18:50:26, sandersd wrote: > > > This is either unspecified (C++<17) or undefined (C++17) when > colour_primaries > > > is outside the range of gfx::ColorSpace::PrimaryID. Since that's easy to > > > arrange, you'll want to check more explicitly. > > > > I'll retract that since all of these enums specify their underlying type, and > > are at least as large as the syntax elements allow. However, I'd still rather > > the results are guaranteed to be valid enum values. > > It's a good idea, but probably best to do in a separate CL. I'd like to see the planned design for this, since it could affect your API. (The obvious choice is to also return a base::Optional here.)
On 2016/09/16 19:42:05, sandersd wrote: > On 2016/09/16 19:39:25, hubbe wrote: > > On 2016/09/16 19:31:47, sandersd wrote: > > > > > > https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... > > > File media/filters/h264_parser.cc (right): > > > > > > > > > https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... > > > media/filters/h264_parser.cc:128: > > > static_cast<gfx::ColorSpace::PrimaryID>(colour_primaries), > > > On 2016/09/16 18:50:26, sandersd wrote: > > > > This is either unspecified (C++<17) or undefined (C++17) when > > colour_primaries > > > > is outside the range of gfx::ColorSpace::PrimaryID. Since that's easy to > > > > arrange, you'll want to check more explicitly. > > > > > > I'll retract that since all of these enums specify their underlying type, > and > > > are at least as large as the syntax elements allow. However, I'd still > rather > > > the results are guaranteed to be valid enum values. > > > > It's a good idea, but probably best to do in a separate CL. > > I'd like to see the planned design for this, since it could affect your API. > (The obvious choice is to also return a base::Optional here.) Working on the CL now. Basically I'm including an UNKNOWN value in each of the enums, which will in most circumstances be treated the same as UNSPECIFIED, ie. it defaults to rec709 values.
Looks like there is still some work for DXVA, everything else is good. https://codereview.chromium.org/2345123002/diff/80001/media/filters/gpu_video... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/2345123002/diff/80001/media/filters/gpu_video... media/filters/gpu_video_decoder.cc:610: frame->set_color_space(picture.color_space()); This is pretty clearly wrong given the next line ;-) https://codereview.chromium.org/2345123002/diff/80001/media/gpu/dxva_video_de... File media/gpu/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/2345123002/diff/80001/media/gpu/dxva_video_de... media/gpu/dxva_video_decode_accelerator_win.cc:462: // TODO(hubbe): Is using last_sps_id_ correct here? There could be a gap between the ID changing and the config change occurring, although in practice I've never seen that (which isn't too surprising since it's invalid for the MP4 container). However, there is a more serious possible problem, which I don't know DXVA well enough off-hand to be sure of: a config change could be detected before the last frame is output, and you would tag trailing frames with the wrong colorspace. To do this correctly the tag should be assigned to frames before decode and tracked all the way through. https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/OW... File media/gpu/ipc/common/OWNERS (right): https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/OW... media/gpu/ipc/common/OWNERS:4: file://ipc/SECURITY_OWNERS I don't see any mention of these changes in the CL description, and they seem unrelated? https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/me... File media/gpu/ipc/common/media_messages.h (right): https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/me... media/gpu/ipc/common/media_messages.h:119: BoolPair) /* Buffer is HW overlay capable */ I'm going to call out that I don't like this, even though I won't actually make you fix it. https://codereview.chromium.org/2345123002/diff/80001/media/gpu/vt_video_deco... File media/gpu/vt_video_decode_accelerator_mac.cc (right): https://codereview.chromium.org/2345123002/diff/80001/media/gpu/vt_video_deco... media/gpu/vt_video_decode_accelerator_mac.cc:1075: // TODO(sandersd): Currently, the size got from This mess of a TODO with my name on it is bothering me. If you *happened* to rip it out and replace it with a grammatical request to use the new methods on H264SPS, well, I wouldn't be sad about that.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2345123002/diff/80001/media/filters/gpu_video... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/2345123002/diff/80001/media/filters/gpu_video... media/filters/gpu_video_decoder.cc:610: frame->set_color_space(picture.color_space()); On 2016/09/16 21:54:13, sandersd wrote: > This is pretty clearly wrong given the next line ;-) Done. https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... media/filters/h264_parser.cc:125: gfx::ColorSpace H264SPS::GetColorSpace() const { On 2016/09/16 18:50:26, sandersd wrote: > Nit: Comment where in the spec this comes from (E.2.1, "VUI parameters > semantics"). Done. https://codereview.chromium.org/2345123002/diff/80001/media/filters/h264_pars... media/filters/h264_parser.cc:128: static_cast<gfx::ColorSpace::PrimaryID>(colour_primaries), On 2016/09/16 19:31:47, sandersd wrote: > On 2016/09/16 18:50:26, sandersd wrote: > > This is either unspecified (C++<17) or undefined (C++17) when colour_primaries > > is outside the range of gfx::ColorSpace::PrimaryID. Since that's easy to > > arrange, you'll want to check more explicitly. > > I'll retract that since all of these enums specify their underlying type, and > are at least as large as the syntax elements allow. However, I'd still rather > the results are guaranteed to be valid enum values. Done. https://codereview.chromium.org/2345123002/diff/80001/media/gpu/dxva_video_de... File media/gpu/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/2345123002/diff/80001/media/gpu/dxva_video_de... media/gpu/dxva_video_decode_accelerator_win.cc:462: // TODO(hubbe): Is using last_sps_id_ correct here? On 2016/09/16 21:54:13, sandersd wrote: > There could be a gap between the ID changing and the config change occurring, > although in practice I've never seen that (which isn't too surprising since it's > invalid for the MP4 container). > > However, there is a more serious possible problem, which I don't know DXVA well > enough off-hand to be sure of: a config change could be detected before the last > frame is output, and you would tag trailing frames with the wrong colorspace. > > To do this correctly the tag should be assigned to frames before decode and > tracked all the way through. Updated the code to handle this (more) correctly. https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/OW... File media/gpu/ipc/common/OWNERS (right): https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/OW... media/gpu/ipc/common/OWNERS:4: file://ipc/SECURITY_OWNERS On 2016/09/16 21:54:13, sandersd wrote: > I don't see any mention of these changes in the CL description, and they seem > unrelated? presubmit complained about this, so I fixed it the right way. https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/me... File media/gpu/ipc/common/media_messages.h (right): https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/me... media/gpu/ipc/common/media_messages.h:119: BoolPair) /* Buffer is HW overlay capable */ On 2016/09/16 21:54:13, sandersd wrote: > I'm going to call out that I don't like this, even though I won't actually make > you fix it. Acknowledged. https://codereview.chromium.org/2345123002/diff/80001/media/gpu/vt_video_deco... File media/gpu/vt_video_decode_accelerator_mac.cc (right): https://codereview.chromium.org/2345123002/diff/80001/media/gpu/vt_video_deco... media/gpu/vt_video_decode_accelerator_mac.cc:1075: // TODO(sandersd): Currently, the size got from On 2016/09/16 21:54:13, sandersd wrote: > This mess of a TODO with my name on it is bothering me. If you *happened* to rip > it out and replace it with a grammatical request to use the new methods on > H264SPS, well, I wouldn't be sad about that. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
sandersd@chromium.org changed reviewers: + jbauman@chromium.org
lgtm +jbauman@ in case he wants to review DXVA changes as well. https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/OW... File media/gpu/ipc/common/OWNERS (right): https://codereview.chromium.org/2345123002/diff/80001/media/gpu/ipc/common/OW... media/gpu/ipc/common/OWNERS:4: file://ipc/SECURITY_OWNERS On 2016/09/21 22:04:26, hubbe wrote: > On 2016/09/16 21:54:13, sandersd wrote: > > I don't see any mention of these changes in the CL description, and they seem > > unrelated? > > presubmit complained about this, so I fixed it the right way. Is your version more correct than the current content? dcheng@ suggested the current content, and I don't think there should be any presubmit errors. https://codereview.chromium.org/2345123002/diff/120001/media/filters/h264_par... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2345123002/diff/120001/media/filters/h264_par... media/filters/h264_parser.cc:126: // Comers from VUI section SPS. (E Comment seems to have gotten corrupted?
https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... File media/gpu/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... media/gpu/dxva_video_decode_accelerator_win.cc:1607: void DXVAVideoDecodeAccelerator::DoDecode(const gfx::ColorSpace& color_space) { Another idea would be to get the color space from MF_MT_YUV_MATRIX from GetOutputAvailableType on the IMFTransform. That would also work for VP9, though it doesn't generalize to other platforms as well.
https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... File media/gpu/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... media/gpu/dxva_video_decode_accelerator_win.cc:1607: void DXVAVideoDecodeAccelerator::DoDecode(const gfx::ColorSpace& color_space) { On 2016/09/23 00:55:25, jbauman wrote: > Another idea would be to get the color space from MF_MT_YUV_MATRIX from > GetOutputAvailableType on the IMFTransform. That would also work for VP9, though > it doesn't generalize to other platforms as well. Not a bad idea, but MS enum doesn't seem to contain all the possible values that are in the h264 standard. I see you can also get the transfer function that way. Is it also possible to get the primaries and range? The fact that you can get these from the codec, does that mean that when IMF does the YUV to RGB transform, it actually does the right thing and takes these enums into account? (Because if it does, that's one thing I can check off my list...)
On 2016/09/23 04:43:01, hubbe wrote: > https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... > File media/gpu/dxva_video_decode_accelerator_win.cc (right): > > https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... > media/gpu/dxva_video_decode_accelerator_win.cc:1607: void > DXVAVideoDecodeAccelerator::DoDecode(const gfx::ColorSpace& color_space) { > On 2016/09/23 00:55:25, jbauman wrote: > > Another idea would be to get the color space from MF_MT_YUV_MATRIX from > > GetOutputAvailableType on the IMFTransform. That would also work for VP9, > though > > it doesn't generalize to other platforms as well. > > Not a bad idea, but MS enum doesn't seem to contain all the possible values that > are in the h264 standard. I see you can also get the transfer function that way. > Is it also possible to get the primaries and range? Yeah, there are also MF_MT_VIDEO_PRIMARIES and MF_MT_VIDEO_NOMINAL_RANGE. It looks like the online documentation is out of date - the header in the windows 10 sdk also has BT.2020_10 and BT.2020_12, though it does seem to be missing a few other standards. > > The fact that you can get these from the codec, does that mean that when IMF > does the YUV to RGB transform, it actually does the right thing and takes these > enums into account? (Because if it does, that's one thing I can check off my > list...) No, unfortunately we don't copy them from the output of the decoder IMFTransform to SetInputType on the video processor IMFTransform.
On 2016/09/23 23:17:18, jbauman wrote: > On 2016/09/23 04:43:01, hubbe wrote: > > > https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... > > File media/gpu/dxva_video_decode_accelerator_win.cc (right): > > > > > https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... > > media/gpu/dxva_video_decode_accelerator_win.cc:1607: void > > DXVAVideoDecodeAccelerator::DoDecode(const gfx::ColorSpace& color_space) { > > On 2016/09/23 00:55:25, jbauman wrote: > > > Another idea would be to get the color space from MF_MT_YUV_MATRIX from > > > GetOutputAvailableType on the IMFTransform. That would also work for VP9, > > though > > > it doesn't generalize to other platforms as well. > > > > Not a bad idea, but MS enum doesn't seem to contain all the possible values > that > > are in the h264 standard. I see you can also get the transfer function that > way. > > Is it also possible to get the primaries and range? > > Yeah, there are also MF_MT_VIDEO_PRIMARIES and MF_MT_VIDEO_NOMINAL_RANGE. It > looks like the online documentation is out of date - the header in the windows > 10 sdk also has BT.2020_10 and BT.2020_12, though it does seem to be missing a > few other standards. > > > > The fact that you can get these from the codec, does that mean that when IMF > > does the YUV to RGB transform, it actually does the right thing and takes > these > > enums into account? (Because if it does, that's one thing I can check off my > > list...) > > No, unfortunately we don't copy them from the output of the decoder IMFTransform > to SetInputType on the video processor IMFTransform. Does anybody understand what the win_clang bot is trying to tell me? It doesn't build, but why?
On 2016/09/26 19:30:41, hubbe wrote: > On 2016/09/23 23:17:18, jbauman wrote: > > On 2016/09/23 04:43:01, hubbe wrote: > > > > > > https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... > > > File media/gpu/dxva_video_decode_accelerator_win.cc (right): > > > > > > > > > https://codereview.chromium.org/2345123002/diff/140001/media/gpu/dxva_video_d... > > > media/gpu/dxva_video_decode_accelerator_win.cc:1607: void > > > DXVAVideoDecodeAccelerator::DoDecode(const gfx::ColorSpace& color_space) { > > > On 2016/09/23 00:55:25, jbauman wrote: > > > > Another idea would be to get the color space from MF_MT_YUV_MATRIX from > > > > GetOutputAvailableType on the IMFTransform. That would also work for VP9, > > > though > > > > it doesn't generalize to other platforms as well. > > > > > > Not a bad idea, but MS enum doesn't seem to contain all the possible values > > that > > > are in the h264 standard. I see you can also get the transfer function that > > way. > > > Is it also possible to get the primaries and range? > > > > Yeah, there are also MF_MT_VIDEO_PRIMARIES and MF_MT_VIDEO_NOMINAL_RANGE. It > > looks like the online documentation is out of date - the header in the windows > > 10 sdk also has BT.2020_10 and BT.2020_12, though it does seem to be missing a > > few other standards. > > > > > > The fact that you can get these from the codec, does that mean that when IMF > > > does the YUV to RGB transform, it actually does the right thing and takes > > these > > > enums into account? (Because if it does, that's one thing I can check off my > > > list...) > > > > No, unfortunately we don't copy them from the output of the decoder > IMFTransform > > to SetInputType on the video processor IMFTransform. > > Does anybody understand what the win_clang bot is trying to tell me? > It doesn't build, but why? I don't think media/gpu/ipc/common/media_messages.h is getting the proper #define of IPC_MESSAGE_EXPORT - it's accidentally getting the one from ui/gfx/ipc/color/gfx_param_traits_macros.h
https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... media/filters/h264_parser.cc:126: // Comers from VUI section SPS. (E s/Comers/Comes/ "(E" ? https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... media/filters/h264_parser.cc:844: READ_BITS_OR_RETURN(8, &sps->colour_primaries); IN_RANGE_OR_RETURN(sps->colour_primaries, 0, gfx::ColorSpace::PrimaryID::LAST_STANDARD_VALUE); https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... media/filters/h264_parser.cc:845: READ_BITS_OR_RETURN(8, &sps->transfer_characteristics); IN_RANGE_OR_RETURN(..., 0, gfx::ColorSpace::TransferID::LAST_STANDARD_VALUE) https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... media/filters/h264_parser.cc:846: READ_BITS_OR_RETURN(8, &sps->matrix_coefficients); And similarly here please.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... media/filters/h264_parser.cc:126: // Comers from VUI section SPS. (E On 2016/09/27 07:01:21, Pawel Osciak wrote: > s/Comers/Comes/ > > "(E" ? Done. https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... media/filters/h264_parser.cc:844: READ_BITS_OR_RETURN(8, &sps->colour_primaries); On 2016/09/27 07:01:21, Pawel Osciak wrote: > IN_RANGE_OR_RETURN(sps->colour_primaries, 0, > gfx::ColorSpace::PrimaryID::LAST_STANDARD_VALUE); I don't think that's a great way to do it. They keep adding new values all the time, and when that happens I think we want to keep parsing but map the unknown color component to gfx::ColorSpace::PrimaryID::UNKNOWN, which happens in the gfx::ColorSpace constructor. https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... media/filters/h264_parser.cc:845: READ_BITS_OR_RETURN(8, &sps->transfer_characteristics); On 2016/09/27 07:01:21, Pawel Osciak wrote: > IN_RANGE_OR_RETURN(..., 0, gfx::ColorSpace::TransferID::LAST_STANDARD_VALUE) See previous comment. https://codereview.chromium.org/2345123002/diff/140001/media/filters/h264_par... media/filters/h264_parser.cc:846: READ_BITS_OR_RETURN(8, &sps->matrix_coefficients); On 2016/09/27 07:01:21, Pawel Osciak wrote: > And similarly here please. See previous comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2345123002/#ps180001 (title: "revert OWNERS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hubbe@chromium.org changed reviewers: + bbudge@chromium.org, tsepez@chromium.org
Adding OWNERS and security reviewers. bbudge: content/renderer/pepper/video_decoder_shim.cc tsepez: media/gpu/ipc/common/media_messages.h ui/gfx/ipc/color/gfx_param_traits_macros.h
https://codereview.chromium.org/2345123002/diff/180001/media/gpu/ipc/common/m... File media/gpu/ipc/common/media_messages.h (right): https://codereview.chromium.org/2345123002/diff/180001/media/gpu/ipc/common/m... media/gpu/ipc/common/media_messages.h:123: typedef std::pair<bool, bool> BoolPair; This needs to be in an include-guarded file, rather than a messages file -- generally the fuzzers break when we violate this rule. I'd suggest making an actual struct containing the rect, CS, and the two bools, but it's up to you. Maybe ui/gfx/ipc/color/gfx_param_traits_macros.h is a good place to do this.
content/renderer/pepper lgtm
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2345123002/diff/180001/media/gpu/ipc/common/m... File media/gpu/ipc/common/media_messages.h (right): https://codereview.chromium.org/2345123002/diff/180001/media/gpu/ipc/common/m... media/gpu/ipc/common/media_messages.h:123: typedef std::pair<bool, bool> BoolPair; On 2016/10/03 22:44:09, Tom Sepez wrote: > This needs to be in an include-guarded file, rather than a messages file -- > generally the fuzzers break when we violate this rule. I'd suggest making an > actual struct containing the rect, CS, and the two bools, but it's up to you. > > Maybe ui/gfx/ipc/color/gfx_param_traits_macros.h is a good place to do this. Fixed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
IPC LGTM, thanks!
The CQ bit was unchecked by hubbe@chromium.org
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, jbauman@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2345123002/#ps200001 (title: "ipc cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 ========== to ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 ========== to ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 Committed: https://crrev.com/b213e899246050d799ccb6df0e40837b619da8fb Cr-Commit-Position: refs/heads/master@{#422942} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/b213e899246050d799ccb6df0e40837b619da8fb Cr-Commit-Position: refs/heads/master@{#422942}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2399133002/ by hubbe@chromium.org. The reason for reverting is: https://bugs.chromium.org/p/chromium/issues/detail?id=653388.
Message was sent while issue was closed.
Description was changed from ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 Committed: https://crrev.com/b213e899246050d799ccb6df0e40837b619da8fb Cr-Commit-Position: refs/heads/master@{#422942} ========== to ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 Committed: https://crrev.com/b213e899246050d799ccb6df0e40837b619da8fb Cr-Commit-Position: refs/heads/master@{#422942} ==========
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hubbe@chromium.org
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, bbudge@chromium.org, jbauman@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2345123002/#ps220001 (title: "minor bugfix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 Committed: https://crrev.com/b213e899246050d799ccb6df0e40837b619da8fb Cr-Commit-Position: refs/heads/master@{#422942} ========== to ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 Committed: https://crrev.com/b213e899246050d799ccb6df0e40837b619da8fb Cr-Commit-Position: refs/heads/master@{#422942} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 Committed: https://crrev.com/b213e899246050d799ccb6df0e40837b619da8fb Cr-Commit-Position: refs/heads/master@{#422942} ========== to ========== Attach color space information to hardware decoded NV12 video frames. This extracts the color information from the h264 stream and attaches it to the video frame. Currently this is only done if the result is NV12 as we don't have the code yet to do it for RGB frames. BUG=622133, 647725 Committed: https://crrev.com/b213e899246050d799ccb6df0e40837b619da8fb Committed: https://crrev.com/f33dc1134c8c48a11280e8f62c96d749c4caf4a0 Cr-Original-Commit-Position: refs/heads/master@{#422942} Cr-Commit-Position: refs/heads/master@{#423776} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/f33dc1134c8c48a11280e8f62c96d749c4caf4a0 Cr-Commit-Position: refs/heads/master@{#423776} |
