[GitHub] [openoffice] ardovm opened a new pull request #89: Use std::vector instead of fixed-size array of cffLocal objects

[GitBox]

ardovm opened a new pull request #89:
URL: https://github.com/apache/openoffice/pull/89


   This is a follow-up of [bug 125359](https://bz.apache.org/ooo/show_bug.cgi?id=125359).
   
   Instead of using fixed-size arrays, use `std::vector` as a variable-sized array for `CffLocal` objects.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-652464308


   I see that in the same file there are other fixed-size arrays used as stacks (if I understand correctly):
   * `CffSubsetterContext::mnValStack`,
   * `CffSubsetterContext::mnHintStack`.
   
   If it is of any interest to turn also them into `std::vector` or other containers, please let me know, and I could include them into this PR.
   
   Please be aware that this is my first PR against OpenOffice; if there are any tests I can run to validate these changes, kindly give me some pointers. By now, I just built the source and tried to reproduce the crash originating [bug 12359](https://bz.apache.org/ooo/show_bug.cgi?id=125359).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-652654917


   If you ncould change those and add them to the PR would be great. Or maybe make a new PR. I have a look on the weekend.
   
   I try to get some pointers on testing. I struggle there myself.
   
   all the best
   Petko


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-652807797


   Thank you, Petko. It would help me to know what is the expected average size and variation of such stacks, to give a "meaningful" initial size and optimize the number of calls to `std::vector::resize()`.
   
   My real problem is, in fact, that I have no idea about the purpose of that code! For example, what is a CFF? And where is that code called from? (I could do a full-text search of the sources, but some more structured information would help).
   
   I like documenting my own code and I would be glad to help also in this way, at least in this file we are working on. Are there any coding standards about code documentation in OpenOffice.org? For example, are you adopting Doxygen, JavaDoc or the like?
   
   Thank you in advance and best regards.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

Pilot-Pirx commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-653749584


   As a first test I could build AOO42X on Windows with this patch applied.
   I could also successfully export a document to PDF with Noto Sans CJK font.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-653869909


   @ardovm
   I searched a bit for CFF on our opengrok site ( http://opengrok.openoffice.org/search?project=trunk&full=CFF&defs=&refs=&path=&hist=&type=&si=full ). And it looks like this is something to do with Fonts. Searching for fonts and CFF brought up this explanation:
   https://www.linotype.com/8120/the-difference-between-cff-and-ttf.html
   
   I am not sure. But there is a cff.cxx file which has some fixed Array. I guess that they are linked.
   HTH. I will check later for further analysis if I can dig more Information out.
   
   Sorry that we have to do a bit of R&D and I can not give you a steight answer.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-657786860


   I am trying to get a grasp on the CFF concepts. It's... a whole world. Thank you @leginee for the pointers!
   
   However it will take me some more time to understand if it is worth changing also those "stacks" to `std::vector`'s... I hope you are not in a hurry :-)
   
   In the meantime, I took the liberty to add some comments. Feel free to criticize the style.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-657991972


   I like the comments. 👍
   Take the time you need. We are all on the same team.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-662696345


   I started to dig a bit deeper into CFF and Type2, then I understood they are out of my reach.
   In the end, if the code works well as it is, I would refrain to change it any more than this.
   
   Here is what I understood about the other fixed-size arrays: `CffSubsetterContext::mnValStack`, `CffSubsetterContext::mnTransVals` and `CffSubsetterContext::mnHintStack`.
   They are sized as per the Type 2 Charstring Format specifications, appendix B.
   They are often accessed by index, i.e. not only by push and pop operations.
   Are we interested in adding run-time checks on their boundaries? Limits should not be crossed by compliant fonts.
   
   If we wanted to minimize the size of objects, we could move them to the heap using fixed-size `std::vector`, but I am not sure we need this.
   
   Please note that I partially reverted one commit. I could force-push and merge everything into a single commit to make merging simpler.
   
   I am waiting for indications, on whether and how to proceed any further.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-662838003


   Thanks for the feedback. I think the patch is then feature complete.
   To have one commit that can be simple be merged would be nice. Please do the merge.
   
   I am not sure if there is some unit test around. I will check on this when I review. But it would be preferable if we could add a unit test that checks the behavior.
   
   > Are we interested in adding run-time checks on their boundaries?
   can we create a test scenario that may break the boundaries? - If we have a unit test that manages to break the code I say yes we should implement checks for that, that are efficient as possible.
   
   I try to review next week.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee edited a comment on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-662838003


   Thanks for the feedback. I think the patch is then feature complete.
   To have one commit that can be simple be merged would be nice. Please do the merge.
   
   I am not sure if there is some unit test around. I will check on this when I review. But it would be preferable if we could add a unit test that checks the behavior.
   
   > Are we interested in adding run-time checks on their boundaries?
   
   can we create a test scenario that may break the boundaries? - If we have a unit test that manages to break the code I say yes we should implement checks for that, that are efficient as possible.
   
   I try to review next week.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-662862458


   > Thanks for the feedback. I think the patch is then feature complete.
   > To have one commit that can be simple be merged would be nice. Please do the merge.
   
   Done.
   
   > I am not sure if there is some unit test around. I will check on this when I review. But it would be preferable if we could add a unit test that checks the behavior.
   >
   > > Are we interested in adding run-time checks on their boundaries?
   >
   > can we create a test scenario that may break the boundaries? - If we have a unit test that manages to break the code I say yes we should implement checks for that, that are efficient as possible.
   
   If I understand correctly the matter, we should test the code against actual CFF fonts.
   I would not know where to start.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

cbmarcum commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-694603116


   I built trunk CentOS 7 without this PR applied and tested a writer document with Noto Sans CKJ JP and CKJ SC and neither one crashed on PDF Export. Should it always crash?  I'm trying to verify a baseline to test the PR against.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

Pilot-Pirx commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-694699613


   The crash was already fixed in trunk (AOO42X and now AOO418).
   This is just a better fix and some additional comments.
   Not sure how to test it...


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

pedlino commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-694711358


   Just followed the steps using AOO418m1(Build:9801)  -  Rev. 07cb168e39 2020-08-26 09:48 - Linux x86_64
   under Ubuntu 18.04 x64 and there was no crash.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-695195811


   A good test is after apply that the Font still works as expected. The patch first was a quick and dirty fix, and this should be the proper one.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-695215925


   > A good test is after apply that the Font still works as expected. The patch first was a quick and dirty fix, and this should be the proper one.
   
   Are there any automated tests about the proper rendering of fonts? Like a comparison with a pre-rasterized sample?
   
   I could prepare a very simple ODT with English and Chinese characters, but it would require human evaluation.
   Something like:
   
   > This character is an horizontal line: 一
   > This character is two horizontal lines: 二
   > This character looks like a tall rectangle with an horizontal line in the middle: 日
   > Here are letters "aeiou" with grave accents: àèìòù
   
   Having the above in the Noto Sans CJK font would ensure that English, European and Chinese characters are all rendered correctly (the first three characters were copied and pasted from Chinese text).
   
   I am feeling a bit stupid while typing this, like I am reinventing the wheel using the wrong tools, but please understand I am only trying to help.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-695216559


   I forgot to mention that while developing this PR, I made some mistakes that produced _invalid output_ with no crashes. The PDF was wrong (garbled characters) but no error messages were visible.
   The text was 99% English in a standard font, with some Chinese characters in Noto Sans CJK. The English text was not affected by the bug I had introduced, only the Chinese characters were.
   I felt _lucky_ I spotted the garbled Chinese characters (I can't read Chinese!) and therefore I could find and fix the problem.
   If we had an _automatic_ way of checking font rendering, that would protect us from similar errors.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

pedlino commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-695216940


   > > A good test is after apply that the Font still works as expected. The patch first was a quick and dirty fix, and this should be the proper one.
   >
   > Are there any automated tests about the proper rendering of fonts? Like a comparison with a pre-rasterized sample?
   >
   > I could prepare a very simple ODT with English and Chinese characters, but it would require human evaluation.
   > Something like:
   >
   > > This character is an horizontal line: 一
   > > This character is two horizontal lines: 二
   > > This character looks like a tall rectangle with an horizontal line in the middle: 日
   > > Here are letters "aeiou" with grave accents: àèìòù
   >
   > Having the above in the Noto Sans CJK font would ensure that English, European and Chinese characters are all rendered correctly (the first three characters were copied and pasted from Chinese text).
   >
   > I am feeling a bit stupid while typing this, like I am reinventing the wheel using the wrong tools, but please understand I am only trying to help.
   
   Never feel stupid for trying to help! If the wheel was already invented but you can't find it, it is certainly not your fault ;)
   
   Such an ODT would help. I think the description is not necessary. What is needed is a screenshot before and after the patch is applied. If visually there is no loss of characters or loss of quality of rendering then the patch can be accepted.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

pedlino edited a comment on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-695216940


   > I am feeling a bit stupid while typing this, like I am reinventing the wheel using the wrong tools, but please understand I am only trying to help.
   
   Never feel stupid for trying to help! If the wheel was already invented but you can't find it, it is certainly not your fault ;)
   
   Such an ODT would help. I think the description is not necessary. What is needed is a screenshot before and after the patch is applied. If visually there is no loss of characters or loss of quality of rendering then the patch can be accepted.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]