Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289293005:
Use /RECT or /QuadPoints for annotation coordinates, depending on /AP (Closed)
DescriptionUse either /RECT or /QuadPoints for annotation coordinates, depending on /AP
On Acrobat, if "/AP" is present on a text markup definition,
the coordinates used to draw the annotation come from "/Rect
values, whereas if "/AP" is not defined, the array defined
in /QuadPoints is used to grab the annotation coordinates from.
PDFium, on the other hand, uses "/Rect" regardless of
presence or absence of "/AP".
CL fixes PDFium to work similarly to Acrobat, in this case.
TEST=testing/resources/pixel/bug_585_*.in
BUG=pdfium:585
Committed: https://pdfium.googlesource.com/pdfium/+/0a17fafd723e8684d1deb4b5ceea58967a0154da
Patch Set 1 #Patch Set 2 : Use /RECT or /QuadPoints for annotation coordinates, depending on /AP #
Total comments: 12
Patch Set 3 : addressed feedback from tsepez/thestig #Patch Set 4 : Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP #
Total comments: 4
Patch Set 5 : addressed thestig's comments #
Total comments: 11
Patch Set 6 : addressed thestig's and jaepark's comments. #Patch Set 7 : rebased against ToT #Patch Set 8 : rebased + new DEPS #
Messages
Total messages: 44 (16 generated)
Description was changed from ========== Use /RECT or /QuadPoints for annotation coordinates, depending on /AP This CL is a result of a "reverse engineering"-like investigation about why PDFium places highlight annotations differently from Acrobat. On Acrobat, if "/AP" is present on the "/Annot" definition, the coordinates used to draw highlight annotations come from "/Rect values. On the other hand, if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium uses "/Rect" in regardless of presence or absence of "/AP". BUG=pdfium:492 PS: CL lacks testing. ========== to ========== Use /RECT or /QuadPoints for annotation coordinates, depending on /AP This CL is a result of a "reverse engineering"-like investigation about why PDFium places highlight annotations differently from Acrobat. On Acrobat, if "/AP" is present on the "/Annot" definition, the coordinates used to draw highlight annotations come from "/Rect values. On the other hand, if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium uses "/Rect" in regardless of presence or absence of "/AP". BUG=pdfium:492 TEST=https://codereview.chromium.org/2300183002/ ==========
Description was changed from ========== Use /RECT or /QuadPoints for annotation coordinates, depending on /AP This CL is a result of a "reverse engineering"-like investigation about why PDFium places highlight annotations differently from Acrobat. On Acrobat, if "/AP" is present on the "/Annot" definition, the coordinates used to draw highlight annotations come from "/Rect values. On the other hand, if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium uses "/Rect" in regardless of presence or absence of "/AP". BUG=pdfium:492 TEST=https://codereview.chromium.org/2300183002/ ========== to ========== Use /RECT or /QuadPoints for annotation coordinates, depending on /AP This CL is a result of a "reverse engineering"-like investigation about why PDFium places highlight annotations differently from Acrobat. On Acrobat, if "/AP" is present on the "/Annot" definition, the coordinates used to draw highlight annotations come from "/Rect values. On the other hand, if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium uses "/Rect" in regardless of presence or absence of "/AP". BUG=pdfium:585 TEST=https://codereview.chromium.org/2300183002/ ==========
tonikitoo@igalia.com changed reviewers: + dsinclair@chromium.org, jaepark@google.com, thestig@chromium.org, tsepez@chromium.org
PTAL
Description was changed from ========== Use /RECT or /QuadPoints for annotation coordinates, depending on /AP This CL is a result of a "reverse engineering"-like investigation about why PDFium places highlight annotations differently from Acrobat. On Acrobat, if "/AP" is present on the "/Annot" definition, the coordinates used to draw highlight annotations come from "/Rect values. On the other hand, if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium uses "/Rect" in regardless of presence or absence of "/AP". BUG=pdfium:585 TEST=https://codereview.chromium.org/2300183002/ ========== to ========== On Acrobat, if "/AP" is present on the "/Annot" definition, the coordinates used to draw annotations come from "/Rect values, whereas if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium, on the other hand, uses "/Rect" regardless of presence or absence of "/AP". CL fixes PDFium to work similarly to Acrobat, in this case. Additionally, it moves ShouldGenerateAPForAnnotation() from cpvt_generateap.cpp to cpdf_annot.cpp, as the later is a central place to call this function. BUG=pdfium:585 TEST=https://codereview.chromium.org/2300183002/ ==========
Description was changed from ========== On Acrobat, if "/AP" is present on the "/Annot" definition, the coordinates used to draw annotations come from "/Rect values, whereas if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium, on the other hand, uses "/Rect" regardless of presence or absence of "/AP". CL fixes PDFium to work similarly to Acrobat, in this case. Additionally, it moves ShouldGenerateAPForAnnotation() from cpvt_generateap.cpp to cpdf_annot.cpp, as the later is a central place to call this function. BUG=pdfium:585 TEST=https://codereview.chromium.org/2300183002/ ========== to ========== Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP On Acrobat, if "/AP" is present on the "/Annot" definition, the coordinates used to draw annotations come from "/Rect values, whereas if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium, on the other hand, uses "/Rect" regardless of presence or absence of "/AP". CL fixes PDFium to work similarly to Acrobat, in this case. Additionally, it moves ShouldGenerateAPForAnnotation() from cpvt_generateap.cpp to cpdf_annot.cpp, as the later is a central place to call this function. BUG=pdfium:585 TEST=https://codereview.chromium.org/2300183002/ ==========
Also, can we add a test case with very different "Rect" and "QuadPoints", and check how that is rendered? https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:113: Even if we changed the rect for CPDF_Annot::GetRect() here, CPVT_GenerateAP::Generate*AP still uses pAnnotDict->GetRectBy("Rect"). So the appearance stream for all the annotations are still unchanged. So how did this affect modification of current test cases?
https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:80: bool CPDF_Annot::ShouldUseQuadPointsCoords(CPDF_Array*& pArray) const { nit: can we make this a CPDF_Array** argument, since the style guide prefers that rather than references for out parameters?
On 2016/09/01 20:52:00, jaepark wrote: > Also, can we add a test case with very different "Rect" and "QuadPoints", and > check how that is rendered? Ok. > https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... > File core/fpdfdoc/cpdf_annot.cpp (right): > > https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... > core/fpdfdoc/cpdf_annot.cpp:113: > Even if we changed the rect for CPDF_Annot::GetRect() here, > CPVT_GenerateAP::Generate*AP still uses pAnnotDict->GetRectBy("Rect"). So the > appearance stream for all the annotations are still unchanged. So how did this > affect modification of current test cases? It did affect. For example, in the case of pdfium/testing/corpus/pdfium/annots/annotation_highlight.pdf we have 7 1 obj << /Type /Annot /Rect [115.75062 706.328568 157.001868 719.2715904 ] /Subtype /Highlight /QuadPoints [115.80264 718.9139232 157.211172 718.9139232 115.80264 706.264416 157.211172 706.264416 ] /M (D:20160712221733) /T (þÿ^@J^@a^@e^@ ^@H^@y^@u^@n^@ ^@P^@a^@r^@k) /Contents (þÿ) /NM (okular-{877b885a-ef11-481c-a832-4d86ce91dea2}) /F 4 /C [1 1 0 ] /CA 1 /Border [0 0 1 ] /P 6 0 R >> endobj - Rect[0] (which correspondes to bottom_left_x) has 115.75062 - QuadPoints[4] (which correspondes to bottom_left_x) has 115.80264 The difference is in the pixel level, for example ~0.001% from image_diff after running run_corpus_tests.py.
On 2016/09/01 21:03:47, tonikitoo wrote: > https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... > > core/fpdfdoc/cpdf_annot.cpp:113: > > Even if we changed the rect for CPDF_Annot::GetRect() here, > > CPVT_GenerateAP::Generate*AP still uses pAnnotDict->GetRectBy("Rect"). So the > > appearance stream for all the annotations are still unchanged. So how did this > > affect modification of current test cases? > > It did affect. For example, in the case of > pdfium/testing/corpus/pdfium/annots/annotation_highlight.pdf we have > > 7 1 obj << > /Type /Annot > /Rect [115.75062 706.328568 157.001868 719.2715904 ] > /Subtype /Highlight > /QuadPoints [115.80264 718.9139232 157.211172 718.9139232 115.80264 706.264416 > 157.211172 706.264416 ] > /M (D:20160712221733) > /T (þÿ^@J^@a^@e^@ ^@H^@y^@u^@n^@ ^@P^@a^@r^@k) > /Contents (þÿ) > /NM (okular-{877b885a-ef11-481c-a832-4d86ce91dea2}) > /F 4 > /C [1 1 0 ] > /CA 1 > /Border [0 0 1 ] > /P 6 0 R >> > endobj > > - Rect[0] (which correspondes to bottom_left_x) has 115.75062 > - QuadPoints[4] (which correspondes to bottom_left_x) has 115.80264 > > The difference is in the pixel level, for example ~0.001% from image_diff after > running run_corpus_tests.py. OK. I might be wrong on this, but I'm wondering if the highlight rectangle is drawn using "Rect", and clipped using "QuadPoints". So what we see right now may be the intersection of "Rect" and "QuadPoints". This will be more apparent if we have a test case with widely different "Rect" and "QuadPoints".
https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:21: namespace { When not forward declaring, add blank lines after the start of a namespace and before the end of the namespace. https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:100: // pair0= top_left add a space before '=', to be consistent with lines 106/107. https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/include/cp... File core/fpdfdoc/include/cpdf_annot.h (right): https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/include/cp... core/fpdfdoc/include/cpdf_annot.h:16: #include "core/fpdfapi/fpdf_parser/include/cpdf_array.h" Just forward declare. If you were keeping it, I would have asked for alphabetical order. https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/include/cp... core/fpdfdoc/include/cpdf_annot.h:104: bool ShouldUseQuadPointsCoords(CPDF_Array*&) const; Please try not do add more parameters that pass by non-const ref. Also name the parameter.
> > The difference is in the pixel level, for example ~0.001% from image_diff > after > > running run_corpus_tests.py. > > OK. I might be wrong on this, but I'm wondering if the highlight rectangle is > drawn using "Rect", and clipped using "QuadPoints". So what we see right now may > be the intersection of "Rect" and "QuadPoints". > This will be more apparent if we have a test case with widely different "Rect" > and "QuadPoints". I have uploaded one test case where the difference can be seen more easily visualized: https://people.igalia.com/agomes/pdfium/samples/test_when_no_ap_use_quadpoint... In line 50 there is the follow highlight annotation definition: << /Type /Annot /Rect [ 422 679 472 690 ] /C [ 0.0 0.0 1.0 ] /CA 0.2 /F 4 /QuadPoints [ 422 688 472 688 422 677 472 677 ] /Subtype /Highlight >> (I am testing it with Acrobat reader installed on Fedora/Linux) On Acrobat, manually tweaking the "Rect" coordinates have no effect, whereas tweaking the QuadPoints values, does. This is because the annotation lacks a "AP" definition, and Acrobat uses QuadPoints to grab the annotation coordinates from. For example, manually change the 5th value of the QuadPoints array to from '422' to '402', and the bottom_left_x will of the highlight annotation changes place.
https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:21: namespace { On 2016/09/01 21:12:59, Lei Zhang wrote: > When not forward declaring, add blank lines after the start of a namespace and > before the end of the namespace. Done. https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:80: bool CPDF_Annot::ShouldUseQuadPointsCoords(CPDF_Array*& pArray) const { On 2016/09/01 20:53:08, Tom Sepez wrote: > nit: can we make this a CPDF_Array** argument, since the style guide prefers > that rather than references for out parameters? Done. https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:100: // pair0= top_left On 2016/09/01 21:12:59, Lei Zhang wrote: > add a space before '=', to be consistent with lines 106/107. Done. https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:113: On 2016/09/01 20:52:00, jaepark wrote: > Even if we changed the rect for CPDF_Annot::GetRect() here, > CPVT_GenerateAP::Generate*AP still uses pAnnotDict->GetRectBy("Rect"). So the > appearance stream for all the annotations are still unchanged. So how did this > affect modification of current test This is a good point. I will check further how the generated "/bbox" plays together with quadpoints/rect. For completeness, this is the stack trace of when ::GetRect is called when loading [1] in chromium/pdfium: #0 0x562838b5d03e base::debug::StackTrace::StackTrace() #1 0x56283b6a5585 CPDF_Annot::GetRect() #2 0x56283b6a5e35 FPDFDOC_Annot_GetMatrix() #3 0x56283b6a5cf8 CPDF_Annot::DrawAppearance() #4 0x56283b630c99 CPDFSDK_PageView::PageView_OnDraw() #5 0x56283b62b7ce FPDF_FFLDraw #6 0x56283882ed7c chrome_pdf::PDFiumEngine::FinishPaint() #7 0x56283882e852 chrome_pdf::PDFiumEngine::Paint() #8 0x562838842b26 chrome_pdf::OutOfProcessInstance::OnPaint() #9 0x562838847b82 PaintManager::DoPaint() <..> [1] https://storage.googleapis.com/pdf-test/test.pdf?calcrtid=Tag-1 https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/include/cp... File core/fpdfdoc/include/cpdf_annot.h (right): https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/include/cp... core/fpdfdoc/include/cpdf_annot.h:16: #include "core/fpdfapi/fpdf_parser/include/cpdf_array.h" On 2016/09/01 21:13:00, Lei Zhang wrote: > Just forward declare. > > If you were keeping it, I would have asked for alphabetical order. Done. https://codereview.chromium.org/2289293005/diff/20001/core/fpdfdoc/include/cp... core/fpdfdoc/include/cpdf_annot.h:104: bool ShouldUseQuadPointsCoords(CPDF_Array*&) const; On 2016/09/01 21:13:00, Lei Zhang wrote: > Please try not do add more parameters that pass by non-const ref. Also name the > parameter. Done.
For the record, I have a new patch that addresses owners' comments (including jaepark's request for more tests), which currently sits on top of https://codereview.chromium.org/2310873002/ . I will update this CL as soon as the dependency (hopefully) gets merged.
Description was changed from ========== Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP On Acrobat, if "/AP" is present on the "/Annot" definition, the coordinates used to draw annotations come from "/Rect values, whereas if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium, on the other hand, uses "/Rect" regardless of presence or absence of "/AP". CL fixes PDFium to work similarly to Acrobat, in this case. Additionally, it moves ShouldGenerateAPForAnnotation() from cpvt_generateap.cpp to cpdf_annot.cpp, as the later is a central place to call this function. BUG=pdfium:585 TEST=https://codereview.chromium.org/2300183002/ ========== to ========== Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP On Acrobat, if "/AP" is present on a text markup definition, the coordinates used to draw the annotation come from "/Rect values, whereas if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium, on the other hand, uses "/Rect" regardless of presence or absence of "/AP". CL fixes PDFium to work similarly to Acrobat, in this case. TEST=testing/resources/pixel/bug_585_*.in BUG=pdfium:585 ==========
On 2016/09/06 06:22:25, tonikitoo wrote: > For the record, I have a new patch that addresses owners' comments (including > jaepark's request for more tests), which currently sits on top of > https://codereview.chromium.org/2310873002/ . I will update this CL as soon as > the dependency (hopefully) gets merged. Patchset #4 includes some tests whose "/Rect" values are invalid (e.g. -1 -1 -1 -1). In acrobat/evince, the generated PDFs still draw the text annotations fine, since the PDF files lack "/AP" definitions, and then the coordinates were taken from the /QuadPoints values.
The CQ bit was checked by thestig@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...
So just to be clear - did you use pixel tests instead of adding PDFs to the corpus, because the difference threshold was is too low for corpus tests? i.e. No matter how far away you move the annotation, as a percentage of the total pixels, it's too small for run_corpus_tests to acknowledge the difference. https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpdf_annot... File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:99: bool shouldUseQuadPointsCoords = m_isMarkupAnnotation && m_bHasGeneratedAP; Maybe just drop the variable? https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:191: CFX_FloatRect CPDF_Annot::RectFromQuads(CPDF_Dictionary* pAnnotDict) { Just FYI, there exists a little bit of related code in FPDFLink_GetQuadPoints(). https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpvt_gener... File core/fpdfdoc/cpvt_generateap.cpp (right): https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpvt_gener... core/fpdfdoc/cpvt_generateap.cpp:600: bool bIsTextAnnotation = false) { I would highly prefer we don't have default arguments. We have found so much dead PDFium code that were masked by default arguments that it left a bad taste... https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/include/cp... File core/fpdfdoc/include/cpdf_annot.h (right): https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/include/cp... core/fpdfdoc/include/cpdf_annot.h:117: bool m_isMarkupAnnotation; m_bIsMarkupAnnotation
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/mac/builds/1951) mac_xfa on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_xfa/builds/...)
On 2016/09/06 21:27:04, Lei Zhang wrote: > So just to be clear - did you use pixel tests instead of adding PDFs to the > corpus, because the difference threshold was is too low for corpus tests? i.e. > No matter how far away you move the annotation, as a percentage of the total > pixels, it's too small for run_corpus_tests to acknowledge the difference. Actually, I found it a bit more convenient to write new test cases based on an existing .in file than real PDF files in corpus. In this case specifically, it was verified that the added test cases do get draw using QuadPoints (rather than Rect). Would you think pixel tests are not appropriate here? For simplicity, I combined these 4 tests into a single one, where the four annotation types are present. > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpdf_annot... > File core/fpdfdoc/cpdf_annot.cpp (right): > > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpdf_annot... > core/fpdfdoc/cpdf_annot.cpp:99: bool shouldUseQuadPointsCoords = > m_isMarkupAnnotation && m_bHasGeneratedAP; > Maybe just drop the variable? I added a variable here as a way to self-document the code. If you feel strong, I can remove it. > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpvt_gener... > File core/fpdfdoc/cpvt_generateap.cpp (right): > > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpvt_gener... > core/fpdfdoc/cpvt_generateap.cpp:600: bool bIsTextAnnotation = false) { > I would highly prefer we don't have default arguments. We have found so much > dead PDFium code that were masked by default arguments that it left a bad > taste... Ok, I deliberately made it as a default parameter here, in order to reduce the diff in the end. Changed. > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/include/cp... > File core/fpdfdoc/include/cpdf_annot.h (right): > > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/include/cp... > core/fpdfdoc/include/cpdf_annot.h:117: bool m_isMarkupAnnotation; > m_bIsMarkupAnnotation Done.
Taking care of the tests results here: https://codereview.chromium.org/2300183002/
On 2016/09/06 23:19:29, tonikitoo wrote: > On 2016/09/06 21:27:04, Lei Zhang wrote: > > So just to be clear - did you use pixel tests instead of adding PDFs to the > > corpus, because the difference threshold was is too low for corpus tests? i.e. > > No matter how far away you move the annotation, as a percentage of the total > > pixels, it's too small for run_corpus_tests to acknowledge the difference. > > Actually, I found it a bit more convenient to write new test cases based on an > existing .in file than real PDF files in corpus. > In this case specifically, it was verified that the added test cases do get draw > using QuadPoints (rather than Rect). > Would you think pixel tests are not appropriate here? > For simplicity, I combined these 4 tests into a single one, where the four > annotation types are present. In the git history, it appears we had pixel tests first in February 2015, and then added corpus tests a month later. They have some overlap in functionality, so I'm just trying to understand why you chose one over the other. In comparison, jaepark@ has been adding corpus tests for all the other annotation work. > > > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpdf_annot... > > File core/fpdfdoc/cpdf_annot.cpp (right): > > > > > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpdf_annot... > > core/fpdfdoc/cpdf_annot.cpp:99: bool shouldUseQuadPointsCoords = > > m_isMarkupAnnotation && m_bHasGeneratedAP; > > Maybe just drop the variable? > > I added a variable here as a way to self-document the code. If you feel strong, > I can remove it. Sure, rename it bShouldUseQuadPointsCoords then? > > > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpvt_gener... > > File core/fpdfdoc/cpvt_generateap.cpp (right): > > > > > https://codereview.chromium.org/2289293005/diff/60001/core/fpdfdoc/cpvt_gener... > > core/fpdfdoc/cpvt_generateap.cpp:600: bool bIsTextAnnotation = false) { > > I would highly prefer we don't have default arguments. We have found so much > > dead PDFium code that were masked by default arguments that it left a bad > > taste... > > Ok, I deliberately made it as a default parameter here, in order to reduce the > diff in the end. Changed. One variation is to change GenerateAndSetAPDict() to GenerateAndSetAPDictImpl(), and add wrapper functions named GenerateAndSetAPDictForTextAnnot() / GenerateAndSetAPDictForNonTextAnnot(). A bit wordy, but it may be more readable GenerateAndSetAPDict(foo, ..., false /*IsTextMarkupAnnotation*/) ?
> In the git history, it appears we had pixel tests first in February 2015, and > then added corpus tests a month later. They have some overlap in functionality, > so I'm just trying to understand why you chose one over the other. In > comparison, jaepark@ has been adding corpus tests for all the other annotation > work. > The motivation for the split was that we didn't want all developers (chromium) to have to pull in many MBs (GBs?) of actual PDF files collected over time, since many of them may themselves be large, but wouldn't change since we didn't necessarily have the tool that originally authored them The pixel tests were to be small, hand-written items, so they wouldn't cost much, and were expected that they might have to change as we tweaked code.
https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:51: m_bIsMarkupAnnotation = IsTextMarkupAnnotation(m_nSubtype); m_bIsTextMarkupAnnotation https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:99: bool shouldUseQuadPointsCoords = m_bIsMarkupAnnotation && m_bHasGeneratedAP; To me, checking m_bHasGeneratedAP in RectForDrawing seems a little bit odd. Why do we need to check if annot has generated AP? RectForDrawing should only check whether an annot is a text markup annotation. If it is, use QuadPoints, if not, use Rect. https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:191: CFX_FloatRect CPDF_Annot::RectFromQuads(CPDF_Dictionary* pAnnotDict) { RectFromQuadPoints https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpvt_gener... File core/fpdfdoc/cpvt_generateap.cpp (right): https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpvt_gener... core/fpdfdoc/cpvt_generateap.cpp:600: bool bIsTextAnnotation) { bIsTextMarkupAnnotation. In PDF annotation, there is text annotation, markup annotation, and text markup annotation. So we should make the variable name clear. https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpvt_gener... core/fpdfdoc/cpvt_generateap.cpp:617: : pAnnotDict->GetRectBy("Rect"); It seems like this and RectForDrawing is basically doing the same thing. Can we use the same function for this?
https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:51: m_bIsMarkupAnnotation = IsTextMarkupAnnotation(m_nSubtype); On 2016/09/07 17:30:09, jaepark wrote: > m_bIsTextMarkupAnnotation Done. https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:99: bool shouldUseQuadPointsCoords = m_bIsMarkupAnnotation && m_bHasGeneratedAP; On 2016/09/07 17:30:09, jaepark wrote: > To me, checking m_bHasGeneratedAP in RectForDrawing seems a little bit odd. Why > do we need to check if annot has generated AP? > > RectForDrawing should only check whether an annot is a text markup annotation. > If it is, use QuadPoints, if not, use Rect. Checking for m_bHasGeneratedAP is important here because it toggles using either QuadPoints or Rect coords. In other words, when /AP is generated (i.e. it is absent in the PDF source) is exactly the case we want to use /QuadPoints. https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:191: CFX_FloatRect CPDF_Annot::RectFromQuads(CPDF_Dictionary* pAnnotDict) { On 2016/09/07 17:30:09, jaepark wrote: > RectFromQuadPoints Done. https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpvt_gener... File core/fpdfdoc/cpvt_generateap.cpp (right): https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpvt_gener... core/fpdfdoc/cpvt_generateap.cpp:600: bool bIsTextAnnotation) { On 2016/09/07 17:30:09, jaepark wrote: > bIsTextMarkupAnnotation. > In PDF annotation, there is text annotation, markup annotation, and text markup > annotation. So we should make the variable name clear. Done. https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpvt_gener... core/fpdfdoc/cpvt_generateap.cpp:617: : pAnnotDict->GetRectBy("Rect"); On 2016/09/07 17:30:09, jaepark wrote: > It seems like this and RectForDrawing is basically doing the same thing. Can we > use the same function for this? Good point. Ideally yes, we could use RectForDrawing here if we had a CPDF_Annot instance. In cpvt_generateap.cpp, though, we only have CPDF_Dictionary. So I factored out the Rect extraction off of /QuadPoint logic into this static helper class method, so that call it can be called from here. We could use
On 2016/09/08 04:10:35, tonikitoo wrote: > (..) > We could use left over sentence. Please ignore it.
https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... File core/fpdfdoc/cpdf_annot.cpp (right): https://codereview.chromium.org/2289293005/diff/80001/core/fpdfdoc/cpdf_annot... core/fpdfdoc/cpdf_annot.cpp:99: bool shouldUseQuadPointsCoords = m_bIsMarkupAnnotation && m_bHasGeneratedAP; On 2016/09/08 04:10:35, tonikitoo wrote: > On 2016/09/07 17:30:09, jaepark wrote: > > To me, checking m_bHasGeneratedAP in RectForDrawing seems a little bit odd. > Why > > do we need to check if annot has generated AP? > > > > RectForDrawing should only check whether an annot is a text markup annotation. > > If it is, use QuadPoints, if not, use Rect. > > Checking for m_bHasGeneratedAP is important here because it toggles using either > QuadPoints or Rect coords. > > In other words, when /AP is generated (i.e. it is absent in the PDF source) is > exactly the case we want to use /QuadPoints. Acknowledged.
lgtm if jaepark@ is happy with this CL.
On 2016/09/08 23:02:45, Lei Zhang wrote: > lgtm if jaepark@ is happy with this CL. lgtm with DEPS roll for testing/corpus. You might want to update https://codereview.chromium.org/2300183002/ . There's one more test case added in https://codereview.chromium.org/2301793002 .
On 2016/09/08 23:13:09, jaepark wrote: > On 2016/09/08 23:02:45, Lei Zhang wrote: > > lgtm if jaepark@ is happy with this CL. > > lgtm with DEPS roll for testing/corpus. > You might want to update https://codereview.chromium.org/2300183002/ . > There's one more test case added in https://codereview.chromium.org/2301793002 . https://codereview.chromium.org/2300183002/ is updated with annotation_highlight_empty_content rebasiline. As soon as it gets merged, I will update DEPS here. Thanks!
This needs a rebase, BTW.
On 2016/09/13 20:35:34, Lei Zhang (OOO) wrote: > This needs a rebase, BTW. Done in patchset #7.
The CQ bit was checked by tonikitoo@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, jaepark@google.com Link to the patchset: https://codereview.chromium.org/2289293005/#ps120001 (title: "rebased against ToT")
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
Try jobs failed on following builders: mac on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/mac/builds/2034)
The CQ bit was checked by tonikitoo@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, jaepark@google.com Link to the patchset: https://codereview.chromium.org/2289293005/#ps140001 (title: "rebased + new DEPS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP On Acrobat, if "/AP" is present on a text markup definition, the coordinates used to draw the annotation come from "/Rect values, whereas if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium, on the other hand, uses "/Rect" regardless of presence or absence of "/AP". CL fixes PDFium to work similarly to Acrobat, in this case. TEST=testing/resources/pixel/bug_585_*.in BUG=pdfium:585 ========== to ========== Use either /RECT or /QuadPoints for annotation coordinates, depending on /AP On Acrobat, if "/AP" is present on a text markup definition, the coordinates used to draw the annotation come from "/Rect values, whereas if "/AP" is not defined, the array defined in /QuadPoints is used to grab the annotation coordinates from. PDFium, on the other hand, uses "/Rect" regardless of presence or absence of "/AP". CL fixes PDFium to work similarly to Acrobat, in this case. TEST=testing/resources/pixel/bug_585_*.in BUG=pdfium:585 Committed: https://pdfium.googlesource.com/pdfium/+/0a17fafd723e8684d1deb4b5ceea58967a01... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://pdfium.googlesource.com/pdfium/+/0a17fafd723e8684d1deb4b5ceea58967a01... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
