BUG: Fix test_watermarking_reportlab_rendering()

[Lucas-C]

This fixes the issue spotted in #2191

The solution was to re-introduce calls to PageObject._push_pop_gs(),
in PageObject._merge_page & PageObject._merge_page_writer(),
but to optimize PageObject._push_pop_gs() by introducing a ContentsStream.isolate_graphics_state() method.

[Lucas-C]

I wonder if we could use this opportunity to consider that use_original=True is always passed to PageObject._push_pop_gs(), and hence never re-recreate a ContentStream object, which could both simplify the code and improve the performances...

Edit: I did that in the 2nd commit of this PR, and it seems to work fine.

[pubpub-zz]

I wonder if we could use this opportunity to consider that use_original=True is always passed to PageObject._push_pop_gs(), and hence never re-recreate a ContentStream object, which could both simplify the code and improve the performances...

What about to do the merge just work on the decode byte streams? to conversion to operations takes some times and we just need something like:
Newcontent.decodedstream.set_data( b"q\n" + page1.content.get_data() +b"Q\nq\n" + page2.rotation( if required) + page2.content.get_data() +b"Q\n")

no ?

[codecov]

Codecov Report

Patch coverage: 92.30% and project coverage change: +0.01% 🎉

Comparison is base (34c6875) 94.37% compared to head (bef49af) 94.38%.
Report is 4 commits behind head on main.

❗ Current head bef49af differs from pull request most recent head 22fb6c5. Consider uploading reports for the commit 22fb6c5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2203      +/-   ##
==========================================
+ Coverage   94.37%   94.38%   +0.01%     
==========================================
  Files          43       43              
  Lines        7588     7588              
  Branches     1496     1497       +1     
==========================================
+ Hits         7161     7162       +1     
+ Misses        263      262       -1     
  Partials      164      164              

Files Changed	Coverage Δ
pypdf/generic/_data_structures.py	91.88% <83.33%> (-0.07%)	⬇️
pypdf/_page.py	94.35% <100.00%> (+0.18%)	⬆️
pypdf/_utils.py	98.58% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Lucas-C]

What about to do the merge just work on the decode byte streams? to conversion to operations takes some times and we just need something like:

Sorry, I don't quite understand your suggestion 😅
Where are you suggesting to perform that change exactly?
What is Newcontent.decodedstream in the line of code you mentioned?
Overall, isn't what you suggest already part of this PR?

[pubpub-zz]

Sorry, I don't quite understand your suggestion 😅

It is quite a draft. I will make a test and prepare a PR if performance is good 😉

[Lucas-C]

It is quite a draft. I will make a test and prepare a PR if performance is good 😉

Alright! 🙂

In the meantime, would you or @MartinThoma be OK to merge this PR?

[pubpub-zz]

for me, this PR is completely valid and what I have in mind will not modify this PR.

[stefan6419846]

In PyMuPDF, double-wrapping will be prevented: https://github.com/pymupdf/PyMuPDF/blob/1c2f1da3eb2541f1c8dbd50acb3a916939c99d3e/src/__init__.py#L9141-L9146 (the is_wrapped check uses additionally some caching mechanism to increase performance, see https://github.com/pymupdf/PyMuPDF/blob/1c2f1da3eb2541f1c8dbd50acb3a916939c99d3e/src/__init__.py#L8864-L8876 as well).

Would this be a possible enhancement here as well? Otherwise, running pypdf on the same PDF page multiple times might generate lots of "useless" wrappers.

[MartinThoma]

Thank you for taking care of this @Lucas-C 🙏 I love that you're so actively contributing now 🎉

To me the PR looks good. @stefan6419846 left two interesting comments. I will take care of the docstring part, but the double-wrapping might be something interesting for another PR.

[Lucas-C]

To me the PR looks good. @stefan6419846 left two interesting comments. I will take care of the docstring part, but the double-wrapping might be something interesting for another PR.

I agree! 🙂

If I understand correctly, your suggestion @stefan6419846 would be to add a .is_wrapped attribute to ContentStream, in order to avoid re-wrapping in q ... Q it if it's already done?
I wonder if that can really happen often in practice: have you witnessed it?
Could you maybe provide a minimal test case reproducing this?

[stefan6419846]

If I understand correctly, your suggestion @stefan6419846 would be to add a .is_wrapped attribute to ContentStream, in order to avoid re-wrapping in q ... Q it if it's already done?

Yes, something like this.

I wonder if that can really happen often in practice: have you witnessed it?
Could you maybe provide a minimal test case reproducing this?

It generally is good practice to already have an isolated graphics state for each page of the PDF file, so a "clean" PDF file would already have wrapped pages. pypdf will currently add another wrapping layer each time.

An easy example to reproduce this is by adding two different watermarks/overlays/backgrounds for example:

from pypdf import PdfReader, PdfWriter


watermark1 = PdfReader('watermark1.pdf').pages[0]
watermark2 = PdfReader('watermark2.pdf').pages[0]

writer = PdfWriter(clone_from='file.pdf')
for page in writer.pages:
    page.merge_page(watermark1)
    page.merge_page(watermark2)

Suppose we start with self._data being q\n 841.680[...]0 Do\nQ\n\n \n. Running the above example leads to self._data being q\nq\nq\n 841[...]ET\nQ\nQ\n\nQ\n, thus we have isolated the graphics state three times, although once should be enough.

[pubpub-zz]

@MartinThoma I've found a "regression" in the test the skipping decorator if ghostscript is not present is missing
fixed in reference PR