|
|
Descriptioncolor: Clarify default behaviors
Change YUVDrawFrame to use REC709 if no valid color space is
specified.
Make ColorTransform do nothing if the source color space is invalid.
Add a method for constructing video color spaces, CreateVideo, which
does translation from h264 spec values to gfx::ColorSpace values. For
now, leave all values in existence, though they can likely be removed
in the future.
Change the media/ callers that used to static-cast h264 values to use
this constructor.
Change the ColorSpace PrimaryID/TransferID/MatrixID to not exactly
match the h264 spec (offset by 1000) to discourage doing static
casts.
Add a method to query the h264 values for casting.
Change vp9 to always specify a color space to its frame, even when
one is not known.
BUG=691522
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2697863003
Cr-Commit-Position: refs/heads/master@{#451151}
Committed: https://chromium.googlesource.com/chromium/src/+/172d451b1f1d429ce496c0bb4c57e8e4874cdc43
Patch Set 1 #
Total comments: 11
Patch Set 2 : More INVALI #Patch Set 3 : color: Remove redundant PrimaryID/TransferID/MatrixID values #
Total comments: 4
Patch Set 4 : Do hubbe's suggestion #Patch Set 5 : Do less #
Total comments: 3
Patch Set 6 : Remove the redundant values #Patch Set 7 : Remove more refs from chromecast #
Total comments: 5
Patch Set 8 : Incorporate review feedback #
Dependent Patchsets: Messages
Total messages: 47 (24 generated)
Description was changed from ========== color: Clarify default behaviors Ensure that incompletely-specified video frames be Rec 709. Make incompletely-specified input sources be treated as sRGB. Don't use QCMS for color space transformations unless the gfx::ColorSpace is not a faithful representation of the gfx::ICCProfile. BUG=691522 ========== to ========== color: Clarify default behaviors Ensure that incompletely-specified video frames be Rec 709. Make incompletely-specified input sources be treated as sRGB. Don't use QCMS for color space transformations unless the gfx::ColorSpace is not a faithful representation of the gfx::ICCProfile. BUG=691522 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
ccameron@chromium.org changed reviewers: + hubbe@chromium.org
Rec709 is the right thing to do for not-well-specified video. sRGB is the right thing to do for not-well-specified anything-but-video. Use Rec709 for unspecified video, and sRGB for unspecified everything-else. Also, don't use QCMS unless it's absolutely necessary -- if an ICCProfile is accurately represented by the gfx::ColorSpace it was converted to, just use the gfx::ColorSpace's transfer function and primaries. Hopefully this will fix the "video appears as YUV" bugs.
I feel like this CL is doing a lot of different things, is there a reason they are all together? https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc File ui/gfx/color_space.cc (left): https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#oldco... ui/gfx/color_space.cc:200: if (primaries_ == PrimaryID::UNSPECIFIED || Not sure I understand why this was the way it was before, or why it should be the way it is now... https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newco... ui/gfx/color_space.cc:91: matrix_ != MatrixID::UNSPECIFIED; What if a video specifies the range but nothing else? https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newco... ui/gfx/color_space.cc:459: // Unspecified values are given the sRGB transfer function. The comment above says otherwise. https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newco... ui/gfx/color_space.cc:499: switch (matrix_) { I'm worried that changing all these defaults will affect video rendering. LOTS of videos have unspecified values in them. I'm thinking that we need a method for filling in default values explicitly, like: ColorSpace fully_specified = color_space.Merge(ColorSpace::BT709); then we can add DCHECKS to all the unspecified/unknown values to make sure that we don't forget to call Merge() and what the switch statements defaults to won't matter anymore. https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.h File ui/gfx/color_space.h (right): https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.h#newcod... ui/gfx/color_space.h:193: RangeID range_ = RangeID::FULL; Maybe we should have an unspecified range? I'm not sure if changing this will cause any problems or not...
Urgh, I switched UNSPECIFIED and UNKNOWN in my head, which probably made this patch confusing. This UNSPECIFIED is a valid video value, while UNKNOWN (now INVALID) was a not-valid non-video value. Setting this to INVALID should clear things up. I've renamed all instances of UNKNOWN to INVALID, made INVALID's behavior fail where it can, and changed ColorTransform. This patch also does touch the ICCProfile behavior, but I felt that that also fell under the umbrella of "not quite well defined yet". I can salami-slice more if you'd like. https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc File ui/gfx/color_space.cc (left): https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#oldco... ui/gfx/color_space.cc:200: if (primaries_ == PrimaryID::UNSPECIFIED || On 2017/02/15 08:48:56, hubbe wrote: > Not sure I understand why this was the way it was before, or why it should be > the way it is now... This is the same as before, just expressed in a way consistent with the rest of the file (IsValid also checks MatrixID, but that's done below, so the effect is the same). https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newco... ui/gfx/color_space.cc:91: matrix_ != MatrixID::UNSPECIFIED; On 2017/02/15 08:48:56, hubbe wrote: > What if a video specifies the range but nothing else? Errr... I meant to put UNKNOWN here. So, UNSPECIFIED implies "default to rec709" and UNKNOWN implies "default to sRGB". https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newco... ui/gfx/color_space.cc:459: // Unspecified values are given the sRGB transfer function. On 2017/02/15 08:48:56, hubbe wrote: > The comment above says otherwise. This is the UNSPECIFIED versus UNKNOWN deal ... that said, the two should be flipped (uploaded a patch that flipped them). https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newco... ui/gfx/color_space.cc:499: switch (matrix_) { On 2017/02/15 08:48:56, hubbe wrote: > I'm worried that changing all these defaults will affect video rendering. LOTS > of videos have unspecified values in them. > > I'm thinking that we need a method for filling in default values explicitly, > like: > > ColorSpace fully_specified = color_space.Merge(ColorSpace::BT709); > > then we can add DCHECKS to all the unspecified/unknown values to make sure that > we don't forget to call Merge() and what the switch statements defaults to won't > matter anymore. Should have been UNKNOWN ... does this fix it? https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.h File ui/gfx/color_space.h (right): https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.h#newcod... ui/gfx/color_space.h:193: RangeID range_ = RangeID::FULL; On 2017/02/15 08:48:56, hubbe wrote: > Maybe we should have an unspecified range? > I'm not sure if changing this will cause any problems or not... Not sure what to do here. The default gfx::ColorSpace for everything-but-video should be sRGB (or "don't convert"), so FULL makes more sense here.
https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newco... ui/gfx/color_space.cc:499: switch (matrix_) { On 2017/02/15 09:34:28, ccameron wrote: > On 2017/02/15 08:48:56, hubbe wrote: > > I'm worried that changing all these defaults will affect video rendering. LOTS > > of videos have unspecified values in them. > > > > I'm thinking that we need a method for filling in default values explicitly, > > like: > > > > ColorSpace fully_specified = color_space.Merge(ColorSpace::BT709); > > > > then we can add DCHECKS to all the unspecified/unknown values to make sure > that > > we don't forget to call Merge() and what the switch statements defaults to > won't > > matter anymore. > > Should have been UNKNOWN ... does this fix it? I don't think so. For video you we want unknown values to default to BT709 as well I think.
On 2017/02/15 18:47:45, hubbe wrote: > https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc > File ui/gfx/color_space.cc (right): > > https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newco... > ui/gfx/color_space.cc:499: switch (matrix_) { > On 2017/02/15 09:34:28, ccameron wrote: > > On 2017/02/15 08:48:56, hubbe wrote: > > > I'm worried that changing all these defaults will affect video rendering. > LOTS > > > of videos have unspecified values in them. > > > > > > I'm thinking that we need a method for filling in default values explicitly, > > > like: > > > > > > ColorSpace fully_specified = color_space.Merge(ColorSpace::BT709); > > > > > > then we can add DCHECKS to all the unspecified/unknown values to make sure > > that > > > we don't forget to call Merge() and what the switch statements defaults to > > won't > > > matter anymore. > > > > Should have been UNKNOWN ... does this fix it? > > I don't think so. For video you we want unknown values to default to BT709 as > well I think. Actually, while thinking about this over lunch, I realized that a general Merge() method might not be what we want, as that can't do specialized guessing, such as replacing Range::DERIVED, or guessing transfer functions based on matrices or vice versa. It might be better to create two functions (which would share a fair amount of code): 1) FillDefaultsForVideo() 2) FillDefaultsForSRGB() Each would guarantee that it returns a color space with no unknown/reserved/unspecified components. What do you think?
On 2017/02/15 21:15:18, hubbe wrote: > On 2017/02/15 18:47:45, hubbe wrote: > > https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc > > File ui/gfx/color_space.cc (right): > > > > > https://codereview.chromium.org/2697863003/diff/1/ui/gfx/color_space.cc#newco... > > ui/gfx/color_space.cc:499: switch (matrix_) { > > On 2017/02/15 09:34:28, ccameron wrote: > > > On 2017/02/15 08:48:56, hubbe wrote: > > > > I'm worried that changing all these defaults will affect video rendering. > > LOTS > > > > of videos have unspecified values in them. > > > > > > > > I'm thinking that we need a method for filling in default values > explicitly, > > > > like: > > > > > > > > ColorSpace fully_specified = color_space.Merge(ColorSpace::BT709); > > > > > > > > then we can add DCHECKS to all the unspecified/unknown values to make sure > > > that > > > > we don't forget to call Merge() and what the switch statements defaults to > > > won't > > > > matter anymore. > > > > > > Should have been UNKNOWN ... does this fix it? > > > > I don't think so. For video you we want unknown values to default to BT709 as > > well I think. > > Actually, while thinking about this over lunch, I realized that a general > Merge() method > might not be what we want, as that can't do specialized guessing, such as > replacing Range::DERIVED, > or guessing transfer functions based on matrices or vice versa. > It might be better to create two functions (which would share a fair amount of > code): > > 1) FillDefaultsForVideo() > 2) FillDefaultsForSRGB() > > Each would guarantee that it returns a color space with no > unknown/reserved/unspecified components. > What do you think? After looking at this a bit, we've reached the point where having TransferID, MatrixID, PrimaryID match the h264 spec is causing more problems than it solves. I'm putting together a patch that makes the media code. With that in place, we don't have to think about UNSPECIFIED/RESERVED/etc, mean.
On 2017/02/15 21:24:59, ccameron wrote: > I'm putting together a patch that makes the media code. ... that makes the media code convert these values explicitly.
Description was changed from ========== color: Clarify default behaviors Ensure that incompletely-specified video frames be Rec 709. Make incompletely-specified input sources be treated as sRGB. Don't use QCMS for color space transformations unless the gfx::ColorSpace is not a faithful representation of the gfx::ICCProfile. BUG=691522 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== color: Clarify default behaviors Don't require that ColorSpace PrimaryID/TransferID/MatrixID values match the h264 spec. Modify media/ code to call an explicit conversion function (instead of a static cast). This helps to resolve some of the ambiguity of what things like "UNSPECIFIED" mean (since they mean different things in media and non-media contexts). Change YUVDrawFrame to use REC709 if no valid color space is specified. Change vp9 to always specify a color space to its frame, even when one is not known. BUG=691522 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
ptal -- this gets rid of the static-cast-h264-values-to-ColorSpace-values behavior, and removes the ambiguous RESERVED/UNSPECIFIED/UNKNOWN values. There remains an INVALID for primaries and transfer function, which are used to identify uninitialized gfx::ColorSpace objects. https://codereview.chromium.org/2697863003/diff/40001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/2697863003/diff/40001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:651: This is a behavior change. The decision of how to treat unspecified color should be made here, and not tossed further down the pipeline, where information about where the unspecified values came from has been lost.
The CQ bit was checked by ccameron@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
On 2017/02/15 21:57:24, ccameron wrote: > ptal -- this gets rid of the static-cast-h264-values-to-ColorSpace-values > behavior, and removes the ambiguous RESERVED/UNSPECIFIED/UNKNOWN values. > > There remains an INVALID for primaries and transfer function, which are used to > identify uninitialized gfx::ColorSpace objects. > > https://codereview.chromium.org/2697863003/diff/40001/media/filters/vpx_video... > File media/filters/vpx_video_decoder.cc (right): > > https://codereview.chromium.org/2697863003/diff/40001/media/filters/vpx_video... > media/filters/vpx_video_decoder.cc:651: > This is a behavior change. > > The decision of how to treat unspecified color should be made here, and not > tossed further down the pipeline, where information about where the unspecified > values came from has been lost. Btw, another approach that seems reasonable to me is to leave the media values as they are, but prefix them with "H264_" so that it's clear that they are referring to media formats.
> Btw, another approach that seems reasonable to me is to > leave the media values > as they are, but prefix them with "H264_" so that > it's clear that they are > referring to media formats. Note that H265 and webm uses the same enums. Maybe just prefix them widh VIDEO_*? Anyways, I have no problems with getting rid of the 1:1 between these enums and the standardized values, I just don't see the point of doing so. However, I do have a problem with getting rid of the UNSPECIFIED values, as that limits our ability to make smarter guesses. https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.cc#n... ui/gfx/color_space.cc:51: // All unknown values default to BT709. This limits our ability to make smart guesses based on the values that *are* set. Like, if a video uses the BT601 primaries, it should probably use the BT601 matrix as well. However, with this code we won't be able to tell the difference between an unset, invalid or default value anymore. https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.h File ui/gfx/color_space.h (right): https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.h#ne... ui/gfx/color_space.h:30: enum class PrimaryID : uint16_t { uint8_t (ditto below) https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.h#ne... ui/gfx/color_space.h:162: MatrixID matrix_ = MatrixID::RGB; I think we should default matrix_ & range_ to INVALID
Sounds good -- I'll let you know when I have a new version uploaded. On 2017/02/15 23:30:43, hubbe wrote: > > Btw, another approach that seems reasonable to me is to > > leave the media values > > as they are, but prefix them with "H264_" so that > > it's clear that they are > > referring to media formats. > > Note that H265 and webm uses the same enums. > Maybe just prefix them widh VIDEO_*? Sounds good. > Anyways, I have no problems with getting rid of the 1:1 between these enums and > the standardized values, I just don't see the point of doing so. I found the number of "assert that our enums match gfx::ColorSpace enums and then do a cast" instances throughout the codebase super-scary. I'll change it to stay 1:1, but require that all of the cast instances go through a conversion function. > However, I do have a problem with getting rid of the UNSPECIFIED values, as that > limits our ability to make smarter guesses. Sg -- I'll change it to be VIDEO_UNSPECIFIED just to be more clear. > > https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.cc > File ui/gfx/color_space.cc (right): > > https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.cc#n... > ui/gfx/color_space.cc:51: // All unknown values default to BT709. > This limits our ability to make smart guesses based on the values that *are* > set. > > Like, if a video uses the BT601 primaries, it should probably use the BT601 > matrix as well. However, with this code we won't be able to tell the difference > between an unset, invalid or default value anymore. That makes sense. > https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.h > File ui/gfx/color_space.h (right): > > https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.h#ne... > ui/gfx/color_space.h:30: enum class PrimaryID : uint16_t { > uint8_t (ditto below) > > https://codereview.chromium.org/2697863003/diff/40001/ui/gfx/color_space.h#ne... > ui/gfx/color_space.h:162: MatrixID matrix_ = MatrixID::RGB; > I think we should default matrix_ & range_ to INVALID Sure.
One more possibility: We might not need the UNSPECIFIED part if we make sure that we always construct valid color spaces. Basically, we'd need to put all the smarts at the time of construction. I was thinking that we'd have constructor functions like: ColorSpace::ConstructVideoColorSpace(int, int, int, int) and it would make sure that we construct a valid color space where all UNSPECIFIED things are replaced with best guesses. On Wed, Feb 15, 2017 at 3:39 PM, <ccameron@chromium.org> wrote: > Sounds good -- I'll let you know when I have a new version uploaded. > > On 2017/02/15 23:30:43, hubbe wrote: > > > Btw, another approach that seems reasonable to me is to > > > leave the media values > > > as they are, but prefix them with "H264_" so that > > > it's clear that they are > > > referring to media formats. > > > > Note that H265 and webm uses the same enums. > > Maybe just prefix them widh VIDEO_*? > > Sounds good. > > > Anyways, I have no problems with getting rid of the 1:1 between these > enums > and > > the standardized values, I just don't see the point of doing so. > > I found the number of "assert that our enums match gfx::ColorSpace enums > and > then do a cast" instances throughout the codebase super-scary. > > I'll change it to stay 1:1, but require that all of the cast instances go > through a conversion function. > > > However, I do have a problem with getting rid of the UNSPECIFIED values, > as > that > > limits our ability to make smarter guesses. > > Sg -- I'll change it to be VIDEO_UNSPECIFIED just to be more clear. > > > > > https://codereview.chromium.org/2697863003/diff/40001/ui/ > gfx/color_space.cc > > File ui/gfx/color_space.cc (right): > > > > > https://codereview.chromium.org/2697863003/diff/40001/ui/ > gfx/color_space.cc#newcode51 > > ui/gfx/color_space.cc:51: // All unknown values default to BT709. > > This limits our ability to make smart guesses based on the values that > *are* > > set. > > > > Like, if a video uses the BT601 primaries, it should probably use the > BT601 > > matrix as well. However, with this code we won't be able to tell the > difference > > between an unset, invalid or default value anymore. > > That makes sense. > > > https://codereview.chromium.org/2697863003/diff/40001/ui/ > gfx/color_space.h > > File ui/gfx/color_space.h (right): > > > > > https://codereview.chromium.org/2697863003/diff/40001/ui/ > gfx/color_space.h#newcode30 > > ui/gfx/color_space.h:30: enum class PrimaryID : uint16_t { > > uint8_t (ditto below) > > > > > https://codereview.chromium.org/2697863003/diff/40001/ui/ > gfx/color_space.h#newcode162 > > ui/gfx/color_space.h:162: MatrixID matrix_ = MatrixID::RGB; > > I think we should default matrix_ & range_ to INVALID > > Sure. > > https://codereview.chromium.org/2697863003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== color: Clarify default behaviors Don't require that ColorSpace PrimaryID/TransferID/MatrixID values match the h264 spec. Modify media/ code to call an explicit conversion function (instead of a static cast). This helps to resolve some of the ambiguity of what things like "UNSPECIFIED" mean (since they mean different things in media and non-media contexts). Change YUVDrawFrame to use REC709 if no valid color space is specified. Change vp9 to always specify a color space to its frame, even when one is not known. BUG=691522 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== color: Clarify default behaviors Change YUVDrawFrame to use REC709 if no valid color space is specified. Make ColorTransform do nothing if the source color space is invalid. Add a method for constructing video color spaces, CreateVideo, which does translation from h264 spec values to gfx::ColorSpace values. For now, leave all values in existence, though they can likely be removed in the future. Change the media/ callers that used to static-cast h264 values to use this constructor. Change the ColorSpace PrimaryID/TransferID/MatrixID to not exactly match the h264 spec (offset by 1000) to discourage doing static casts. Add a method to query the h264 values for casting. Change vp9 to always specify a color space to its frame, even when one is not known. BUG=691522 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by ccameron@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...
I took the approach from the last comment -- added a CreateVideo method, which takes H264 spec values. I also got rid of the static casts and checks that our IDs match the h264 values exactly -- I shifted them by 1000 to ensure that anything that passes h264 values to anything but CreateVideo will explode. Also added an accessor to get h264 values back out (which ChromeCast wanted quite some time ago). I left UNSPECIFIED/RESERVED around for now, just to avoid too much churn. I did git rid of UNKNOWN and replaced with INVALID.
The CQ bit was checked by ccameron@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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
> I took the approach from the last comment -- added a > CreateVideo method, which takes H264 spec values. I guess I should have clarified. I meant that we should get rid of all UNSPECIFIED/RESERVED values and have the constructor always construct values that are easy to interpret. > I also got rid of the static casts and checks that our IDs match the h264 values > exactly -- I shifted them by 1000 to ensure that anything that passes h264 > values to anything but CreateVideo will explode. But you don't use a switch() to convert the values, so the checks are still needed..... > Also added an accessor to get h264 values back out > (which ChromeCast wanted quite some time ago). But why? It would be easier and safer to just return our enums... > I left UNSPECIFIED/RESERVED around for now, just to avoid too much churn. I don't think we should. > I did git rid of UNKNOWN and replaced with INVALID. Same difference. However, if it's truly INVALID, we should DCHECK or something when you try to use one of them in a transform instead of defaulting to something. https://codereview.chromium.org/2697863003/diff/80001/media/formats/webm/webm... File media/formats/webm/webm_colour_parser.cc (left): https://codereview.chromium.org/2697863003/diff/80001/media/formats/webm/webm... media/formats/webm/webm_colour_parser.cc:23: enum class MatrixCoefficients : std::uint64_t { The author of this will not want you to remove it. I already commented on it, and they left it in. https://codereview.chromium.org/2697863003/diff/80001/ui/gfx/color_space.h File ui/gfx/color_space.h (right): https://codereview.chromium.org/2697863003/diff/80001/ui/gfx/color_space.h#ne... ui/gfx/color_space.h:55: RESERVED0 = 1000, Actually, this is really weird to me. Either we embrace that the values are EXACTLY THE SAME, or we do not. If we do not, then having numbered values is just silly. https://codereview.chromium.org/2697863003/diff/80001/ui/gfx/color_space.h#ne... ui/gfx/color_space.h:145: void GetVideoParameters(int* h264_primary, I'd rather have four individual accessors.
Updated -- ptal. On 2017/02/16 05:02:39, hubbe wrote: > > I took the approach from the last comment -- added a > > CreateVideo method, which takes H264 spec values. > > I guess I should have clarified. I meant that we should get rid of all > UNSPECIFIED/RESERVED values and have the constructor always construct values > that are easy to interpret. D'oh ... I'd taken that to mean that you wanted the values back, so I'd re-added them. I've re-removed them, but kept them in ColorSpace::CreateVideo. I've also moved to using a switch for the values -- everything undefined goes to BT709 now, but in principle we could do mix'n'match guesses in the future. > > > I also got rid of the static casts and checks that our IDs match the h264 > values > > exactly -- I shifted them by 1000 to ensure that anything that passes h264 > > values to anything but CreateVideo will explode. > > But you don't use a switch() to convert the values, so the checks are still > needed..... Switched to a switch (like it was in PS4). > > Also added an accessor to get h264 values back out > > (which ChromeCast wanted quite some time ago). > > But why? It would be easier and safer to just return our enums... So, to look at it, it's not clear what information they need. I'll re-check-in with them to see if we can now provide them with something more useful. > > I left UNSPECIFIED/RESERVED around for now, just to avoid too much churn. > > I don't think we should. Torched them. > > I did git rid of UNKNOWN and replaced with INVALID. > > Same difference. > However, if it's truly INVALID, we should DCHECK or something when you try to > use one of them in a transform instead of defaulting to something. In due time ... DCHECKing will just blow up all the time, since we're not done with the implementation. > https://codereview.chromium.org/2697863003/diff/80001/media/formats/webm/webm... > File media/formats/webm/webm_colour_parser.cc (left): > > https://codereview.chromium.org/2697863003/diff/80001/media/formats/webm/webm... > media/formats/webm/webm_colour_parser.cc:23: enum class MatrixCoefficients : > std::uint64_t { > The author of this will not want you to remove it. > I already commented on it, and they left it in. Thanks for the heads-up -- left it in. > https://codereview.chromium.org/2697863003/diff/80001/ui/gfx/color_space.h > File ui/gfx/color_space.h (right): > > https://codereview.chromium.org/2697863003/diff/80001/ui/gfx/color_space.h#ne... > ui/gfx/color_space.h:55: RESERVED0 = 1000, > Actually, this is really weird to me. > Either we embrace that the values are EXACTLY THE SAME, or we do not. > If we do not, then having numbered values is just silly. I'm not sure "silly" is productive ... I was trying to follow the suggestions from the comments on the CL, but I find myself going in circles. > https://codereview.chromium.org/2697863003/diff/80001/ui/gfx/color_space.h#ne... > ui/gfx/color_space.h:145: void GetVideoParameters(int* h264_primary, > I'd rather have four individual accessors. I'm going to follow up with Chromecast to see if we can give them something more useful (they asked for the color interface ~4 months ago, and we're in a much better place now). So, for the moment, I've just removed their enums (I'll see if that's reasonable to them).
ccameron@chromium.org changed reviewers: + halliwell@chromium.org
halliwell@, can you take a look at the chromecast/ parts? I deleted some enum checks cause they had started diverging (and broke with this patch). Many months ago we discussed adding some color accessors for cast. We (or, well, I) have a clearer idea of where we're headed now, and maybe we can provide more useful interfaces at this point, so we should follow up on this.
LGTM > On 2017/02/16 05:02:39, hubbe wrote: > > > I took the approach from the last comment -- added a > > > CreateVideo method, which takes H264 spec values. > > > > I guess I should have clarified. I meant that we > should get rid of all > > UNSPECIFIED/RESERVED values and have the constructor > always construct values > > that are easy to interpret. > > D'oh ... I'd taken that to mean that you wanted the > values back, so I'd re-added them. Sorry about the sloppy communication. > I've re-removed them, but kept them in > ColorSpace::CreateVideo. Good. > I've also moved to using a switch for the values -- > everything undefined goes to > BT709 now, but in principle we could do mix'n'match > guesses in the future. Good. > > > Also added an accessor to get h264 values back out > > > (which ChromeCast wanted quite some time ago). > > > > But why? It would be easier and safer to just return our enums... > > So, to look at it, it's not clear what information they need. I'll re-check-in > with them to see if we can now provide them with something more useful. Sounds reasonable. > > > I left UNSPECIFIED/RESERVED around for now, just to avoid too much churn. > > > > I don't think we should. > > Torched them. :) > > > I did git rid of UNKNOWN and replaced with INVALID. > > > > Same difference. > > However, if it's truly INVALID, we should DCHECK or something when you try to > > use one of them in a transform instead of defaulting to something. > > In due time ... DCHECKing will just blow up all the > time, since we're not done with the implementation. I don't think so actually. At this point, we either have fully initialized unambiguous color spaces, or completely invalid color spaces, we never have anything in between. As long as we do: if (!colorspace.IsValid()) colorspace = default; Before we make transforms, we should be good. (And we really need to do that anyways...) > I'm not sure "silly" is productive ... > I was trying to follow the suggestions > from the comments on the CL, but I find > myself going in circles. Sorry, poor choice of words. I appreciate your effort on this, and I'm sorry if my poorly explained ideas sent you in circles. The final result is definitely good though. > > >https://codereview.chromium.org/2697863003/diff/80001/ui/gfx/color_space.h#ne... > > ui/gfx/color_space.h:145: void GetVideoParameters(int* h264_primary, > > I'd rather have four individual accessors. > > I'm going to follow up with Chromecast to see if we can > give them something more > useful (they asked for the color interface ~4 months > ago, and we're in a much better place now). > > So, for the moment, I've just removed their enums (I'll > see if that's reasonable to them). Ok. https://codereview.chromium.org/2697863003/diff/120001/media/filters/h264_par... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2697863003/diff/120001/media/filters/h264_par... media/filters/h264_parser.cc:133: return gfx::ColorSpace( I'm not sure I like that we're hardcoding our fallback here, but I don't have a good suggestions for how to make it better right now either... https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.cc#... ui/gfx/color_space.cc:24: switch (video_primary) { Add TODO to make this smarter so that UNSPECIFIED values can be inferred from values that are specified. Feel free to put my name on it. :)
halliwell@chromium.org changed reviewers: + servolk@chromium.org
On 2017/02/16 08:20:50, hubbe wrote: > LGTM > > > On 2017/02/16 05:02:39, hubbe wrote: > > > > I took the approach from the last comment -- added a > > > > CreateVideo method, which takes H264 spec values. > > > > > > I guess I should have clarified. I meant that we > > should get rid of all > > > UNSPECIFIED/RESERVED values and have the constructor > > always construct values > > > that are easy to interpret. > > > > D'oh ... I'd taken that to mean that you wanted the > > values back, so I'd re-added them. > > Sorry about the sloppy communication. > > > I've re-removed them, but kept them in > > ColorSpace::CreateVideo. > > Good. > > > I've also moved to using a switch for the values -- > > everything undefined goes to > > BT709 now, but in principle we could do mix'n'match > > guesses in the future. > > Good. > > > > > > Also added an accessor to get h264 values back out > > > > (which ChromeCast wanted quite some time ago). > > > > > > But why? It would be easier and safer to just return our enums... > > > > So, to look at it, it's not clear what information they need. I'll re-check-in > > with them to see if we can now provide them with something more useful. > > Sounds reasonable. > > > > > I left UNSPECIFIED/RESERVED around for now, just to avoid too much churn. > > > > > > I don't think we should. > > > > Torched them. > > :) > > > > > I did git rid of UNKNOWN and replaced with INVALID. > > > > > > Same difference. > > > However, if it's truly INVALID, we should DCHECK or something when you try > to > > > use one of them in a transform instead of defaulting to something. > > > > In due time ... DCHECKing will just blow up all the > > time, since we're not done with the implementation. > > I don't think so actually. > At this point, we either have fully initialized unambiguous color spaces, or > completely invalid color spaces, we never have anything in between. As long as > we do: > > if (!colorspace.IsValid()) colorspace = default; > > Before we make transforms, we should be good. > (And we really need to do that anyways...) > > > I'm not sure "silly" is productive ... > > I was trying to follow the suggestions > > from the comments on the CL, but I find > > myself going in circles. > > Sorry, poor choice of words. > I appreciate your effort on this, and I'm sorry if my poorly explained ideas > sent you in circles. The final result is definitely good though. > > > > > >https://codereview.chromium.org/2697863003/diff/80001/ui/gfx/color_space.h#ne... > > > ui/gfx/color_space.h:145: void GetVideoParameters(int* h264_primary, > > > I'd rather have four individual accessors. > > > > I'm going to follow up with Chromecast to see if we can > > give them something more > > useful (they asked for the color interface ~4 months > > ago, and we're in a much better place now). > > > > So, for the moment, I've just removed their enums (I'll > > see if that's reasonable to them). > > Ok. > > https://codereview.chromium.org/2697863003/diff/120001/media/filters/h264_par... > File media/filters/h264_parser.cc (right): > > https://codereview.chromium.org/2697863003/diff/120001/media/filters/h264_par... > media/filters/h264_parser.cc:133: return gfx::ColorSpace( > I'm not sure I like that we're hardcoding our fallback here, but I don't have a > good suggestions for how to make it better right now either... > > https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.cc > File ui/gfx/color_space.cc (right): > > https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.cc#... > ui/gfx/color_space.cc:24: switch (video_primary) { > Add TODO to make this smarter so that UNSPECIFIED values can be inferred from > values that are specified. Feel free to put my name on it. :) +servolk. I think it would be better to keep the old version of chromecast/public for now (fine to delete the static assertions). We have external partners depending on our public headers and have to be careful about how often we update them. So we will probably want to temporarily convert from the newer version of ColorSpace to those enum values until the timing is right to update our headers. (Sergey, would also be great to upstream the conversion code).
servolk@chromium.org changed reviewers: + gfhuang@chromium.org
Please don't remove chromecast enums, this will break our vendor interface. We do need those values. Chromecast is using a custom media pipeline with hardware decoders. Here is how it works: we use Chromium's demuxers and parsers to extract audio and video frames from the input stream and then pass those frames (still compressed) to the vendor media pipeline for decoding and rendering on the screen. In order for them to be able to render video correctly, we need to let them know those 4 color space params that we've read from the input stream. TBH it's not clear to me what you mean when you say 'follow up with Chromecast to see if we can give them something more useful'. These 4 color space components are what we need, I don't think we need anything more useful than that. https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.h File ui/gfx/color_space.h (right): https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.h#n... ui/gfx/color_space.h:116: static ColorSpace CreateVideo(int h264_primary, I can see there was some previous discussion about the fact that these values are not really H.264 specific (HEVC and VP9 use exact same values). So why did we decide to leave h264_ in here? Wouldn't it be better to have generic names here? See for example VP9 spec (e.g. http://www.webmproject.org/docs/container/#TransferCharacteristics) which says those values are from ISO/IEC 23001-8:2013/DCOR1 spec, which is not H.264-specific.
Thanks -- updated the chromecast parts to just remove the checks https://codereview.chromium.org/2697863003/diff/120001/media/filters/h264_par... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/2697863003/diff/120001/media/filters/h264_par... media/filters/h264_parser.cc:133: return gfx::ColorSpace( On 2017/02/16 08:20:50, hubbe wrote: > I'm not sure I like that we're hardcoding our fallback here, but I don't have a > good suggestions for how to make it better right now either... I've put a "TODO(ccameron/hubbe): determine how to set default values" here. Another idea would be to have a gfx::ColorSpace::CreateVideoDefault(bool full_fange);" and use that. But I'll leave that in the TODO. https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.cc File ui/gfx/color_space.cc (right): https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.cc#... ui/gfx/color_space.cc:24: switch (video_primary) { On 2017/02/16 08:20:50, hubbe wrote: > Add TODO to make this smarter so that UNSPECIFIED values can be inferred from > values that are specified. Feel free to put my name on it. :) Done.
On 2017/02/16 20:45:27, ccameron wrote: > Thanks -- updated the chromecast parts to just remove the checks > > https://codereview.chromium.org/2697863003/diff/120001/media/filters/h264_par... > File media/filters/h264_parser.cc (right): > > https://codereview.chromium.org/2697863003/diff/120001/media/filters/h264_par... > media/filters/h264_parser.cc:133: return gfx::ColorSpace( > On 2017/02/16 08:20:50, hubbe wrote: > > I'm not sure I like that we're hardcoding our fallback here, but I don't have > a > > good suggestions for how to make it better right now either... > > I've put a "TODO(ccameron/hubbe): determine how to set default values" here. > > Another idea would be to have a gfx::ColorSpace::CreateVideoDefault(bool > full_fange);" and use that. But I'll leave that in the TODO. > > https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.cc > File ui/gfx/color_space.cc (right): > > https://codereview.chromium.org/2697863003/diff/120001/ui/gfx/color_space.cc#... > ui/gfx/color_space.cc:24: switch (video_primary) { > On 2017/02/16 08:20:50, hubbe wrote: > > Add TODO to make this smarter so that UNSPECIFIED values can be inferred from > > values that are specified. Feel free to put my name on it. :) > > Done. chromecast/ lgtm
The CQ bit was checked by ccameron@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 ccameron@chromium.org
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org Link to the patchset: https://codereview.chromium.org/2697863003/#ps140001 (title: "Incorporate review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487283859298980, "parent_rev": "10ea5654857dc5463ddec0af8e5fca34f4dc1a94", "commit_rev": "172d451b1f1d429ce496c0bb4c57e8e4874cdc43"}
Message was sent while issue was closed.
Description was changed from ========== color: Clarify default behaviors Change YUVDrawFrame to use REC709 if no valid color space is specified. Make ColorTransform do nothing if the source color space is invalid. Add a method for constructing video color spaces, CreateVideo, which does translation from h264 spec values to gfx::ColorSpace values. For now, leave all values in existence, though they can likely be removed in the future. Change the media/ callers that used to static-cast h264 values to use this constructor. Change the ColorSpace PrimaryID/TransferID/MatrixID to not exactly match the h264 spec (offset by 1000) to discourage doing static casts. Add a method to query the h264 values for casting. Change vp9 to always specify a color space to its frame, even when one is not known. BUG=691522 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== color: Clarify default behaviors Change YUVDrawFrame to use REC709 if no valid color space is specified. Make ColorTransform do nothing if the source color space is invalid. Add a method for constructing video color spaces, CreateVideo, which does translation from h264 spec values to gfx::ColorSpace values. For now, leave all values in existence, though they can likely be removed in the future. Change the media/ callers that used to static-cast h264 values to use this constructor. Change the ColorSpace PrimaryID/TransferID/MatrixID to not exactly match the h264 spec (offset by 1000) to discourage doing static casts. Add a method to query the h264 values for casting. Change vp9 to always specify a color space to its frame, even when one is not known. BUG=691522 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2697863003 Cr-Commit-Position: refs/heads/master@{#451151} Committed: https://chromium.googlesource.com/chromium/src/+/172d451b1f1d429ce496c0bb4c57... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/172d451b1f1d429ce496c0bb4c57... |