Adding PageViewport to exports

[soadzoor]

Right now I use a very dirty hack to be able to create an instance of a PageViewport, this PR solves this issue.

[Snuffleupagus]

Right now I use a very dirty hack to be able to create an instance of a PageViewport, this PR solves this issue.

Why do you need to manually initialize a PageViewport though, since the correct way of obtaining such a thing is by calling PDFPageProxy.getViewport; see

pdf.js/src/display/api.js

Lines 956 to 973 in 16ae7c6

	/**
	* @param {GetViewportParameters} params - Viewport parameters.
	* @returns {PageViewport} Contains 'width' and 'height' properties
	* along with transforms required for rendering.
	*/
	getViewport({ scale, rotation = this.rotate, dontFlip = false, } = {}) {
	if ((typeof PDFJSDev !== 'undefined' && PDFJSDev.test('GENERIC')) &&
	(arguments.length > 1 || typeof arguments[0] === 'number')) {
	throw new Error(
	'PDFPageProxy.getViewport is called with obsolete arguments.');
	}
	return new PageViewport({
	viewBox: this.view,
	scale,
	rotation,
	dontFlip,
	});
	}

Most likely, you'll need to provide a good explanation/example of why this change is necessary in order for the PR to be considered.

[soadzoor]

Right now I use a very dirty hack to be able to create an instance of a PageViewport, this PR solves this issue.

Why do you need to manually initialize a PageViewport though, since the correct way of obtaining such a thing is by calling PDFPageProxy.getViewport; see

pdf.js/src/display/api.js

Lines 956 to 973 in 16ae7c6

	/**
	* @param {GetViewportParameters} params - Viewport parameters.
	* @returns {PageViewport} Contains 'width' and 'height' properties
	* along with transforms required for rendering.
	*/
	getViewport({ scale, rotation = this.rotate, dontFlip = false, } = {}) {
	if ((typeof PDFJSDev !== 'undefined' && PDFJSDev.test('GENERIC')) &&
	(arguments.length > 1 || typeof arguments[0] === 'number')) {
	throw new Error(
	'PDFPageProxy.getViewport is called with obsolete arguments.');
	}
	return new PageViewport({
	viewBox: this.view,
	scale,
	rotation,
	dontFlip,
	});
	}

Most likely, you'll need to provide a good explanation/example of why this change is necessary in order for the PR to be considered.

The point is: I'm not trying to "obtain" a viewport, I'm trying to create a new one.

What's the purpose? - You might ask.

I work on a project where it's essential to get only a portion of a pdf rendered. I determine that portion with a newly created PageViewport.

Any other questions/suggestions are welcome.

[Snuffleupagus]

I work on a project where it's essential to get only a portion of a pdf rendered. I determine that portion with a newly created PageViewport.

So you're not calling getViewport at all, not even initially? Because if so, you can use the clone method on the viewport that that method returns; see

pdf.js/src/display/display_utils.js

Lines 252 to 267 in 16ae7c6

	/**
	* Clones viewport, with optional additional properties.
	* @param {PageViewportCloneParameters} [params]
	* @returns {PageViewport} Cloned viewport.
	*/
	clone({ scale = this.scale, rotation = this.rotation,
	dontFlip = false, } = {}) {
	return new PageViewport({
	viewBox: this.viewBox.slice(),
	scale,
	rotation,
	offsetX: this.offsetX,
	offsetY: this.offsetY,
	dontFlip,
	});
	}

[soadzoor]

To me that sounds like a workaround, rather than a solution. Also, getters usually don't need parameters, neither "clone" functions. With all due respect, these are kind of counterintuitive to me.

If you had a "Circle" class, and I need to create a "new Circle", I'd rather not do it like this:
Object.getCircle({radius: 3}).clone({radius: 3}). It's overcomplicated and counterproductive. Doesn't it make more sense to create it like this: new Circle({radius: 3})?

[Snuffleupagus]

Also, getters usually don't need parameters,

I'm slightly confused here, since there shouldn't be any getters involved here...

My general point though is that you want to avoid extending the API surface, especially with code which is completely unused/untested in the general library, if the relevant functionality is already being exposed elsewhere.

[soadzoor]

Also, getters usually don't need parameters,

I'm slightly confused here, since there shouldn't be any getters involved here...

My general point though is that you want to avoid extending the API surface, especially with code which is completely unused/untested in the general library, if the relevant functionality is already being exposed elsewhere.

Maybe it's my poor choice of words, by getter I meant this: getViewport.

I doubt it's unused/untested, because before my current approach (using npm), I tried this lib using the script src method (about a year ago), and with that I was able to use that class (PageViewport) without any issue. It was part of a global var called PDFJS. So either it was removed from the public classes, or it was never part of the "npm" version..?

[timvandermeij]

The global PDFJS variable got removed in version 2.0 to keep PDF.js more self-contained (less global namespace usage) and to expose an API without any internals.

The point is: I'm not trying to "obtain" a viewport, I'm trying to create a new one

The code in #11239 (comment) creates a new ViewPort object, so does that not suffice because the viewBox cannot be tweaked?

Given /~https://github.com/mozilla/pdf.js/blob/master/src/display/api.js#L867 and your specific use-case I'm not against adding it to the exports. However, alternatively we could consider updating getViewport to, optionally, give another viewBox to it. In that case we don't have to expose ViewPort in the API and still give the flexibility to get a different viewport. getViewport is a method of the page object, so I would see it as getting the full view on the page, or, when a different viewBox is provided, a part of it. We can also add a test then, just like the optional background parameter. Would that perhaps be a more desired situation?

[soadzoor]

The code in #11239 (comment) creates a new ViewPort object, so does that not suffice because the viewBox cannot be tweaked?

In theory it should work, but as I mentioned, it looks hacky (I'm not sure if my "Circle" metaphor above makes any sense to you).

BTW I tried with that method, and it doesn't work for me, gives me a blank (white) image, so in practice, it doesn't work. Is it possible that page.getViewport or clone modifies the viewport of the page?

Anyway, I don't see any reason why we shouldn't expose this class. It was public in the recent past, some of us need it, so why not expose it again?

[soadzoor]

Any news on this?

[Snuffleupagus]

It would be most helpful if you could provide a link to your code (or a reduced test-case), such that it's possible to see how this is actually being used, since it's quite difficult to understand the exact requirements otherwise and the discussion thus becomes a bit theoretical.

So either it was removed from the public classes, or it was never part of the "npm" version..?

I don't believe that PageViewport was ever intentionally exported publicly, since it's mostly internal functionality, but it was rather an artefact of the old (and now removed) global PDFJS object and of the old build system.

I doubt it's unused/untested, [...]

My point was that the change proposed in this PR adds code which is unused/untested (from the perspective of the library), and thus cannot be guaranteed to remain available.

[soadzoor]

@Snuffleupagus thanks, I'll prepare a demo in the following days of my use-case. Is there any jsfiddle template available with this lib?

[Snuffleupagus]

Is there any jsfiddle template available with this lib?

Possibly the "Edit in JSFiddle" links found at https://mozilla.github.io/pdf.js/examples/#interactive-examples could be used as a starting point.

[soadzoor]

@Snuffleupagus
Ok, so I tried putting together a smaller demo, but I realized that I can't access the PageViewport class within the jsfiddle project, so I can't do it 😢 (Funny, cause this is what this whole PR is about.. )

So instead of that, I'm going to link my old demo, which uses the old library (where this class was still available): http://vargapeter.info/works/pdf-rasterizer/

The point of this is that it splits the PDF into multiple canvases (like a tile-system), so they can be used as webgl textures. The relevant part is at line 100 in js/app.js: var viewport = new PDFJS.PageViewport(_this._pdfViewBox, _this._scale, _this._page.rotate, -xOffset, -yOffset);

So basically what I need is the ability to pass a custom viewport (with offset, rotation, scale, etc) to the renderContext, before rasterizing the pdf.

Exposing the pageviewport class makes this super easy, while not-exposing it makes it impossible (as far as I know).

[Snuffleupagus]

PDFJS.PageViewport(_this._pdfViewBox, _this._scale, _this._page.rotate, -xOffset, -yOffset);

Looking very briefly at the code, it appears that _this._pdfViewBox is simply the result of calling

pdf.js/src/display/api.js

Lines 948 to 954 in d7f651a

	/**
	* @type {Array} An array of the visible portion of the PDF page in user
	* space units [x1, y1, x2, y2].
	*/
	get view() {
	return this._pageInfo.view;
	}

Assuming that's actually correct, then it looks like what you really need here is the ability to pass custom offsets to PDFPageProxy.getViewport and/or PageViewport.clone which shouldn't be difficult to support (in a way that's tested and thus more future-proof).

[soadzoor]

@Snuffleupagus
Yea, that's correct.
The method you describe sounds a little bit more difficult and less elegant to me, but I don't really care as long as it works. :)

[Snuffleupagus]

Please see PR #11273.

The method you describe sounds a little bit more difficult and less elegant to me,

Sorry, but I'm not sure that I really understand how using an existing, well-established, documented, and unit-tested API method, i.e. PDFPageProxy.getViewport with additional parameters, would be either difficult or ugly.

The point is that by simply extending a pre-existing API method to accept new options, it's easy to ensure that the functionality remains available and, thanks to the unit-tests, working even with any future code changes. (There's no guarantees of either with the approach suggest in this PR, unfortunately.)

Generally speaking: Please note that the more functionality you're exposing as part of the public API, the more difficult maintaining the code becomes. Basically, as soon as something is exposed publicly then fixing bugs, adding features, and/or re-factoring code all has the potential to be difficult (which experience has shown). Hence the reason for not wanting to unnecessarily export things unless absolutely necessary.

[soadzoor]

@Snuffleupagus
Sorry, I meant no offense, but since you asked, I'm going to express my opinion:

What I meant by "difficult" is that the exposing can be done in 1 single line (what this PR does), whereas the other solution surely needs more work. What I meant by "less elegant" is the way it's going to be used. IMHO new PageViewport makes more sense when I want to create a "new PageViewport", than getViewport(params). As I mentionted above, "get"Something functions (in other libs / code in general) usually don't require any parameters, and they don't create anything new, they simply provide you something. Again: Usually. Thus it makes it a little bit unintiuitive to me.

I don't know much about the inner parts of this lib (unit tests / whatever), so if you're saying that you're proposed PR is safer / better / etc, I'm going to believe you, no questions asked.

Again: As long as it works, I'm fine with it, and thank you for your time and effort for making this possible.

So, as far as I understand, if someone wants to have a custom pageviewport, instead of this:

var viewport = new pdfjs.PageViewport({
	viewBox: pageViewBox,
	scale: scale,
	rotation: rotate,
	offsetX: offsetX,
	offsetY: offsetY,
	dontFlip: false
});

They need to use it like this:

var viewport = page.getViewport({
	viewBox: pageViewBox,
	scale: scale,
	rotation: rotate,
	offsetX: offsetX,
	offsetY: offsetY,
	dontFlip: false
});

Is that right?

[timvandermeij]

Yes. Thanks to the pull request above, you can then pass offsetX and offsetY parameters to create a custom viewport.