Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1659873002:
Updates Piex and uses it to obtain the DNG dimensions. (Closed)
DescriptionUpdates Piex and uses it to obtain the DNG dimensions.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1659873002
Committed: https://skia.googlesource.com/skia/+/46d2aa824c0a0ee8218d90e821786bd51a63be1e
Patch Set 1 #Patch Set 2 : Fixes windows build #
Total comments: 20
Patch Set 3 : Worked through all comments. #Patch Set 4 : Merge with head. #Patch Set 5 : rebase #Patch Set 6 : Adds comment #Patch Set 7 : Updates the stream code. #Patch Set 8 : Adds unit test. Removes rotation code. #
Messages
Total messages: 37 (18 generated)
Description was changed from ========== Updates Piex and uses it to obtain the DNG dimensions. BUG=skia: ========== to ========== Updates Piex and uses it to obtain the DNG dimensions. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by ebrauer@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659873002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659873002/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-02-02 18:06 UTC
Description was changed from ========== Updates Piex and uses it to obtain the DNG dimensions. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Updates Piex and uses it to obtain the DNG dimensions. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
ebrauer@google.com changed reviewers: + adaubert@google.com, msarett@google.com, scroggo@google.com, yujieqin@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by ebrauer@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659873002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659873002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (left): https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:416: fNegative.reset(nullptr); Why is this no longer necessary? https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:308: * save time and storage to obtain the DNG dimensions and CFA pattern. Should there be a comment somewhere describing what a "CFA pattern" is? (Maybe it's common enough. When I looked up "demosaicing" (which always trips me up until I interpret it as "de-mosaicing" (both are acceptable, according to Wikipedia)), I see the following from Wikipedia: A demosaicing (also de-mosaicing, demosaicking or debayering) algorithm is a digital image process used to reconstruct a full color image from the incomplete color samples output from an image sensor overlaid with a color filter array (CFA). It is also known as CFA interpolation or color reconstruction.) https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:392: // The DNG SDK scales only for at demosaicing, so only when a mosaic info is available also I don't follow this sentence. I think you mean something like: // The DNG SDK only scales during demosaicing, so scaling is only possible when a mosaic info is available https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:400: if (::piex::IsRaw(fStream.get()) These two methods have already been called by SkRawCodec::NewFromStream, down below. Maybe SkRawStream should have a method that calls this and caches the interesting information. (On the other hand, maybe these methods are cheap, and it's simpler to call them again than to cache the values? It just seems weird that we need to call them twice.) https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:401: && ::piex::GetPreviewImageData(fStream.get(), &imageData) == ::piex::Error::kOk) { nit: Easier to follow along if you put the brace on the next line (since the if condition is two lines) https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:402: init(static_cast<int>(imageData.full_width), static_cast<int>(imageData.full_height), this->init https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:403: imageData.cfa_pattern_dim[0] * imageData.cfa_pattern_dim[1]); nit: This should either be indented 8 spaces, or to line up with static_cast, above. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:427: dng_point cfaPattern(0, 0); Nit: it appears that a more accurate name for this is cfaPatternSize? https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:431: init(static_cast<int>(fNegative->DefaultCropSizeH().As_real64()), this->init
https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:395: fIsXtransImage = fIsScalable ? cfaPatternSize == 36 : false; This seems like a possible behavior change to me? Is it possible for CFAPatternSize.v to be 3 and CFAPatternSize.h to be 12? Is this also an XtransImage?
https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (left): https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:416: fNegative.reset(nullptr); On 2016/02/03 15:56:02, scroggo wrote: > Why is this no longer necessary? This is unrelated to the cl, but it is somehow misleading to reset only fNegative and not all the other members. However, it is not necessary at all. The new code is easier to read. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:308: * save time and storage to obtain the DNG dimensions and CFA pattern. On 2016/02/03 15:56:02, scroggo wrote: > Should there be a comment somewhere describing what a "CFA pattern" is? > > (Maybe it's common enough. When I looked up "demosaicing" (which always trips me > up until I interpret it as "de-mosaicing" (both are acceptable, according to > Wikipedia)), I see the following from Wikipedia: > > A demosaicing (also de-mosaicing, demosaicking or debayering) algorithm is a > digital image process used to reconstruct a full color image from the incomplete > color samples output from an image sensor overlaid with a color filter array > (CFA). It is also known as CFA interpolation or color reconstruction.) Done. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:392: // The DNG SDK scales only for at demosaicing, so only when a mosaic info is available also On 2016/02/03 15:56:02, scroggo wrote: > I don't follow this sentence. I think you mean something like: > > // The DNG SDK only scales during demosaicing, so scaling is only possible when > a mosaic info is available Done. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:395: fIsXtransImage = fIsScalable ? cfaPatternSize == 36 : false; On 2016/02/03 16:21:49, msarett wrote: > This seems like a possible behavior change to me? > > Is it possible for CFAPatternSize.v to be 3 and CFAPatternSize.h to be 12? Is > this also an XtransImage? The pattern size is either 0, 2x2 or 6x6, but theoretically it is possible to have arbitrary patterns. I'll change this to something more robust. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:400: if (::piex::IsRaw(fStream.get()) On 2016/02/03 15:56:02, scroggo wrote: > These two methods have already been called by SkRawCodec::NewFromStream, down > below. Maybe SkRawStream should have a method that calls this and caches the > interesting information. (On the other hand, maybe these methods are cheap, and > it's simpler to call them again than to cache the values? It just seems weird > that we need to call them twice.) Yes, they are pretty cheap that is why we try to use them instead of the DNG SDK methods. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:401: && ::piex::GetPreviewImageData(fStream.get(), &imageData) == ::piex::Error::kOk) { On 2016/02/03 15:56:02, scroggo wrote: > nit: Easier to follow along if you put the brace on the next line (since the if > condition is two lines) Done. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:402: init(static_cast<int>(imageData.full_width), static_cast<int>(imageData.full_height), On 2016/02/03 15:56:02, scroggo wrote: > this->init Done. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:403: imageData.cfa_pattern_dim[0] * imageData.cfa_pattern_dim[1]); On 2016/02/03 15:56:02, scroggo wrote: > nit: This should either be indented 8 spaces, or to line up with static_cast, > above. Done. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:427: dng_point cfaPattern(0, 0); On 2016/02/03 15:56:02, scroggo wrote: > Nit: it appears that a more accurate name for this is cfaPatternSize? Done. https://codereview.chromium.org/1659873002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:431: init(static_cast<int>(fNegative->DefaultCropSizeH().As_real64()), On 2016/02/03 15:56:02, scroggo wrote: > this->init Done.
lgtm
The CQ bit was checked by ebrauer@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659873002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659873002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by ebrauer@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659873002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659873002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-...)
The CQ bit was checked by ebrauer@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659873002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659873002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Mac-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-Arm7...) Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by ebrauer@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659873002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659873002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ebrauer@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1659873002/#ps140001 (title: "Adds unit test. Removes rotation code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1659873002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1659873002/140001
Message was sent while issue was closed.
Description was changed from ========== Updates Piex and uses it to obtain the DNG dimensions. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Updates Piex and uses it to obtain the DNG dimensions. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/46d2aa824c0a0ee8218d90e821786bd51a63be1e ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/46d2aa824c0a0ee8218d90e821786bd51a63be1e |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
