Bug 50852

Created attachment 26701 [details]
Patch 01/20

Created attachment 26702 [details]
Patch 02/20

Created attachment 26703 [details]
Patch 03/20

Created attachment 26704 [details]
Patch 04/20

Created attachment 26705 [details]
Patch 05/20

Created attachment 26706 [details]
Patch 06/20

Created attachment 26707 [details]
Patch 07/20

Created attachment 26708 [details]
Patch 08/20

Created attachment 26709 [details]
Patch 09/20

Created attachment 26710 [details]
Patch 10/20

Created attachment 26711 [details]
Patch 11/20

Created attachment 26712 [details]
Patch 12/20

Created attachment 26713 [details]
Patch 13/20

Created attachment 26714 [details]
Patch 14/20

Created attachment 26715 [details]
Patch 15/20

Created attachment 26716 [details]
Patch 16/20

Created attachment 26717 [details]
Patch 17/20

Created attachment 26718 [details]
Patch 18/20

Created attachment 26719 [details]
Patch 19/20

Created attachment 26720 [details]
Patch 20/20

Hi Martin,

I finally got round to having a look at your patch. First, I’d like to 
thank you for having taken the time to create 20 smaller self-contained 
patches. This made the review much easier. So thanks for that!

The new data structure is indeed much more efficient than the DOM that 
FOP manipulates at the moment. Unfortunately, this doesn’t solve (AFAIU) 
the fundamental problem that we recently discovered: empty content may 
be the cause for a wrong final structure tree. Take the following table:

  <fo:table width="100%" table-layout="fixed">
    <fo:table-body>
      <fo:table-row>
        <fo:table-cell>
          <fo:block>Cell 1.1</fo:block>
        </fo:table-cell>
        <fo:table-cell>
          <fo:block>Cell 1.2</fo:block>
        </fo:table-cell>
      </fo:table-row>
      <fo:table-row>
        <fo:table-cell>
          <fo:block/>
        </fo:table-cell>
        <fo:table-cell>
          <fo:block>Cell 2.2</fo:block>
        </fo:table-cell>
      </fo:table-row>
    </fo:table-body>
  </fo:table>

The content of the first cell in the second row is empty, which will 
result into a TR structure element having only one TD kid for the second 
cell; that TD element will be mistakenly interpreted by a screen reader 
as belonging to the first column.

See also discussion here:
http://markmail.org/message/mn7jdbxmjdq7ey52

To solve this we need to integrate the handling of the structure tree 
into the normal processing chain (FO tree -> Layout Engine -> Area tree) 
instead of bypassing it.


That said, I have a few comments and questions relating to some of your 
specific patches:
[PATCH 10/20] Avoid overhead of creating writers
I can imagine that if there are a lot of PDF objects to stream, creating 
an instance of BufferedWriter and OutputStreamWriter for each of them 
may have quite some performance impact. However replacing them with 
calls to PDFObject.encode everywhere it is necessary is not really an 
option. This makes the code difficult to read and maintain, and is 
error-prone as it’s very easy to miss one call somewhere.

I think the problem of encoding text into the output should be solved by 
defining a specialized PDFOutputStream that would be able to stream both 
String and bytes. That PDFOutputStream would be passed around to objects 
that then wouldn’t have to handle their own wrapper or make calls to 
encode. Does that make sense?

[PATCH 11/20] Add support for clearing objects at write time
I’m wondering why this is necessary? Isn’t it just possible to null out 
references to the objects and let the garbage collector do the work?

[PATCH 12/20] Add support for lazy object number assignment
Same here, what’s exactly the purpose of lazy object number assignment?


Thanks,
Vincent

I'll split my answers.

Add "Add support for lazy object number assignment":

The codes creates an PDF structure tree containing objects, which will be trimmed away (eg. because they are empty leaf objects).

Without lazy object number assignment such objects will get an object number too, as they are assigned while creating these objects. It will increase the xref size and the xref entries for such numbers will be incorrect (as there is no support for free object numbers).

Lazy assignment is the simplest solution for this problem without rewriting larger parts of the code. For such objects, the assignment will happen, when the first reference to such objects (or the object itself) is written into the PDF.

Add "empty table cell problem":

This should be easy to solve:
In PDFStructElement.prepare() 
change 
        if (kids.length() == 0) {

If the test is extended to only trigger, if the current structure element is not a table cell [getStructureType() not in (TD, TH, ..) && kids.length() == 0], an empty table cell will be put in the structure tree.

Maybe PDFStructElement should get a trim empty leaf flag and the decision moved to the FOToPDFRoleMap.

Add "Avoid overhead of creating writers":

By default, nearly each FO tag results in an PDF structure element [= object], so there are lots of OutputStreamWriters created.

The trunk implementation with two objects (Writer+Stream) has the weak point, that forgetting to flush the writer before using the stream can lead to incorrect PDFs.

As OutputStream.write only supports bytes, any forgotten encode will result in an syntax error => My solution will catch errors at compile time.

> I think the problem of encoding text into the output should be solved by 
> defining a specialized PDFOutputStream that would be able to stream both 
> String and bytes. That PDFOutputStream would be passed around to objects 
> that then wouldn’t have to handle their own wrapper or make calls to 
> encode. Does that make sense?

If the PDFOutputStream is persistent (eg, not recreated for each batch of objects), it is an interesting idea.

The current code creates lots of CountingOutputStreams too, which would be needed to be wrapped in a PDFOutputStream. To overcome this, the new PDFOutputStream could count the total number of written bytes. Instead of using a CountingOutputStream, the code can store the start number of written bytes and use the difference to the end number of written bytes.

This is an optional enhancement, therefore I would currently like to concentrate on the other patches.

"Add support for clearing objects at write time":

The structure tree is one big document wide tree. The root is written after the last page. 

The structure tree has circular links (parent=>child, child=>parent) and external links to tree nodes (eg. from the per page element list). 

If we don't want to keep the whole thing into memory, we need to write parts of the tree and then purge all references to them.

Without this patch, a write of structure elements would be:
* add the object to the PDF document
* callback to the caller do the actual write or get the OutputStream somehow to do the write (The whole logic is in PDFLogicalStructureHandler, which currently only has access to the PDF document, not the output stream).
* Traverse the written structure elements again and break the loops

With this patch, a write of structure elements is only adding the objects to the document.
The next write operation will store the streams and call clean the objects afterwards. For a structure element, the clean operation is mostly removing it's links to child elements, which also results in breaking the loops.

The same logic could eg. be used for writing pages: The clean function of a pages would be deleting the page content.

(In reply to comment #23)
> Add "empty table cell problem":
> 
> This should be easy to solve:
> In PDFStructElement.prepare() 
> change 
>         if (kids.length() == 0) {
> 
> If the test is extended to only trigger, if the current structure element is
> not a table cell [getStructureType() not in (TD, TH, ..) && kids.length() ==
> 0], an empty table cell will be put in the structure tree.

I tried commenting out all the code in that method that removes elements and it does not work.

Anyway, as described in the mailing list discussion that I linked to in comment #21, I think the structure tree should not be trimmed, as there is structural information there that would be necessary for a round-trip process. But in the current design there is no way to create the right order of text and other structure elements.


> Maybe PDFStructElement should get a trim empty leaf flag and the decision moved
> to the FOToPDFRoleMap.

Created attachment 27171 [details]
Handle empty nodes

The attached patch should be able to handle both corner cases for empty blocks/table-cells (at least my testcase).

Empty blocks can have a meaning in the structure tree. Or they are created for technical reasons without any meaning. I created an interface for this policy decision in FOToPDFRoleMap. The patch includes a simple test policy - the final policy will require further discussion.

resetting P2 open bugs to P3 pending further review

increase priority due to presence of a patch

vincent/peter, what is the status of this patch with your recent accessibility improvements? have these patches been applied? if not, then do expect to apply them? (when?)

(In reply to comment #30)
> vincent/peter, what is the status of this patch with your recent accessibility
> improvements? have these patches been applied? if not, then do expect to apply
> them? (when?)

vincent/peter?

(In reply to comment #31)
> (In reply to comment #30)
> > vincent/peter, what is the status of this patch with your recent accessibility
> > improvements? have these patches been applied? if not, then do expect to apply
> > them? (when?)
> 
> vincent/peter?

Some of those patches may still be worth applying as they may bring improvements in areas that were not touched by the work on accessibility. I'll need to have another look.

Created attachment 28824 [details]
Mark no longer relevant patches as obsolete

The accessibility code has been refactored to fix the fundamental limitation that empty content was not properly handled. It also suppresses the need for an intermediate representation of the structure tree and directly generates the appropriate PDF structure elements from the FO tree.

Marking as obsolete the patches that are no longer relevant in the context of the new implementation. (Adding an empty patch is the quickest way I found to avoid editing each attachment individually.)

Created attachment 28825 [details]
Treat some content as artifact

This is an adaptation of patch #19 to the new accessibility code. I chose the value 'artifact' instead of 'hidden' to follow the naming convention of the PDF Reference.

I also chose to implement this special property value on fo:wrapper only, for simplicity. It should be easy to wrap any content that must be treated as artifact into an appropriate fo:wrapper element.

I didn't implement the 'inherit' value, as I believe the PDF Reference already provides a standard structure type ('NonStruct') that allows to achieve a similar result. The node is not removed from the structure tree but the end result should be the same.

Any feedback is welcome. If I don't hear anything within 3 working days, I'll assume lazy consensus and apply the patch.

Thanks,
Vincent

Created attachment 28826 [details]
Treat some content as artifact – Documentation

This patch is against the staging CMS website, that sooner or later will become the official website.

Comment on attachment 26718 [details]
Patch 18/20

Applied patch #18 in rev. 1341992:
http://svn.apache.org/viewvc?rev=1341992&view=rev

I left the criteria for compact output in PDFDictionary to 2, as we have to decide on a size and I don't think the savings are significant anyway (2 bytes per entry).

Thanks for the suggestion,
Vincent

Comment on attachment 26717 [details]
Patch 17/20

I must mark this patch as invalid. The structure tree corresponds to the logical structure of the document and is not meant to reflect the way it is laid out on pages. So the static-content cannot be duplicated in the structure tree as many times as there are pages in the final output.

As hinted by the PDF Reference, the proper way to deal with content that is repeated over pages is to treat it as artifact. This will become possible once patches #20 and artifact have been handled.

Vincent

Comment on attachment 26716 [details]
Patch 16/20

This patch does no longer apply after the modifications brought to the accessibility code. Feel free to submit an updated patch, although the matter has been made slightly more complicated by the presence of placeholder structure elements. Also, it would be good to improve encapsulation, immutability and robustness by avoiding some of the tests (instanceof PDFDictionary, type equal to "/MCR"). Maybe a method like addMarkedContentKid on PDFStructElem would do. That would also avoid creating an instance of PDFDictionary, only to discard it later on because the reference to the kid can be simplified.

This simplification has been made less critical by the implementation of object streams, that allow to achieve a much more significant reduction of the file size. It may be of interest in cases where for some reason the user has to stick to PDF 1.4. But I'm not sure how common such a use case is.

Thanks,
Vincent

Comment on attachment 26720 [details]
Patch 20/20

I applied the changes suggested by this patch in rev. 1342680:
http://svn.apache.org/viewvc?rev=1342680&view=rev

I adapted the changes to the new accessibility code and I also implemented support for the intermediate format.

Thanks,
Vincent

I applied the artifact patch in rev. 1343632:
http://svn.apache.org/viewvc?rev=1343632&view=rev

This fixes the last remaining issue for this bug.