Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2938593002:
Remove unwanted decoder failure check present in DecodeToYUV.
DescriptionRemove unwanted decoder failure check present in DecodeToYUV.
decode_failed_ variable is used only on DecodeAndScale. It's
not related yuv decoding so removing this condition check.
BUG=732769
Patch Set 1 : Initial patch to remove unwanted decoder failure check #
Total comments: 4
Messages
Total messages: 14 (7 generated)
Description was changed from ========== Remove unwanted decoder failure check present in DecodeToYUV. decode_failed_ variable is used only on DecodeAndScale. It's not related yuv decoding so removing this condition check. BUG=NONE ========== to ========== Remove unwanted decoder failure check present in DecodeToYUV. decode_failed_ variable is used only on DecodeAndScale. It's not related yuv decoding so removing this condition check. BUG=732769 R=SamsungPeerReview ==========
nagarajan.n@samsung.com changed reviewers: + shanmuga.m@samsung.com, srirama.m@samsung.com
Description was changed from ========== Remove unwanted decoder failure check present in DecodeToYUV. decode_failed_ variable is used only on DecodeAndScale. It's not related yuv decoding so removing this condition check. BUG=732769 R=SamsungPeerReview ========== to ========== Remove unwanted decoder failure check present in DecodeToYUV. decode_failed_ variable is used only on DecodeAndScale. It's not related yuv decoding so removing this condition check. BUG=732769 ==========
nagarajan.n@samsung.com changed reviewers: + noel@chromium.org
nagarajan.n@samsung.com changed reviewers: + scroggo@chromium.org
nagarajan.n@samsung.com changed reviewers: + chrishtr@chromium.org
nagarajan.n@samsung.com changed reviewers: + fmalita@chromium.org
https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:175: if (decode_failed_) > decode_failed_ variable is used only on DecodeAndScale. It's > not related yuv decoding so removing this condition check. The motivation for this check is that if the image fails to decode the old fashioned way, we don't want to bother attempting to decode to YUV. The TODO you're removing is to move this whole method to a different class, since it's not really related to ImageFrameGenerator. The method is only on ImageFrameGenerator because of the above - we don't want to try to YUV decode if the image is known to be broken. If we detect that another way, we can move the method somewhere more appropriate.
https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:175: if (decode_failed_) On 2017/06/15 14:50:33, scroggo_chromium wrote: > > decode_failed_ variable is used only on DecodeAndScale. It's > > not related yuv decoding so removing this condition check. > > The motivation for this check is that if the image fails to decode the old > fashioned way, we don't want to bother attempting to decode to YUV. > > The TODO you're removing is to move this whole method to a different class, > since it's not really related to ImageFrameGenerator. The method is only on > ImageFrameGenerator because of the above - we don't want to try to YUV decode if > the image is known to be broken. If we detect that another way, we can move the > method somewhere more appropriate. In the current code decode_failed_ variable is not related to yuv decoding. This condition unnecessary. This condtion will never fail for jpeg to yuv decoding
https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:175: if (decode_failed_) On 2017/06/15 15:05:25, naga wrote: > On 2017/06/15 14:50:33, scroggo_chromium wrote: > > > decode_failed_ variable is used only on DecodeAndScale. It's > > > not related yuv decoding so removing this condition check. > > > > The motivation for this check is that if the image fails to decode the old > > fashioned way, we don't want to bother attempting to decode to YUV. > > > > The TODO you're removing is to move this whole method to a different class, > > since it's not really related to ImageFrameGenerator. The method is only on > > ImageFrameGenerator because of the above - we don't want to try to YUV decode > if > > the image is known to be broken. If we detect that another way, we can move > the > > method somewhere more appropriate. > > In the current code decode_failed_ variable is not related to yuv decoding. I disagree. Here's how it's related: If decode_failed_ is true, that means we attempted to decode it before, and we failed. Presumably YUV decoding will also fail, so we do not bother trying. > This condition unnecessary. This condtion will never fail for jpeg to yuv > decoding We only attempt to decode to YUV if the image is complete. Would it be possible to start decoding an image that is not complete, and then attempt to YUV decode later? If so, I think we still want this check. If it's not possible, where do we prevent it from happening? If you're correct, and we don't need this check, then please move the code into DecodingImageGenerator (its only caller). It is unrelated to the general purpose of ImageFrameGenerator.
https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:175: if (decode_failed_) On 2017/06/15 16:46:19, scroggo_chromium wrote: > On 2017/06/15 15:05:25, naga wrote: > > On 2017/06/15 14:50:33, scroggo_chromium wrote: > > > > decode_failed_ variable is used only on DecodeAndScale. It's > > > > not related yuv decoding so removing this condition check. > > > > > > The motivation for this check is that if the image fails to decode the old > > > fashioned way, we don't want to bother attempting to decode to YUV. > > > > > > The TODO you're removing is to move this whole method to a different class, > > > since it's not really related to ImageFrameGenerator. The method is only on > > > ImageFrameGenerator because of the above - we don't want to try to YUV > decode > > if > > > the image is known to be broken. If we detect that another way, we can move > > the > > > method somewhere more appropriate. > > > > In the current code decode_failed_ variable is not related to yuv decoding. > > I disagree. Here's how it's related: If decode_failed_ is true, that means we > attempted to decode it before, and we failed. Presumably YUV decoding will also > fail, so we do not bother trying. > > > This condition unnecessary. This condtion will never fail for jpeg to yuv > > decoding > > We only attempt to decode to YUV if the image is complete. Would it be possible > to start decoding an image that is not complete, and then attempt to YUV decode > later? If so, I think we still want this check. > > If it's not possible, where do we prevent it from happening? > > If you're correct, and we don't need this check, then please move the code into > DecodingImageGenerator (its only caller). It is unrelated to the general purpose > of ImageFrameGenerator. decode_failed_ variable status is updated on DecodeAndScale()->TryToResumeDecode(). https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... The above function called only in OnGetPixels(). This will not come under jpeg to yuv decoding code flow. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... For yuv decoding we used to call onGetYUV8Planes(). https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
On 2017/06/15 16:59:20, naga wrote: > https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (left): > > https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:175: if > (decode_failed_) > On 2017/06/15 16:46:19, scroggo_chromium wrote: > > On 2017/06/15 15:05:25, naga wrote: > > > On 2017/06/15 14:50:33, scroggo_chromium wrote: > > > > > decode_failed_ variable is used only on DecodeAndScale. It's > > > > > not related yuv decoding so removing this condition check. > > > > > > > > The motivation for this check is that if the image fails to decode the old > > > > fashioned way, we don't want to bother attempting to decode to YUV. > > > > > > > > The TODO you're removing is to move this whole method to a different > class, > > > > since it's not really related to ImageFrameGenerator. The method is only > on > > > > ImageFrameGenerator because of the above - we don't want to try to YUV > > decode > > > if > > > > the image is known to be broken. If we detect that another way, we can > move > > > the > > > > method somewhere more appropriate. > > > > > > In the current code decode_failed_ variable is not related to yuv decoding. > > > > I disagree. Here's how it's related: If decode_failed_ is true, that means we > > attempted to decode it before, and we failed. Presumably YUV decoding will > also > > fail, so we do not bother trying. > > > > > This condition unnecessary. This condtion will never fail for jpeg to yuv > > > decoding > > > > We only attempt to decode to YUV if the image is complete. Would it be > possible > > to start decoding an image that is not complete, and then attempt to YUV > decode > > later? If so, I think we still want this check. > > > > If it's not possible, where do we prevent it from happening? > > > > If you're correct, and we don't need this check, then please move the code > into > > DecodingImageGenerator (its only caller). It is unrelated to the general > purpose > > of ImageFrameGenerator. > > decode_failed_ variable status is updated on > DecodeAndScale()->TryToResumeDecode(). > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > > The above function called only in OnGetPixels(). This will not come under jpeg > to yuv decoding code flow. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > For yuv decoding we used to call onGetYUV8Planes(). > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... Right, I understand that. My question is, is it possible to call OnGetPixels, get a failure, then download more data, then call onGetYUV8Planes? If it's not possible, then it would make sense to remove this code.
On 2017/06/15 17:09:32, scroggo_chromium wrote: > On 2017/06/15 16:59:20, naga wrote: > > > https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... > > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp > (left): > > > > > https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:175: if > > (decode_failed_) > > On 2017/06/15 16:46:19, scroggo_chromium wrote: > > > On 2017/06/15 15:05:25, naga wrote: > > > > On 2017/06/15 14:50:33, scroggo_chromium wrote: > > > > > > decode_failed_ variable is used only on DecodeAndScale. It's > > > > > > not related yuv decoding so removing this condition check. > > > > > > > > > > The motivation for this check is that if the image fails to decode the > old > > > > > fashioned way, we don't want to bother attempting to decode to YUV. > > > > > > > > > > The TODO you're removing is to move this whole method to a different > > class, > > > > > since it's not really related to ImageFrameGenerator. The method is only > > on > > > > > ImageFrameGenerator because of the above - we don't want to try to YUV > > > decode > > > > if > > > > > the image is known to be broken. If we detect that another way, we can > > move > > > > the > > > > > method somewhere more appropriate. > > > > > > > > In the current code decode_failed_ variable is not related to yuv > decoding. > > > > > > I disagree. Here's how it's related: If decode_failed_ is true, that means > we > > > attempted to decode it before, and we failed. Presumably YUV decoding will > > also > > > fail, so we do not bother trying. > > > > > > > This condition unnecessary. This condtion will never fail for jpeg to yuv > > > > decoding > > > > > > We only attempt to decode to YUV if the image is complete. Would it be > > possible > > > to start decoding an image that is not complete, and then attempt to YUV > > decode > > > later? If so, I think we still want this check. > > > > > > If it's not possible, where do we prevent it from happening? > > > > > > If you're correct, and we don't need this check, then please move the code > > into > > > DecodingImageGenerator (its only caller). It is unrelated to the general > > purpose > > > of ImageFrameGenerator. > > > > decode_failed_ variable status is updated on > > DecodeAndScale()->TryToResumeDecode(). > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > > > > > The above function called only in OnGetPixels(). This will not come under jpeg > > to yuv decoding code flow. > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > > > For yuv decoding we used to call onGetYUV8Planes(). > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > Right, I understand that. My question is, is it possible to call OnGetPixels, > get a failure, then download more data, then call onGetYUV8Planes? If it's not > possible, then it would make sense to remove this code. I don't think it's possible. The above use case is not required.
On 2017/06/15 17:33:01, naga wrote: > On 2017/06/15 17:09:32, scroggo_chromium wrote: > > On 2017/06/15 16:59:20, naga wrote: > > > > > > https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... > > > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp > > (left): > > > > > > > > > https://codereview.chromium.org/2938593002/diff/1/third_party/WebKit/Source/p... > > > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:175: if > > > (decode_failed_) > > > On 2017/06/15 16:46:19, scroggo_chromium wrote: > > > > On 2017/06/15 15:05:25, naga wrote: > > > > > On 2017/06/15 14:50:33, scroggo_chromium wrote: > > > > > > > decode_failed_ variable is used only on DecodeAndScale. It's > > > > > > > not related yuv decoding so removing this condition check. > > > > > > > > > > > > The motivation for this check is that if the image fails to decode the > > old > > > > > > fashioned way, we don't want to bother attempting to decode to YUV. > > > > > > > > > > > > The TODO you're removing is to move this whole method to a different > > > class, > > > > > > since it's not really related to ImageFrameGenerator. The method is > only > > > on > > > > > > ImageFrameGenerator because of the above - we don't want to try to YUV > > > > decode > > > > > if > > > > > > the image is known to be broken. If we detect that another way, we can > > > move > > > > > the > > > > > > method somewhere more appropriate. > > > > > > > > > > In the current code decode_failed_ variable is not related to yuv > > decoding. > > > > > > > > I disagree. Here's how it's related: If decode_failed_ is true, that means > > we > > > > attempted to decode it before, and we failed. Presumably YUV decoding will > > > also > > > > fail, so we do not bother trying. > > > > > > > > > This condition unnecessary. This condtion will never fail for jpeg to > yuv > > > > > decoding > > > > > > > > We only attempt to decode to YUV if the image is complete. Would it be > > > possible > > > > to start decoding an image that is not complete, and then attempt to YUV > > > decode > > > > later? If so, I think we still want this check. > > > > > > > > If it's not possible, where do we prevent it from happening? > > > > > > > > If you're correct, and we don't need this check, then please move the code > > > into > > > > DecodingImageGenerator (its only caller). It is unrelated to the general > > > purpose > > > > of ImageFrameGenerator. > > > > > > decode_failed_ variable status is updated on > > > DecodeAndScale()->TryToResumeDecode(). > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > > > > > > > > The above function called only in OnGetPixels(). This will not come under > jpeg > > > to yuv decoding code flow. > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > > > > > For yuv decoding we used to call onGetYUV8Planes(). > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > > > Right, I understand that. My question is, is it possible to call OnGetPixels, > > get a failure, then download more data, then call onGetYUV8Planes? If it's not > > possible, then it would make sense to remove this code. > > I don't think it's possible. The above use case is not required. You don't think so? It'd be nice to verify. Try partially loading a broken JPEG image, attempting to draw it (meaning we'll go through onGetPixels and fail to decode), supplying the rest of the data, and then seeing if we go through YUV. If we do, then we could have aborted early. The check was introduced with crrev.com/496413002. It's not specifically discussed in the code review, but I assume we were trying to prevent unnecessary work in the case where it had already failed. Maybe a good argument for removing this line is that it's a rare case (I don't know whether it is or not, though), but that's not what your CL description suggests. Even if we do remove it for that reason, it doesn't seem to make the code significantly simpler, and I don't think the single if statement is hurting performance. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
