Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529543003:
Add inline JPEGs. (Closed)
DescriptionAdd inline JPEGs.
Allows JPEG data to be copied into the file rather than left in a
separate file. This is needed to allow rasterized PDFs to avoid saving
image files for each page. See Chromium issue 2524143003 for chromium
changes.
BUG=534945, 550205, 480628
Review-Url: https://codereview.chromium.org/2529543003
Committed: https://pdfium.googlesource.com/pdfium/+/f085db37c21ad2bd66e349f9307fc89b426217f5
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix rebase #
Total comments: 8
Patch Set 4 : Add new API for inline JPEGs #
Total comments: 2
Patch Set 5 : Consolidate common code #
Total comments: 2
Patch Set 6 : Return to 2 functions #
Total comments: 1
Patch Set 7 : Fix extra arg #
Messages
Total messages: 28 (16 generated)
The CQ bit was checked by rbpotter@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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-pdfium-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Add inline JPEGs. Allows JPEG data to be copied into the file rather than left in a separate file. This is needed to allow rasterized PDFs to avoid saving image files for each page. See Chromium issue 2524143003 for chromium changes. BUG=534945, 550205, 480628 ========== to ========== Add inline JPEGs. Allows JPEG data to be copied into the file rather than left in a separate file. This is needed to allow rasterized PDFs to avoid saving image files for each page. See Chromium issue 2524143003 for chromium changes. BUG=534945, 550205, 480628 ==========
rbpotter@chromium.org changed reviewers: + weili@chromium.org
See issue description. If you aren't the right person to review this, let me know. Thanks!
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
https://codereview.chromium.org/2529543003/diff/40001/fpdfsdk/fpdfeditimg.cpp File fpdfsdk/fpdfeditimg.cpp (right): https://codereview.chromium.org/2529543003/diff/40001/fpdfsdk/fpdfeditimg.cpp... fpdfsdk/fpdfeditimg.cpp:43: pImgObj->GetImage()->SetJpegImageInline(pFile); If this has side-effects we probably don't want to modify the existing public API as there maybe existing callers depending on the current behaviour. It would be better to add a second API.
weili@google.com changed reviewers: + weili@google.com
Adding a new API looks good. https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... File core/fpdfapi/page/cpdf_image.cpp (right): https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... core/fpdfapi/page/cpdf_image.cpp:145: uint32_t size = (uint32_t)pFile->GetSize(); Avoid C-cast here pls https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... core/fpdfapi/page/cpdf_image.cpp:157: m_pStream->InitStream(&(data[0]), size, std::move(pDict)); Seems this line is the only diff with SetJpegImage() function. Maybe combine them into one? https://codereview.chromium.org/2529543003/diff/60001/fpdfsdk/fpdfeditimg.cpp File fpdfsdk/fpdfeditimg.cpp (right): https://codereview.chromium.org/2529543003/diff/60001/fpdfsdk/fpdfeditimg.cpp... fpdfsdk/fpdfeditimg.cpp:49: FPDFImageObj_LoadJpegFileInline(FPDF_PAGE* pages, Can this and FPDFImageObj_LoadJpegFile() share common code?
https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... File core/fpdfapi/page/cpdf_image.cpp (right): https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... core/fpdfapi/page/cpdf_image.cpp:145: uint32_t size = (uint32_t)pFile->GetSize(); On 2016/12/13 21:55:01, weili wrote: > Avoid C-cast here pls Done. https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... core/fpdfapi/page/cpdf_image.cpp:157: m_pStream->InitStream(&(data[0]), size, std::move(pDict)); On 2016/12/13 21:55:00, weili wrote: > Seems this line is the only diff with SetJpegImage() function. Maybe combine > them into one? There's actually another diff (the size), but I was able to mostly combine them - let me know if you prefer it this way or if I should go back to 2 different functions. https://codereview.chromium.org/2529543003/diff/40001/fpdfsdk/fpdfeditimg.cpp File fpdfsdk/fpdfeditimg.cpp (right): https://codereview.chromium.org/2529543003/diff/40001/fpdfsdk/fpdfeditimg.cpp... fpdfsdk/fpdfeditimg.cpp:43: pImgObj->GetImage()->SetJpegImageInline(pFile); On 2016/12/13 20:37:14, dsinclair wrote: > If this has side-effects we probably don't want to modify the existing public > API as there maybe existing callers depending on the current behaviour. It > would be better to add a second API. Done. https://codereview.chromium.org/2529543003/diff/60001/fpdfsdk/fpdfeditimg.cpp File fpdfsdk/fpdfeditimg.cpp (right): https://codereview.chromium.org/2529543003/diff/60001/fpdfsdk/fpdfeditimg.cpp... fpdfsdk/fpdfeditimg.cpp:49: FPDFImageObj_LoadJpegFileInline(FPDF_PAGE* pages, On 2016/12/13 21:55:01, weili wrote: > Can this and FPDFImageObj_LoadJpegFile() share common code? Done.
Looks good otherwise https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... File core/fpdfapi/page/cpdf_image.cpp (right): https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... core/fpdfapi/page/cpdf_image.cpp:157: m_pStream->InitStream(&(data[0]), size, std::move(pDict)); On 2016/12/14 01:03:28, rbpotter wrote: > On 2016/12/13 21:55:00, weili wrote: > > Seems this line is the only diff with SetJpegImage() function. Maybe combine > > them into one? > > There's actually another diff (the size), but I was able to mostly combine them > - let me know if you prefer it this way or if I should go back to 2 different > functions. Sorry, didn't realize the size diff. Looks like keeping them separate looks clearer.
https://codereview.chromium.org/2529543003/diff/80001/core/fpdfapi/page/cpdf_... File core/fpdfapi/page/cpdf_image.cpp (right): https://codereview.chromium.org/2529543003/diff/80001/core/fpdfapi/page/cpdf_... core/fpdfapi/page/cpdf_image.cpp:136: if (!pDict && size > dwEstimateSize) { Also, if we use this merged function, not change 'dwEstimateSize', just use "if ((!pDict || inlineImage) && size > dwEstimateSize)" here. That may be a bit clearer.
https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... File core/fpdfapi/page/cpdf_image.cpp (right): https://codereview.chromium.org/2529543003/diff/40001/core/fpdfapi/page/cpdf_... core/fpdfapi/page/cpdf_image.cpp:157: m_pStream->InitStream(&(data[0]), size, std::move(pDict)); On 2016/12/14 01:33:00, Wei Li wrote: > On 2016/12/14 01:03:28, rbpotter wrote: > > On 2016/12/13 21:55:00, weili wrote: > > > Seems this line is the only diff with SetJpegImage() function. Maybe combine > > > them into one? > > > > There's actually another diff (the size), but I was able to mostly combine > them > > - let me know if you prefer it this way or if I should go back to 2 different > > functions. > > Sorry, didn't realize the size diff. Looks like keeping them separate looks > clearer. Done. https://codereview.chromium.org/2529543003/diff/80001/core/fpdfapi/page/cpdf_... File core/fpdfapi/page/cpdf_image.cpp (right): https://codereview.chromium.org/2529543003/diff/80001/core/fpdfapi/page/cpdf_... core/fpdfapi/page/cpdf_image.cpp:136: if (!pDict && size > dwEstimateSize) { On 2016/12/14 17:29:38, Wei Li wrote: > Also, if we use this merged function, not change 'dwEstimateSize', just use "if > ((!pDict || inlineImage) && size > dwEstimateSize)" here. That may be a bit > clearer. Acknowledged.
lgtm https://codereview.chromium.org/2529543003/diff/100001/core/fpdfapi/page/cpdf... File core/fpdfapi/page/cpdf_image.cpp (right): https://codereview.chromium.org/2529543003/diff/100001/core/fpdfapi/page/cpdf... core/fpdfapi/page/cpdf_image.cpp:121: bool inlineImage) { |inlineImage| is no longer needed
lgtm
The CQ bit was checked by rbpotter@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: This issue passed the CQ dry run.
The CQ bit was checked by dsinclair@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from weili@chromium.org Link to the patchset: https://codereview.chromium.org/2529543003/#ps120001 (title: "Fix extra arg")
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": 120001, "attempt_start_ts": 1481744655067190,
"parent_rev": "0d73909e89a5c93917b9cb73fe5c03c484f2793d", "commit_rev":
"f085db37c21ad2bd66e349f9307fc89b426217f5"}
Message was sent while issue was closed.
Description was changed from ========== Add inline JPEGs. Allows JPEG data to be copied into the file rather than left in a separate file. This is needed to allow rasterized PDFs to avoid saving image files for each page. See Chromium issue 2524143003 for chromium changes. BUG=534945, 550205, 480628 ========== to ========== Add inline JPEGs. Allows JPEG data to be copied into the file rather than left in a separate file. This is needed to allow rasterized PDFs to avoid saving image files for each page. See Chromium issue 2524143003 for chromium changes. BUG=534945, 550205, 480628 Review-Url: https://codereview.chromium.org/2529543003 Committed: https://pdfium.googlesource.com/pdfium/+/f085db37c21ad2bd66e349f9307fc89b4262... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://pdfium.googlesource.com/pdfium/+/f085db37c21ad2bd66e349f9307fc89b4262... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
