Pdfviewer refactoring.
Mostly superficial changes, to help me make sure I understand the
code while making modifications.
SkPdfRenderer:
First class I'm modifying. Move it into include/ and src/ directories.
Inherit from SkNoncopyable.
Replace load() with factory function which returns NULL if the load
fails.
Remove unload() and loaded(), which no longer make sense, since the
factory will return NULL on a failure to load, and unload() happens
on destruction.
Use a const char* for loading a PDF, following the convention of
SkStream::NewFromFile.
Remove unnecessary call to sqrt in SkPDFNativeRenderToBitmap.
Also in SkPDFNativeRenderToBitmap, use an appropriate SkScalar macro
to convert to an integer.
Use this-> when calling member functions.
pdf_viewer_main.cpp:
Call the new interface for SkPdfRenderer.
gyp files:
Refer to the new location of SkPdfRenderer.
R=edisonn@google.com
Review URL: https://codereview.chromium.org/59493011
git-svn-id: http://skia.googlecode.com/svn/trunk@12296 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/experimental/PdfViewer/SkPdfRenderer.h b/experimental/PdfViewer/inc/SkPdfRenderer.h
similarity index 79%
rename from experimental/PdfViewer/SkPdfRenderer.h
rename to experimental/PdfViewer/inc/SkPdfRenderer.h
index 51b5649..c978a73 100644
--- a/experimental/PdfViewer/SkPdfRenderer.h
+++ b/experimental/PdfViewer/inc/SkPdfRenderer.h
@@ -9,17 +9,16 @@
#ifndef SkPdfRenderer_DEFINED
#define SkPdfRenderer_DEFINED
-// TODO(edisonn): remove this dependency, and load only from a stream!
-#include "SkString.h"
+#include "SkTypes.h"
class SkBitmap;
class SkCanvas;
class SkPdfNativeDoc;
struct SkRect;
class SkStream;
-class SkString;
// What kind of content to render.
+// FIXME: Currently unused.
enum SkPdfContent {
kNoForms_SkPdfContent,
kAll_SkPdfContent,
@@ -30,27 +29,21 @@
* The SkPdfRenderer class is used to render a PDF into canvas.
*
*/
-class SkPdfRenderer {
+class SkPdfRenderer : public SkNoncopyable {
public:
- SkPdfRenderer() : fPdfDoc(NULL) {}
- virtual ~SkPdfRenderer() {unload();}
+ // Create a new renderer from a stream.
+ // TODO(edisonn): replace it with a SkSmartStream which would know to to efficiently
+ // deal with a HTTP stream.
+ // FIXME: Untested.
+ static SkPdfRenderer* CreateFromStream(SkStream*);
+ // Create a new renderer from a file.
+ static SkPdfRenderer* CreateFromFile(const char* filename);
+
+ ~SkPdfRenderer();
// Render a specific page into the canvas, in a specific rectangle.
bool renderPage(int page, SkCanvas* canvas, const SkRect& dst) const;
- // TODO(edisonn): deprecated, should be removed!
- bool load(const SkString inputFileName);
-
- // TODO(edisonn): replace it with a SkSmartStream which would know to to efficiently
- // deal with a HTTP stream.
- bool load(SkStream* stream);
-
- // Unloads the pdf document.
- void unload();
-
- // Returns true if we succesfully loaded a document.
- bool loaded() const {return fPdfDoc != NULL && pages() > 0;}
-
// Returns the number of pages in the loaded pdf.
int pages() const;
@@ -62,6 +55,8 @@
size_t bytesUsed() const;
private:
+ // Takes ownership of SkPdfNativeDoc.
+ SkPdfRenderer(SkPdfNativeDoc*);
SkPdfNativeDoc* fPdfDoc;
};
diff --git a/experimental/PdfViewer/pdf_viewer_main.cpp b/experimental/PdfViewer/pdf_viewer_main.cpp
index 6f8fb53..5ecd2db 100644
--- a/experimental/PdfViewer/pdf_viewer_main.cpp
+++ b/experimental/PdfViewer/pdf_viewer_main.cpp
@@ -214,81 +214,67 @@
/** Reads an skp file, renders it to pdf and writes the output to a pdf file
* @param inputPath The skp file to be read.
* @param outputDir Output dir.
- * @param renderer The object responsible to render the skp object into pdf.
*/
-static bool process_pdf(const SkString& inputPath, const SkString& outputDir,
- SkPdfRenderer& renderer) {
+static bool process_pdf(const SkString& inputPath, const SkString& outputDir) {
SkDebugf("Loading PDF: %s\n", inputPath.c_str());
SkString inputFilename = SkOSPath::SkBasename(inputPath.c_str());
- bool success = true;
+ SkAutoTDelete<SkPdfRenderer> renderer(SkPdfRenderer::CreateFromFile(inputPath.c_str()));
+ if (NULL == renderer.get()) {
+ SkDebugf("Failure loading file %s\n", inputPath.c_str());
+ return false;
+ }
- success = renderer.load(inputPath);
if (FLAGS_showMemoryUsage) {
- SkDebugf("Memory usage after load: %u\n", (unsigned int)renderer.bytesUsed());
+ SkDebugf("Memory usage after load: %u\n", (unsigned int) renderer->bytesUsed());
}
// TODO(edisonn): bench timers
if (FLAGS_benchLoad > 0) {
for (int i = 0 ; i < FLAGS_benchLoad; i++) {
- success = renderer.load(inputPath) && success;
- if (FLAGS_showMemoryUsage) {
+ SkAutoTDelete<SkPdfRenderer> benchRenderer(
+ SkPdfRenderer::CreateFromFile(inputPath.c_str()));
+ if (NULL == benchRenderer.get()) {
+ SkDebugf("Failed to load on %ith attempt\n", i);
+ } else if (FLAGS_showMemoryUsage) {
SkDebugf("Memory usage after load %i number : %u\n", i,
- (unsigned int)renderer.bytesUsed());
+ (unsigned int) benchRenderer->bytesUsed());
}
}
}
- if (success) {
- if (!renderer.pages())
- {
- SkDebugf("ERROR: Empty PDF Document %s\n", inputPath.c_str());
- return false;
- } else {
- for (int i = 0; i < FLAGS_benchRender + 1; i++) {
- // TODO(edisonn) if (i == 1) start timer
- if (strcmp(FLAGS_pages[0], "all") == 0) {
- for (int pn = 0; pn < renderer.pages(); ++pn) {
- success = render_page(
- outputDir,
- inputFilename,
- renderer,
- FLAGS_noExtensionForOnePagePdf && renderer.pages() == 1 ? -1 :
- pn) &&
- success;
- }
- } else if (strcmp(FLAGS_pages[0], "reverse") == 0) {
- for (int pn = renderer.pages() - 1; pn >= 0; --pn) {
- success = render_page(
- outputDir,
- inputFilename,
- renderer,
- FLAGS_noExtensionForOnePagePdf && renderer.pages() == 1 ? -1 :
- pn) &&
- success;
- }
- } else if (strcmp(FLAGS_pages[0], "first") == 0) {
- success = render_page(
- outputDir,
- inputFilename,
- renderer,
- FLAGS_noExtensionForOnePagePdf && renderer.pages() == 1 ? -1 : 0) &&
- success;
- } else if (strcmp(FLAGS_pages[0], "last") == 0) {
- success = render_page(
- outputDir,
- inputFilename,
- renderer,
- FLAGS_noExtensionForOnePagePdf &&
- renderer.pages() == 1 ? -1 : renderer.pages() - 1) && success;
- } else {
- int pn = atoi(FLAGS_pages[0]);
- success = render_page(outputDir, inputFilename, renderer,
- FLAGS_noExtensionForOnePagePdf &&
- renderer.pages() == 1 ? -1 : pn) && success;
- }
+ if (!renderer->pages()) {
+ // This should never happen, since CreateFromFile will return NULL if there are no pages.
+ SkASSERT(false);
+ SkDebugf("ERROR: Empty PDF Document %s\n", inputPath.c_str());
+ return false;
+ }
+
+ bool success = true;
+ for (int i = 0; i < FLAGS_benchRender + 1; i++) {
+ // TODO(edisonn) if (i == 1) start timer
+ if (strcmp(FLAGS_pages[0], "all") == 0) {
+ for (int pn = 0; pn < renderer->pages(); ++pn) {
+ success &= render_page(outputDir, inputFilename, *renderer,
+ FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1 : pn);
}
+ } else if (strcmp(FLAGS_pages[0], "reverse") == 0) {
+ for (int pn = renderer->pages() - 1; pn >= 0; --pn) {
+ success &= render_page(outputDir, inputFilename, *renderer,
+ FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1 : pn);
+ }
+ } else if (strcmp(FLAGS_pages[0], "first") == 0) {
+ success &= render_page(outputDir, inputFilename, *renderer,
+ FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1 : 0);
+ } else if (strcmp(FLAGS_pages[0], "last") == 0) {
+ success &= render_page(outputDir, inputFilename, *renderer,
+ FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1
+ : renderer->pages() - 1);
+ } else {
+ int pn = atoi(FLAGS_pages[0]);
+ success &= render_page(outputDir, inputFilename, *renderer,
+ FLAGS_noExtensionForOnePagePdf && renderer->pages() == 1 ? -1 : pn);
}
}
@@ -303,23 +289,21 @@
* parse_pdf.
* @param input A directory or an pdf file.
* @param outputDir Output dir.
- * @param renderer The object responsible to render the skp object into pdf.
*/
-static int process_input(const char* input, const SkString& outputDir,
- SkPdfRenderer& renderer) {
+static int process_input(const char* input, const SkString& outputDir) {
int failures = 0;
if (sk_isdir(input)) {
SkOSFile::Iter iter(input, PDF_FILE_EXTENSION);
SkString inputFilename;
while (iter.next(&inputFilename)) {
SkString inputPath = SkOSPath::SkPathJoin(input, inputFilename.c_str());
- if (!process_pdf(inputPath, outputDir, renderer)) {
+ if (!process_pdf(inputPath, outputDir)) {
++failures;
}
}
} else {
SkString inputPath(input);
- if (!process_pdf(inputPath, outputDir, renderer)) {
+ if (!process_pdf(inputPath, outputDir)) {
++failures;
}
}
@@ -336,8 +320,6 @@
exit(-1);
}
- SkPdfRenderer renderer;
-
SkString outputDir;
if (FLAGS_writePath.count() == 1) {
outputDir.set(FLAGS_writePath[0]);
@@ -345,8 +327,7 @@
int failures = 0;
for (int i = 0; i < FLAGS_readPath.count(); i ++) {
- failures += process_input(FLAGS_readPath[i], outputDir, renderer);
- renderer.unload();
+ failures += process_input(FLAGS_readPath[i], outputDir);
}
reportPdfRenderStats();
diff --git a/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.cpp b/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.cpp
index 1d8f510..d3fea71 100644
--- a/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.cpp
+++ b/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.cpp
@@ -126,11 +126,11 @@
bool storeCatalog = true;
while (xrefByteOffset >= 0) {
- const unsigned char* trailerStart = readCrossReferenceSection(fFileContent + xrefByteOffset,
- xrefstartKeywordLine);
+ const unsigned char* trailerStart = this->readCrossReferenceSection(fFileContent + xrefByteOffset,
+ xrefstartKeywordLine);
xrefByteOffset = -1;
if (trailerStart < xrefstartKeywordLine) {
- readTrailer(trailerStart, xrefstartKeywordLine, storeCatalog, &xrefByteOffset, false);
+ this->readTrailer(trailerStart, xrefstartKeywordLine, storeCatalog, &xrefByteOffset, false);
storeCatalog = false;
}
}
@@ -303,7 +303,7 @@
return current;
}
- addCrossSectionInfo(startId + i, generation, offset, *token.c_str() == 'f');
+ this->addCrossSectionInfo(startId + i, generation, offset, *token.c_str() == 'f');
}
}
SkPdfReport(kInfo_SkPdfIssueSeverity, kNoIssue_SkPdfIssue,
@@ -363,7 +363,7 @@
void SkPdfNativeDoc::addCrossSectionInfo(int id, int generation, int offset, bool isFreed) {
// TODO(edisonn): security here, verify id
while (fObjects.count() < id + 1) {
- reset(fObjects.append());
+ this->reset(fObjects.append());
}
fObjects[id].fOffset = offset;
diff --git a/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.h b/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.h
index d824137..73265ca 100644
--- a/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.h
+++ b/experimental/PdfViewer/pdfparser/native/SkPdfNativeDoc.h
@@ -68,6 +68,7 @@
SkPdfNativeDoc(const char* path);
// TODO(edisonn) should be deprecated
+ // FIXME: Untested.
SkPdfNativeDoc(SkStream* stream);
~SkPdfNativeDoc();
diff --git a/experimental/PdfViewer/SkPdfRenderer.cpp b/experimental/PdfViewer/src/SkPdfRenderer.cpp
similarity index 98%
rename from experimental/PdfViewer/SkPdfRenderer.cpp
rename to experimental/PdfViewer/src/SkPdfRenderer.cpp
index 2d79f13..25fc41d 100644
--- a/experimental/PdfViewer/SkPdfRenderer.cpp
+++ b/experimental/PdfViewer/src/SkPdfRenderer.cpp
@@ -3087,73 +3087,73 @@
return true;
}
-bool SkPdfRenderer::load(const SkString inputFileName) {
- unload();
-
- fPdfDoc = new SkPdfNativeDoc(inputFileName.c_str());
- if (fPdfDoc->pages() == 0) {
- delete fPdfDoc;
- fPdfDoc = NULL;
+SkPdfRenderer* SkPdfRenderer::CreateFromFile(const char* inputFileName) {
+ // FIXME: SkPdfNativeDoc should have a similar Create function.
+ SkPdfNativeDoc* pdfDoc = SkNEW_ARGS(SkPdfNativeDoc, (inputFileName));
+ if (pdfDoc->pages() == 0) {
+ SkDELETE(pdfDoc);
+ return NULL;
}
- return fPdfDoc != NULL;
+ return SkNEW_ARGS(SkPdfRenderer, (pdfDoc));
}
-bool SkPdfRenderer::load(SkStream* stream) {
- unload();
-
+SkPdfRenderer* SkPdfRenderer::CreateFromStream(SkStream* stream) {
// TODO(edisonn): create static function that could return NULL if there are errors
- fPdfDoc = new SkPdfNativeDoc(stream);
- if (fPdfDoc->pages() == 0) {
- delete fPdfDoc;
- fPdfDoc = NULL;
+ SkPdfNativeDoc* pdfDoc = SkNEW_ARGS(SkPdfNativeDoc, (stream));
+ if (pdfDoc->pages() == 0) {
+ SkDELETE(pdfDoc);
+ return NULL;
}
- return fPdfDoc != NULL;
+ return SkNEW_ARGS(SkPdfRenderer, (pdfDoc));
}
+SkPdfRenderer::SkPdfRenderer(SkPdfNativeDoc* doc)
+ :fPdfDoc(doc) {
+}
+
+SkPdfRenderer::~SkPdfRenderer() {
+ SkDELETE(fPdfDoc);
+}
int SkPdfRenderer::pages() const {
- return fPdfDoc != NULL ? fPdfDoc->pages() : 0;
-}
-
-void SkPdfRenderer::unload() {
- delete fPdfDoc;
- fPdfDoc = NULL;
+ SkASSERT(fPdfDoc != NULL);
+ return fPdfDoc->pages();
}
SkRect SkPdfRenderer::MediaBox(int page) const {
- SkASSERT(fPdfDoc);
+ SkASSERT(fPdfDoc != NULL);
return fPdfDoc->MediaBox(page);
}
size_t SkPdfRenderer::bytesUsed() const {
- return fPdfDoc ? fPdfDoc->bytesUsed() : 0;
+ SkASSERT(fPdfDoc != NULL);
+ return fPdfDoc->bytesUsed();
}
bool SkPDFNativeRenderToBitmap(SkStream* stream,
SkBitmap* output,
int page,
- SkPdfContent content,
+ SkPdfContent unused,
double dpi) {
SkASSERT(page >= 0);
- SkPdfRenderer renderer;
- renderer.load(stream);
- if (!renderer.loaded() || page >= renderer.pages() || page < 0) {
+ SkPdfRenderer* renderer = SkPdfRenderer::CreateFromStream(stream);
+ if (NULL == renderer) {
return false;
}
- SkRect rect = renderer.MediaBox(page < 0 ? 0 :page);
+ SkRect rect = renderer->MediaBox(page < 0 ? 0 :page);
- SkScalar width = SkScalarMul(rect.width(), SkDoubleToScalar(sqrt(dpi / 72.0)));
- SkScalar height = SkScalarMul(rect.height(), SkDoubleToScalar(sqrt(dpi / 72.0)));
+ SkScalar width = SkScalarMul(rect.width(), SkDoubleToScalar(dpi / 72.0));
+ SkScalar height = SkScalarMul(rect.height(), SkDoubleToScalar(dpi / 72.0));
rect = SkRect::MakeWH(width, height);
- setup_bitmap(output, (int)SkScalarToDouble(width), (int)SkScalarToDouble(height));
+ setup_bitmap(output, SkScalarCeilToInt(width), SkScalarCeilToInt(height));
SkAutoTUnref<SkBaseDevice> device(SkNEW_ARGS(SkBitmapDevice, (*output)));
SkCanvas canvas(device);
- return renderer.renderPage(page, &canvas, rect);
+ return renderer->renderPage(page, &canvas, rect);
}
diff --git a/gyp/SampleApp.gyp b/gyp/SampleApp.gyp
index 3a07702..2708036 100644
--- a/gyp/SampleApp.gyp
+++ b/gyp/SampleApp.gyp
@@ -1,3 +1,4 @@
+#
{
'variables': {
#manually set sample_pdf_file_viewer to 1 to have the PdfViewer in SampleApp
@@ -167,7 +168,7 @@
'pdfviewer_lib.gyp:pdfviewer_lib',
],
'include_dirs' : [
- '../experimental/PdfViewer/',
+ '../experimental/PdfViewer/inc',
],
'sources': [
'../samplecode/SamplePdfFileViewer.cpp',
diff --git a/gyp/gm.gyp b/gyp/gm.gyp
index 504b11e..90c09a4 100644
--- a/gyp/gm.gyp
+++ b/gyp/gm.gyp
@@ -65,7 +65,7 @@
'SK_BUILD_NATIVE_PDF_RENDERER',
],
'include_dirs' : [
- '../experimental/PdfViewer',
+ '../experimental/PdfViewer/inc',
],
'dependencies': [
'pdfviewer_lib.gyp:pdfviewer_lib',
diff --git a/gyp/pdfviewer.gyp b/gyp/pdfviewer.gyp
index 1f7877f..7635877 100644
--- a/gyp/pdfviewer.gyp
+++ b/gyp/pdfviewer.gyp
@@ -16,6 +16,7 @@
],
'include_dirs': [
'../experimental/PdfViewer',
+ '../experimental/PdfViewer/inc',
'../experimental/PdfViewer/pdfparser',
'../experimental/PdfViewer/pdfparser/native',
],
diff --git a/gyp/pdfviewer_lib.gyp b/gyp/pdfviewer_lib.gyp
index f9acdd6..e25e024 100644
--- a/gyp/pdfviewer_lib.gyp
+++ b/gyp/pdfviewer_lib.gyp
@@ -9,9 +9,13 @@
'target_name': 'pdfviewer_lib',
'type': 'static_library',
'sources': [
+ # FIXME: Include directory is named "inc" (instead of "include") in
+ # order to not be considered the public API.
+ '../experimental/PdfViewer/inc/SkPdfRenderer.h',
+ '../experimental/PdfViewer/src/SkPdfRenderer.cpp',
+
'../experimental/PdfViewer/SkPdfGraphicsState.cpp',
'../experimental/PdfViewer/SkPdfFont.cpp',
- '../experimental/PdfViewer/SkPdfRenderer.cpp',
'../experimental/PdfViewer/SkPdfReporter.cpp',
'../experimental/PdfViewer/SkPdfUtils.cpp',
#'../experimental/PdfViewer/SkPdfNYI.cpp',
@@ -25,6 +29,7 @@
],
'include_dirs': [
'../experimental/PdfViewer',
+ '../experimental/PdfViewer/inc',
'../experimental/PdfViewer/pdfparser',
'../experimental/PdfViewer/pdfparser/native',
'../experimental/PdfViewer/pdfparser/native/pdfapi',
diff --git a/samplecode/SamplePdfFileViewer.cpp b/samplecode/SamplePdfFileViewer.cpp
index fc8b0f0..55c7002 100644
--- a/samplecode/SamplePdfFileViewer.cpp
+++ b/samplecode/SamplePdfFileViewer.cpp
@@ -37,18 +37,16 @@
SkPicture* fPicture; // TODO(edisonn): multiple pages, one page / picture, make it an array
static SkPicture* LoadPdf(const char path[]) {
- SkPicture* pic = NULL;
-
- SkPdfRenderer renderer;
- SkString skpath;
- skpath.append(path);
- renderer.load(skpath);
- if (renderer.loaded()) {
- pic = SkNEW(SkPicture);
- SkCanvas* canvas = pic->beginRecording((int)renderer.MediaBox(0).width(), (int)renderer.MediaBox(0).height());
- renderer.renderPage(0, canvas, renderer.MediaBox(0));
- pic->endRecording();
+ SkAutoTDelete<SkPdfRenderer> renderer(SkPdfRenderer::CreateFromFile(path));
+ if (NULL == renderer.get()) {
+ return NULL;
}
+
+ SkPicture* pic = SkNEW(SkPicture);
+ SkCanvas* canvas = pic->beginRecording((int) renderer->MediaBox(0).width(),
+ (int) renderer->MediaBox(0).height());
+ renderer->renderPage(0, canvas, renderer->MediaBox(0));
+ pic->endRecording();
return pic;
}