Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SVG] Adds clip operator, fixes setGState, fill, stroke operators #5034

Merged
merged 5 commits into from
Jul 25, 2014

Conversation

pramodhkp
Copy link
Contributor

This PR:

@pramodhkp
Copy link
Contributor Author

cc @timvandermeij

@timvandermeij
Copy link
Contributor

@pramodhkp I have added some review comments. Could you link one or two specific PDFs that are affected (i.e. render better) with this change?

@pramodhkp
Copy link
Contributor Author

Ah, yeah. I meant to do that. I will add it after I make the changes. Thanks for the review.

@pramodhkp
Copy link
Contributor Author

@timvandermeij I have made the changes. I also separated the changes in fillStroke method into a separate commit. I have updated the reference pdfs in the PR comment.

@timvandermeij
Copy link
Contributor

@pramodhkp I can confirm that /test/pdfs/txt2pdf.pdf renders much better (the text color is correct now and the spheres are filled correctly). test/pdfs/extgstate.pdf seems to render worse for me compared to Acrobat and the version prior to this PR (i.e. the line is now two dots and the text is way too large). I could not verify the 'clip' operator change, because Tracemonkey shows no difference to me (I assume that's what you meant by 'default doc'; if you meant the document loaded by default in the SVG viewer example, than I cannot verify it because the text still renders crazily for me because of that Bugzilla bug).

I have also reviewed the code again and added two small comments. Overall, this is starting to look really good.

Below a screenshot of how extgstate.pdf renders before and after applying this PR. You can clearly see that the rendering became worse. Any idea what's causing that?

naamloos

@pramodhkp
Copy link
Contributor Author

@timvandermeij I am not sure what's wrong with the rendering of /test/pdfs/extgstate.pdf . I could not try it in adobe reader. But, I did try in Okular (KDE) and PDF.js (canvas), and it seemed to render similarly. The text font is courier and the sizes are also proper, I think.

As for the clippping, by default PDF, I meant the 'default' in SVG. (liveprogramming.pdf). Anyway, I think it's not a good example. I tried it on the PDF 32000 document, and I think the clip is still not working properly. For reference, it's Page No. 240 in that document.

@timvandermeij
Copy link
Contributor

This is how Adobe Acrobat (and Adobe Reader for that matter) renders it:

naamloos-1

So we have two different renderings for this file now: Adobe's rendering and PDF.js's canvas/your current PR rendering. Since your SVG rendering is now equal to PDF.js's rendering, I wouldn't worry about this file too much. PDF.js currently also renders it wrong compared to Adobe.

@@ -307,6 +313,12 @@ var SVGGraphics = (function SVGGraphicsClosure(ctx) {
case OPS.fillStroke:
this.fillStroke();
break;
case OPS.clip:
this.clip();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: try to not use method call with different amount of parameters, e.g. make it this.clip('nonzero') and if needed use e.g. this.applyClip('nonzero')

@pramodhkp pramodhkp changed the title [SVG] Adds clip operator, fixes setGState operator [SVG] Adds clip operator, fixes setGState, fill, stroke operators Jul 25, 2014
@yurydelendik
Copy link
Contributor

Really good, thank you.

yurydelendik added a commit that referenced this pull request Jul 25, 2014
[SVG] Adds clip operator, fixes setGState, fill, stroke operators
@yurydelendik yurydelendik merged commit 5d4eebc into mozilla:master Jul 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants