Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1883783002:
Update PIEX version and adjust the RAW codec accordingly (Closed)
DescriptionSince PIEX can now return an uncompressed RGB thumbnail, check the type of the image before treating it as a JPEG.
Highlights of new PIEX:
* PIEX can now return JPEG compressed image or uncompressed RGB image
* Add IsOfType() and GEtNumberofBytesForIsOfType() to image_type_recognition_lite
* Add GetDngInformation() and GetOrientation() to piex
* Remove deprecated data entries from piex_types
BUG=b/27214608, b/28119810
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1883783002
Committed: https://skia.googlesource.com/skia/+/e34635dee169b8710141d6d0841db1e2bde9626b
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix comments #Patch Set 3 : Fix comments #Patch Set 4 : Use new PIEX #Messages
Total messages: 25 (12 generated)
Description was changed from ========== Update PIEX version and adjust the RAW codec accordingly * PIEX can now return JPEG compressed image or uncompressed RGB image * Add IsOfType() and GEtNumberofBytesForIsOfType() to image_type_recognition_lite * Add GetDngInformation() and GetOrientation() to piex * Remove deprecated data entries from piex_types BUG=b/27214608, b/28119810 ========== to ========== Update PIEX version and adjust the RAW codec accordingly * PIEX can now return JPEG compressed image or uncompressed RGB image * Add IsOfType() and GEtNumberofBytesForIsOfType() to image_type_recognition_lite * Add GetDngInformation() and GetOrientation() to piex * Remove deprecated data entries from piex_types BUG=b/27214608, b/28119810 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Update PIEX version and adjust the RAW codec accordingly * PIEX can now return JPEG compressed image or uncompressed RGB image * Add IsOfType() and GEtNumberofBytesForIsOfType() to image_type_recognition_lite * Add GetDngInformation() and GetOrientation() to piex * Remove deprecated data entries from piex_types BUG=b/27214608, b/28119810 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Update PIEX version and adjust the RAW codec accordingly * PIEX can now return JPEG compressed image or uncompressed RGB image * Add IsOfType() and GEtNumberofBytesForIsOfType() to image_type_recognition_lite * Add GetDngInformation() and GetOrientation() to piex * Remove deprecated data entries from piex_types BUG=b/27214608, b/28119810 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
yujieqin@google.com changed reviewers: + msarett@google.com, scroggo@google.com
The CQ bit was checked by yujieqin@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/1883783002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883783002/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-04-13 15:13 UTC
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-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:643: // PIEX can return JPEG compressed image or uncompressed RGB image. We only handle the JPEG I'm confused. What is the "uncompressed RGB image"? Isn't that what the DNG SDK returns?
https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:643: // PIEX can return JPEG compressed image or uncompressed RGB image. We only handle the JPEG There are two kind of thumbnails image can be embeded into a RAW file: * JPEG compressed * plain uncompressed RGB data (e.g. DNG from mobile or some NEF contains uncompressed thumbnail) Before this change, PIEX only return image (thumbnail / preview) if it is JPEG compressed. The uncompressed case will be ignore. The new PIEX can also return image (thumbnail / preview) if it is aplain uncompressed RGB. We should add this format check here to ensure we only call the JPEG codec if it is really a JPEG.
yujieqin@google.com changed reviewers: + ebrauer@google.com
> Update PIEX version and adjust the RAW codec accordingly > > * PIEX can now return JPEG compressed image or uncompressed RGB image nit: Most of these comments seem to relate to PIEX, but don't really appear to affect how Skia uses it. It's not clear to me they all belong here. I think it would be more interesting to say how you changed Skia. This sentence almost does. Maybe this should say something like: "Since PIEX can now return an uncompressed RGB thumbnail, check the type of the thumbnail before treating it as a JPEG." (Maybe also include a FIXME/TODO to handle the RGB thumbnail, if you plan to add that/think it might be better?) > * Add IsOfType() and GEtNumberofBytesForIsOfType() to > image_type_recognition_lite > * Add GetDngInformation() and GetOrientation() to piex FYI: Matt has recently added the ability (and an API) to retrieve an orientation (see crrev.com/1813273002). We should add support in Skia for RAW. > * Remove deprecated data entries from piex_types 1gtm once you fix the build failures. https://codereview.chromium.org/1883783002/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/1883783002/diff/1/DEPS#newcode25 DEPS:25: "third_party/externals/piex" : "https://android.googlesource.com/platform/external/piex.git@2f5d8eafb5648056703d6b3dec237d293f75f99e", In the future, it might be a good idea to update here before updating nyc-dev so you can get some feedback. It would at least be good to keep them in sync as much as you can. We've missed bugs in libpng because our tests weren't testing the latest version shipping in Android :( https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:643: // PIEX can return JPEG compressed image or uncompressed RGB image. We only handle the JPEG On 2016/04/13 13:11:29, yujieqin wrote: > There are two kind of thumbnails image can be embeded into a RAW file: > * JPEG compressed > * plain uncompressed RGB data (e.g. DNG from mobile or some NEF contains > uncompressed thumbnail) > > Before this change, PIEX only return image (thumbnail / preview) if it is JPEG > compressed. The uncompressed case will be ignore. > > The new PIEX can also return image (thumbnail / preview) if it is aplain > uncompressed RGB. We should add this format check here to ensure we only call > the JPEG codec if it is really a JPEG. Ah, this probably is more obvious to someone who is more familiar with piex. Could you replace "image" with "thumbnail" in the comment? So it would instead say: PIEX can return JPEG compressed thumbnail or uncompressed RGB thumbnail
Description was changed from ========== Update PIEX version and adjust the RAW codec accordingly * PIEX can now return JPEG compressed image or uncompressed RGB image * Add IsOfType() and GEtNumberofBytesForIsOfType() to image_type_recognition_lite * Add GetDngInformation() and GetOrientation() to piex * Remove deprecated data entries from piex_types BUG=b/27214608, b/28119810 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Since PIEX can now return an uncompressed RGB thumbnail, check the type of the image before treating it as a JPEG. Highlights of new PIEX: * PIEX can now return JPEG compressed image or uncompressed RGB image * Add IsOfType() and GEtNumberofBytesForIsOfType() to image_type_recognition_lite * Add GetDngInformation() and GetOrientation() to piex * Remove deprecated data entries from piex_types BUG=b/27214608, b/28119810 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1883783002/diff/1/DEPS File DEPS (right): https://codereview.chromium.org/1883783002/diff/1/DEPS#newcode25 DEPS:25: "third_party/externals/piex" : "https://android.googlesource.com/platform/external/piex.git@2f5d8eafb5648056703d6b3dec237d293f75f99e", Totally agree!!! It was my fault this time to rush into nyc-dev before doing here. https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:643: // PIEX can return JPEG compressed image or uncompressed RGB image. We only handle the JPEG Actually it is not only for "thumbnail". Theoretically a preview can also be "uncompressed", e.g. one can crate such DNG with little effort. That is why I said "images". In real world, we don't see any uncompressed preview case. And the current change for PIEX is for getting uncompressed thumbnail only. We don't plan to add support for uncompressed preview so far.
lgtm https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1883783002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:643: // PIEX can return JPEG compressed image or uncompressed RGB image. We only handle the JPEG On 2016/04/14 07:54:59, yujieqin wrote: > Actually it is not only for "thumbnail". Theoretically a preview can also be > "uncompressed", e.g. one can crate such DNG with little effort. That is why I > said "images". > > In real world, we don't see any uncompressed preview case. And the current > change for PIEX is for getting uncompressed thumbnail only. We don't plan to add > support for uncompressed preview so far. Ah, thanks for clarifying.
The CQ bit was checked by yujieqin@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/1883783002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883783002/60001
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 yujieqin@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/1883783002/#ps60001 (title: "Use new PIEX")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883783002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883783002/60001
Message was sent while issue was closed.
Description was changed from ========== Since PIEX can now return an uncompressed RGB thumbnail, check the type of the image before treating it as a JPEG. Highlights of new PIEX: * PIEX can now return JPEG compressed image or uncompressed RGB image * Add IsOfType() and GEtNumberofBytesForIsOfType() to image_type_recognition_lite * Add GetDngInformation() and GetOrientation() to piex * Remove deprecated data entries from piex_types BUG=b/27214608, b/28119810 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Since PIEX can now return an uncompressed RGB thumbnail, check the type of the image before treating it as a JPEG. Highlights of new PIEX: * PIEX can now return JPEG compressed image or uncompressed RGB image * Add IsOfType() and GEtNumberofBytesForIsOfType() to image_type_recognition_lite * Add GetDngInformation() and GetOrientation() to piex * Remove deprecated data entries from piex_types BUG=b/27214608, b/28119810 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e34635dee169b8710141d6d0841db1e2bde9626b ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/e34635dee169b8710141d6d0841db1e2bde9626b |
