Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2940593002:
Optimize yuv decoding fail check and remove the usage of unwanted variable.
|
Created:
3 years, 6 months ago by naga Modified:
3 years, 5 months ago Reviewers:
chrishtr, Shanmuga Pandi, bsalomon, scroggo_chromium, f(malita), Srirama, Noel Gordon, reed1 CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionOptimize yuv decoding fail check and remove the usage of unwanted variable.
This patch optimizes the yuv decoder failure check and removes the unwanted variable
usage. yuv_decoding_failed_ condition check present in the GetYUVComponentSizes()
but this variable is updated after calling this function so we can remove this.
BUG=732769
Patch Set 1 : Optimize YUV decode fail check #Patch Set 2 : Modified the function return type #
Total comments: 6
Patch Set 3 : Remove image plane creation check #
Messages
Total messages: 19 (9 generated)
Description was changed from ========== Optimize yuv decoding fail chak and remove the usage of unwanted variable. This patch optimizes the yuv decoder failure check and removes the unwated vriable usage. yuv_decoding_failed_ condition check present in the GetYUVComponentSizes() but this variable is updated after calling this function so we can remove this. BUG=NONE ========== to ========== Optimize yuv decoding fail chak and remove the usage of unwanted variable. This patch optimizes the yuv decoder failure check and removes the unwated vriable usage. yuv_decoding_failed_ condition check present in the GetYUVComponentSizes() but this variable is updated after calling this function so we can remove this. BUG=NONE R=SamsungPeerReview ==========
nagarajan.n@samsung.com changed reviewers: + shanmuga.m@samsung.com, srirama.m@samsung.com
Description was changed from ========== Optimize yuv decoding fail chak and remove the usage of unwanted variable. This patch optimizes the yuv decoder failure check and removes the unwated vriable usage. yuv_decoding_failed_ condition check present in the GetYUVComponentSizes() but this variable is updated after calling this function so we can remove this. BUG=NONE R=SamsungPeerReview ========== to ========== Optimize yuv decoding fail check and remove the usage of unwanted variable. This patch optimizes the yuv decoder failure check and removes the unwanted variable usage. yuv_decoding_failed_ condition check present in the GetYUVComponentSizes() but this variable is updated after calling this function so we can remove this. BUG=NONE R=SamsungPeerReview ==========
Description was changed from ========== Optimize yuv decoding fail check and remove the usage of unwanted variable. This patch optimizes the yuv decoder failure check and removes the unwanted variable usage. yuv_decoding_failed_ condition check present in the GetYUVComponentSizes() but this variable is updated after calling this function so we can remove this. BUG=NONE R=SamsungPeerReview ========== to ========== Optimize yuv decoding fail check and remove the usage of unwanted variable. This patch optimizes the yuv decoder failure check and removes the unwanted variable usage. yuv_decoding_failed_ condition check present in the GetYUVComponentSizes() but this variable is updated after calling this function so we can remove this. BUG=732769 R=SamsungPeerReview ==========
Description was changed from ========== Optimize yuv decoding fail check and remove the usage of unwanted variable. This patch optimizes the yuv decoder failure check and removes the unwanted variable usage. yuv_decoding_failed_ condition check present in the GetYUVComponentSizes() but this variable is updated after calling this function so we can remove this. BUG=732769 R=SamsungPeerReview ========== to ========== Optimize yuv decoding fail check and remove the usage of unwanted variable. This patch optimizes the yuv decoder failure check and removes the unwanted variable usage. yuv_decoding_failed_ condition check present in the GetYUVComponentSizes() but this variable is updated after calling this function so we can remove this. BUG=732769 ==========
nagarajan.n@samsung.com changed reviewers: + noel@chromium.org, scroggo@chromium.org
nagarajan.n@samsung.com changed reviewers: + chrishtr@chromium.org
nagarajan.n@samsung.com changed reviewers: + fmalita@chromium.org
Sorry if I'm reviewing this before you were ready for review. I just noticed that I have a few open reviews. You've been changing your reviewers, so I'm assuming you want one? In case you didn't know (this is a common mistake with this old code review tool), you have to click "Publish+Mail Comments" for us to get an email. Simply adding reviewers does not notify us. https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:380: if (!decoder || !decoder->CanDecodeToYUV()) This change (moving caller of CanDecodeToYUV from UpdateYUVComponentSizes to here) seems fine to me. This saves us the creation of dummy image planes if CanDecodeToYUV returns false, and CanDecodeToYUV does not take into consideration whether image planes are set. It seems unrelated to the other change though, and maybe belongs in its own CL. https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (left): https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:148: bool yuv_decoding_failed_; I don't understand the removal of this variable. The purpose is so that we do not try to YUV decode a second time if we failed previously. Does something else prevent us from retrying? Or is it okay to retry for some reason?
On 2017/06/15 14:45:23, scroggo_chromium wrote: > Sorry if I'm reviewing this before you were ready for review. I just noticed > that I have a few open reviews. You've been changing your reviewers, so I'm > assuming you want one? > > In case you didn't know (this is a common mistake with this old code review > tool), you have to click "Publish+Mail Comments" for us to get an email. Simply > adding reviewers does not notify us. Ok I will take care of this next time. > > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp > (right): > > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:380: if > (!decoder || !decoder->CanDecodeToYUV()) > This change (moving caller of CanDecodeToYUV from UpdateYUVComponentSizes to > here) seems fine to me. This saves us the creation of dummy image planes if > CanDecodeToYUV returns false, and CanDecodeToYUV does not take into > consideration whether image planes are set. > Yes > It seems unrelated to the other change though, and maybe belongs in its own CL. > > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (left): > > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:148: bool > yuv_decoding_failed_; > I don't understand the removal of this variable. The purpose is so that we do > not try to YUV decode a second time if we failed previously. > > Does something else prevent us from retrying? Or is it okay to retry for some > reason? In the current code this condition will never fail - if (yuv_decoding_failed_) - return false; So I removed in this patch.
https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:204: yuv_decoding_failed_ = true; > In the current code this condition will never fail > - if (yuv_decoding_failed_) > - return false; > So I removed in this patch. It sounds like you're saying that yuv_decoding_failed_ is never set to true, so we don't need to check its value? We set it to true right here, if there was a failure.
https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:204: yuv_decoding_failed_ = true; On 2017/06/15 16:50:38, scroggo_chromium wrote: > > In the current code this condition will never fail > > - if (yuv_decoding_failed_) > > - return false; > > So I removed in this patch. > > It sounds like you're saying that yuv_decoding_failed_ is never set to true, so > we don't need to check its value? We set it to true right here, if there was a > failure. yuv_decoding_failed _ is set to true once DecodeToYUV() is failed. https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrYUVProvider.c... In our current source code, yuv_decoding_failed_ check present before calling the decoder. This function is used to fetch yuv plane sizes for memory allocation https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrYUVProvider.c...
https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:204: yuv_decoding_failed_ = true; On 2017/06/15 17:49:02, naga wrote: > On 2017/06/15 16:50:38, scroggo_chromium wrote: > > > In the current code this condition will never fail > > > - if (yuv_decoding_failed_) > > > - return false; > > > So I removed in this patch. > > > > It sounds like you're saying that yuv_decoding_failed_ is never set to true, > so > > we don't need to check its value? We set it to true right here, if there was a > > failure. > yuv_decoding_failed _ is set to true once DecodeToYUV() is failed. Isn't that what I just said? This method is DecodeToYUV(). > https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrYUVProvider.c... > > In our current source code, yuv_decoding_failed_ check present before calling > the decoder. This function is used to fetch yuv plane sizes for memory > allocation > https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrYUVProvider.c... > I'm not sure what you're pointing out to me with the links. I agree that the first time this method is called, yuv_decoding_failed_ is guaranteed to be false. Are you telling me that we will never call it a second time?
On 2017/06/15 20:40:01, scroggo_chromium wrote: > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): > > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:204: > yuv_decoding_failed_ = true; > On 2017/06/15 17:49:02, naga wrote: > > On 2017/06/15 16:50:38, scroggo_chromium wrote: > > > > In the current code this condition will never fail > > > > - if (yuv_decoding_failed_) > > > > - return false; > > > > So I removed in this patch. > > > > > > It sounds like you're saying that yuv_decoding_failed_ is never set to true, > > so > > > we don't need to check its value? We set it to true right here, if there was > a > > > failure. > > yuv_decoding_failed _ is set to true once DecodeToYUV() is failed. > > Isn't that what I just said? This method is DecodeToYUV(). > > > > https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrYUVProvider.c... > > > > > In our current source code, yuv_decoding_failed_ check present before calling > > the decoder. This function is used to fetch yuv plane sizes for memory > > allocation > > > https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrYUVProvider.c... > > > > I'm not sure what you're pointing out to me with the links. > > I agree that the first time this method is called, yuv_decoding_failed_ is > guaranteed to be false. Are you telling me that we will never call it a second > time? yes the above code tells it will never call in second time
scroggo@chromium.org changed reviewers: + bsalomon@google.com, reed@google.com
On 2017/06/16 00:12:28, naga wrote: > On 2017/06/15 20:40:01, scroggo_chromium wrote: > > > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp > (left): > > > > > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:204: > > yuv_decoding_failed_ = true; > > On 2017/06/15 17:49:02, naga wrote: > > > On 2017/06/15 16:50:38, scroggo_chromium wrote: > > > > > In the current code this condition will never fail > > > > > - if (yuv_decoding_failed_) > > > > > - return false; > > > > > So I removed in this patch. > > > > > > > > It sounds like you're saying that yuv_decoding_failed_ is never set to > true, > > > so > > > > we don't need to check its value? We set it to true right here, if there > was > > a > > > > failure. > > > yuv_decoding_failed _ is set to true once DecodeToYUV() is failed. > > > > Isn't that what I just said? This method is DecodeToYUV(). > > > > > > > > https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrYUVProvider.c... > > > > > > > > In our current source code, yuv_decoding_failed_ check present before > calling > > > the decoder. This function is used to fetch yuv plane sizes for memory > > > allocation > > > > > > https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrYUVProvider.c... > > > > > > > I'm not sure what you're pointing out to me with the links. > > > > I agree that the first time this method is called, yuv_decoding_failed_ is > > guaranteed to be false. Are you telling me that we will never call it a second > > time? > yes the above code tells it will never call in second time This is still not obvious to me. Would it be possible to add a test that confirms this? Maybe the exercise in trying to write a test will help verify that it cannot happen? (I suppose you'd need to have a broken JPEG image that we try to decode to YUV. We'd need to somehow force two attempts to draw, maybe by using different tiles, or drawing in different contexts?) Brian or Mike, do you know whether it's possible to attempt to YUV decode a second time? naga, I think you still need to separate out the code I suggested moving into its own CL.
https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:380: if (!decoder || !decoder->CanDecodeToYUV()) On 2017/06/15 14:45:22, scroggo_chromium wrote: > This change (moving caller of CanDecodeToYUV from UpdateYUVComponentSizes to > here) seems fine to me. This saves us the creation of dummy image planes if > CanDecodeToYUV returns false, and CanDecodeToYUV does not take into > consideration whether image planes are set. > > It seems unrelated to the other change though, and maybe belongs in its own CL. > naga, I think you still need to separate out the code I suggested > moving into its own CL. It looks like you've partially done that in crrev.com/2953023002, but you haven't removed it from here. This CL should contain just the one change.
On 2017/07/06 21:10:19, scroggo_chromium wrote: > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp > (right): > > https://codereview.chromium.org/2940593002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:380: if > (!decoder || !decoder->CanDecodeToYUV()) > On 2017/06/15 14:45:22, scroggo_chromium wrote: > > This change (moving caller of CanDecodeToYUV from UpdateYUVComponentSizes to > > here) seems fine to me. This saves us the creation of dummy image planes if > > CanDecodeToYUV returns false, and CanDecodeToYUV does not take into > > consideration whether image planes are set. > > > > It seems unrelated to the other change though, and maybe belongs in its own > CL. > > > naga, I think you still need to separate out the code I suggested > > moving into its own CL. > > It looks like you've partially done that in crrev.com/2953023002, but you > haven't removed it from here. This CL should contain just the one change. Removed the above changes in this patch |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
