GitHub PullRequestEvent at 2014-03-29 21:41:24
Repository: pdf.js
Actor: Snuffleupagus
Payload: {"action":"opened","number":4533,"pull_request":{"url":"https://api.github.com/repos/mozilla/pdf.js/pulls/4533","id":14146638,"html_url":"https://github.com/mozilla/pdf.js/pull/4533","diff_url":"https://github.com/mozilla/pdf.js/pull/4533.diff","patch_url":"https://github.com/mozilla/pdf.js/pull/4533.patch","issue_url":"https://api.github.com/repos/mozilla/pdf.js/issues/4533","number":4533,"state":"open","title":"[WIP] Enforce minimum line widths and rectangle sizes","user":{"login":"Snuffleupagus","id":2692120,"avatar_url":"https://avatars.githubusercontent.com/u/2692120?","gravatar_id":"e05382aa67ec592c3eb32e9cd9edd65e","url":"https://api.github.com/users/Snuffleupagus","html_url":"https://github.com/Snuffleupagus","followers_url":"https://api.github.com/users/Snuffleupagus/followers","following_url":"https://api.github.com/users/Snuffleupagus/following{/other_user}","gists_url":"https://api.github.com/users/Snuffleupagus/gists{/gist_id}","starred_url":"https://api.github.com/users/Snuffleupagus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Snuffleupagus/subscriptions","organizations_url":"https://api.github.com/users/Snuffleupagus/orgs","repos_url":"https://api.github.com/users/Snuffleupagus/repos","events_url":"https://api.github.com/users/Snuffleupagus/events{/privacy}","received_events_url":"https://api.github.com/users/Snuffleupagus/received_events","type":"User","site_admin":false},"body":"This PR contains experimental patches that enforce lower bounds on both line widths and rectangle sizes. I don't think this should be merged as is, but I wanted to submit the PR to get feedback. My apologies for the very long post!\r\n\r\nGiven that PDF.js currently renders some files with lines that are way to thin, in some cases becoming almost invisible, I think that we should replace too narrow line widths with more reasonable ones.\r\nComparing PDF.js with Adobe Reader, it seems that they enforce minimum line widths. This would explain why there are a number of bugs filed about missing lines PDF.js. (See below for an incomplete list.)\r\n\r\nThe obvious candidate to use when establishing a minimum line width should be [`getSinglePixelWidth`](https://github.com/mozilla/pdf.js/blob/master/src/display/canvas.js#L2191). Unfortunately, in my testing, using that value causes many lines to become *way* too thick.\r\nThrough some trial and error, I found that reducing the value returned by `getSinglePixelWidth` to `0.75 * getSinglePixelWidth` seemed to yield *much* better results. Obviously, this seems like a ugly hack, but I don't really understand why `getSinglePixelWidth` isn't returning more reasonable values.\r\nIt then occurred to me that `0.75 = 1 / CSS_UNITS`, where `CSS_UNITS = 96.0 /72.0` (see [viewer.js#L31](https://github.com/mozilla/pdf.js/blob/master/web/viewer.js#L31)). Is this just a coincidence, or could it explain the issues I had with the values returned by `getSinglePixelWidth`?\r\n\r\nAs this PR currently stands, it would fix issues like: #4314, #3316 and #2722, and it would also prevent any future issues with lines getting drawn too thin.\r\n\r\nPlease note that if we, as suggested in this PR, start to enforce minimum line widths (and rectangle sizes) this obviously means that a fair number of the test PDFs would change. These changes would fall in two general categories:\r\n* Changes that are so small that they are virtually indistinguishable to the naked eye.\r\n\r\n* Changes that are easily noticeable. In most of these cases, the slightly thicker lines looks a lot better (to me, at least) and the rendering in PDF.js is now very close to the one in Adobe Reader.\r\nIn a number of the test files, it's also possible to see details (in figures) that currently are almost invisible.","created_at":"2014-03-29T21:41:22Z","updated_at":"2014-03-29T21:41:22Z","closed_at":null,"merged_at":null,"merge_commit_sha":"58c5c24146c3c5751125f22687c612e4129ecb42","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/mozilla/pdf.js/pulls/4533/commits","review_comments_url":"https://api.github.com/repos/mozilla/pdf.js/pulls/4533/comments","review_comment_url":"https://api.github.com/repos/mozilla/pdf.js/pulls/comments/{number}","comments_url":"https://api.github.com/repos/mozilla/pdf.js/issues/4533/comments","statuses_url":"https://api.github.com/repos/mozilla/pdf.js/statuses/409f1db93b97fd9a645e95206ff824c51a03fe1b","head":{"label":"Snuffleupagus:canvas-enforce-min-sizes","ref":"canvas-enforce-min-sizes","sha":"409f1db93b97fd9a645e95206ff824c51a03fe1b","user":{"login":"Snuffleupagus","id":2692120,"avatar_url":"https://avatars.githubusercontent.com/u/2692120?","gravatar_id":"e05382aa67ec592c3eb32e9cd9edd65e","url":"https://api.github.com/users/Snuffleupagus","html_url":"https://github.com/Snuffleupagus","followers_url":"https://api.github.com/users/Snuffleupagus/followers","following_url":"https://api.github.com/users/Snuffleupagus/following{/other_user}","gists_url":"https://api.github.com/users/Snuffleupagus/gists{/gist_id}","starred_url":"https://api.github.com/users/Snuffleupagus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Snuffleupagus/subscriptions","organizations_url":"https://api.github.com/users/Snuffleupagus/orgs","repos_url":"https://api.github.com/users/Snuffleupagus/repos","events_url":"https://api.github.com/users/Snuffleupagus/events{/privacy}","received_events_url":"https://api.github.com/users/Snuffleupagus/received_events","type":"User","site_admin":false},"repo":{"id":7120249,"name":"pdf.js","full_name":"Snuffleupagus/pdf.js","owner":{"login":"Snuffleupagus","id":2692120,"avatar_url":"https://avatars.githubusercontent.com/u/2692120?","gravatar_id":"e05382aa67ec592c3eb32e9cd9edd65e","url":"https://api.github.com/users/Snuffleupagus","html_url":"https://github.com/Snuffleupagus","followers_url":"https://api.github.com/users/Snuffleupagus/followers","following_url":"https://api.github.com/users/Snuffleupagus/following{/other_user}","gists_url":"https://api.github.com/users/Snuffleupagus/gists{/gist_id}","starred_url":"https://api.github.com/users/Snuffleupagus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Snuffleupagus/subscriptions","organizations_url":"https://api.github.com/users/Snuffleupagus/orgs","repos_url":"https://api.github.com/users/Snuffleupagus/repos","events_url":"https://api.github.com/users/Snuffleupagus/events{/privacy}","received_events_url":"https://api.github.com/users/Snuffleupagus/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/Snuffleupagus/pdf.js","description":"PDF Reader in JavaScript","fork":true,"url":"https://api.github.com/repos/Snuffleupagus/pdf.js","forks_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/forks","keys_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/teams","hooks_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/hooks","issue_events_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/issues/events{/number}","events_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/events","assignees_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/assignees{/user}","branches_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/branches{/branch}","tags_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/tags","blobs_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/git/refs{/sha}","trees_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/statuses/{sha}","languages_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/languages","stargazers_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/stargazers","contributors_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/contributors","subscribers_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/subscribers","subscription_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/subscription","commits_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/commits{/sha}","git_commits_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/git/commits{/sha}","comments_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/comments{/number}","issue_comment_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/issues/comments/{number}","contents_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/contents/{+path}","compare_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/merges","archive_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/downloads","issues_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/issues{/number}","pulls_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/pulls{/number}","milestones_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/milestones{/number}","notifications_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/labels{/name}","releases_url":"https://api.github.com/repos/Snuffleupagus/pdf.js/releases{/id}","created_at":"2012-12-11T22:06:46Z","updated_at":"2014-03-29T21:41:22Z","pushed_at":"2014-03-29T16:49:16Z","git_url":"git://github.com/Snuffleupagus/pdf.js.git","ssh_url":"git@github.com:Snuffleupagus/pdf.js.git","clone_url":"https://github.com/Snuffleupagus/pdf.js.git","svn_url":"https://github.com/Snuffleupagus/pdf.js","homepage":"","size":41028,"stargazers_count":1,"watchers_count":1,"language":"JavaScript","has_issues":false,"has_downloads":true,"has_wiki":true,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":1,"default_branch":"master","master_branch":"master"}},"base":{"label":"mozilla:master","ref":"master","sha":"7d5bf83ce879a2a937ff1a5cf6c1b65e008beaa5","user":{"login":"mozilla","id":131524,"avatar_url":"https://avatars.githubusercontent.com/u/131524?","gravatar_id":"9ba739c8f3288e256b13553e4d257b5e","url":"https://api.github.com/users/mozilla","html_url":"https://github.com/mozilla","followers_url":"https://api.github.com/users/mozilla/followers","following_url":"https://api.github.com/users/mozilla/following{/other_user}","gists_url":"https://api.github.com/users/mozilla/gists{/gist_id}","starred_url":"https://api.github.com/users/mozilla/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mozilla/subscriptions","organizations_url":"https://api.github.com/users/mozilla/orgs","repos_url":"https://api.github.com/users/mozilla/repos","events_url":"https://api.github.com/users/mozilla/events{/privacy}","received_events_url":"https://api.github.com/users/mozilla/received_events","type":"Organization","site_admin":false},"repo":{"id":1663468,"name":"pdf.js","full_name":"mozilla/pdf.js","owner":{"login":"mozilla","id":131524,"avatar_url":"https://avatars.githubusercontent.com/u/131524?","gravatar_id":"9ba739c8f3288e256b13553e4d257b5e","url":"https://api.github.com/users/mozilla","html_url":"https://github.com/mozilla","followers_url":"https://api.github.com/users/mozilla/followers","following_url":"https://api.github.com/users/mozilla/following{/other_user}","gists_url":"https://api.github.com/users/mozilla/gists{/gist_id}","starred_url":"https://api.github.com/users/mozilla/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mozilla/subscriptions","organizations_url":"https://api.github.com/users/mozilla/orgs","repos_url":"https://api.github.com/users/mozilla/repos","events_url":"https://api.github.com/users/mozilla/events{/privacy}","received_events_url":"https://api.github.com/users/mozilla/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/mozilla/pdf.js","description":"PDF Reader in JavaScript","fork":false,"url":"https://api.github.com/repos/mozilla/pdf.js","forks_url":"https://api.github.com/repos/mozilla/pdf.js/forks","keys_url":"https://api.github.com/repos/mozilla/pdf.js/keys{/key_id}","collaborators_url":"https://api.github.com/repos/mozilla/pdf.js/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/mozilla/pdf.js/teams","hooks_url":"https://api.github.com/repos/mozilla/pdf.js/hooks","issue_events_url":"https://api.github.com/repos/mozilla/pdf.js/issues/events{/number}","events_url":"https://api.github.com/repos/mozilla/pdf.js/events","assignees_url":"https://api.github.com/repos/mozilla/pdf.js/assignees{/user}","branches_url":"https://api.github.com/repos/mozilla/pdf.js/branches{/branch}","tags_url":"https://api.github.com/repos/mozilla/pdf.js/tags","blobs_url":"https://api.github.com/repos/mozilla/pdf.js/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/mozilla/pdf.js/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/mozilla/pdf.js/git/refs{/sha}","trees_url":"https://api.github.com/repos/mozilla/pdf.js/git/trees{/sha}","statuses_url":"https://api.github.com/repos/mozilla/pdf.js/statuses/{sha}","languages_url":"https://api.github.com/repos/mozilla/pdf.js/languages","stargazers_url":"https://api.github.com/repos/mozilla/pdf.js/stargazers","contributors_url":"https://api.github.com/repos/mozilla/pdf.js/contributors","subscribers_url":"https://api.github.com/repos/mozilla/pdf.js/subscribers","subscription_url":"https://api.github.com/repos/mozilla/pdf.js/subscription","commits_url":"https://api.github.com/repos/mozilla/pdf.js/commits{/sha}","git_commits_url":"https://api.github.com/repos/mozilla/pdf.js/git/commits{/sha}","comments_url":"https://api.github.com/repos/mozilla/pdf.js/comments{/number}","issue_comment_url":"https://api.github.com/repos/mozilla/pdf.js/issues/comments/{number}","contents_url":"https://api.github.com/repos/mozilla/pdf.js/contents/{+path}","compare_url":"https://api.github.com/repos/mozilla/pdf.js/compare/{base}...{head}","merges_url":"https://api.github.com/repos/mozilla/pdf.js/merges","archive_url":"https://api.github.com/repos/mozilla/pdf.js/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/mozilla/pdf.js/downloads","issues_url":"https://api.github.com/repos/mozilla/pdf.js/issues{/number}","pulls_url":"https://api.github.com/repos/mozilla/pdf.js/pulls{/number}","milestones_url":"https://api.github.com/repos/mozilla/pdf.js/milestones{/number}","notifications_url":"https://api.github.com/repos/mozilla/pdf.js/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/mozilla/pdf.js/labels{/name}","releases_url":"https://api.github.com/repos/mozilla/pdf.js/releases{/id}","created_at":"2011-04-26T06:32:03Z","updated_at":"2014-03-27T23:05:54Z","pushed_at":"2014-03-27T23:05:53Z","git_url":"git://github.com/mozilla/pdf.js.git","ssh_url":"git@github.com:mozilla/pdf.js.git","clone_url":"https://github.com/mozilla/pdf.js.git","svn_url":"https://github.com/mozilla/pdf.js","homepage":"","size":100407,"stargazers_count":8024,"watchers_count":8024,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":1660,"mirror_url":null,"open_issues_count":380,"forks":1660,"open_issues":380,"watchers":8024,"default_branch":"master","master_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/mozilla/pdf.js/pulls/4533"},"html":{"href":"https://github.com/mozilla/pdf.js/pull/4533"},"issue":{"href":"https://api.github.com/repos/mozilla/pdf.js/issues/4533"},"comments":{"href":"https://api.github.com/repos/mozilla/pdf.js/issues/4533/comments"},"review_comments":{"href":"https://api.github.com/repos/mozilla/pdf.js/pulls/4533/comments"},"review_comment":{"href":"https://api.github.com/repos/mozilla/pdf.js/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/mozilla/pdf.js/pulls/4533/commits"},"statuses":{"href":"https://api.github.com/repos/mozilla/pdf.js/statuses/409f1db93b97fd9a645e95206ff824c51a03fe1b"}},"merged":false,"mergeable":true,"mergeable_state":"clean","merged_by":null,"comments":0,"review_comments":0,"commits":2,"additions":24,"deletions":3,"changed_files":1}}