Add simple tests for Brotli compression [pdfium : main]
Aryan Krishnan (Gerrit)
Aryan Krishnan added 8 comments![]( "Open in Gerrit")
Add Simple Tests for Brotli CompressionAryan Krishnanlowercase
Done
Add Simple Tests for Brotli CompressionAryan Krishnanlowercase
Done
sources += [] # TODO(https://crbug.com/475855993): Add Brotli sourceAryan KrishnanThis is a no-op. Just omit it. The TODO comment is sufficient.
Done
if (pdf_enable_brotli) {Aryan KrishnanIs this necessary for unittests?
Done
if (pdf_enable_brotli) {Aryan KrishnanPut the conditionals after the non-conditionals.
Done
class BrotliEmbedderTest : public EmbedderTest {};Aryan Krishnan`using BrotliEmbedderTest = EmbedderTest;`
Done
pdf_enable_brotli = falseAryan KrishnanTry to keep the list somewhat sorted. Move above `pdf_enable_click_logging`.
Done
Aryan KrishnanWhere exactly are these files from?
From the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 1 comment![]( "Open in Gerrit")
# TODO(https://crbug.com/475855993): Add Brotli source files here onceSome formatting still left...
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
if (pdf_enable_brotli) {Aryan KrishnanIs this necessary for unittests?
Done
Not sure what was done.
Aryan KrishnanWhere exactly are these files from?
From the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
It's a bit vague. Assuming this is https://pdfa.org/download-area/examples/Brotli-Prototype-PDFs.zip, add that to the commit message to be more specific?
Related details
- Andy Phan
- Aryan Krishnan
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 2 comments![]( "Open in Gerrit")
if (pdf_enable_brotli) {Aryan KrishnanIs this necessary for unittests?
Lei ZhangDone
Not sure what was done.
Removed the same sources += [] no-op.
Assuming then that you want me to remove the whole block?
Done I think.
Aryan KrishnanWhere exactly are these files from?
Lei ZhangFrom the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
It's a bit vague. Assuming this is https://pdfa.org/download-area/examples/Brotli-Prototype-PDFs.zip, add that to the commit message to be more specific?
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Aryan KrishnanWhere exactly are these files from?
Lei ZhangFrom the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
Aryan KrishnanIt's a bit vague. Assuming this is https://pdfa.org/download-area/examples/Brotli-Prototype-PDFs.zip, add that to the commit message to be more specific?
Added the link.
Thanks. It's unclear what license those files are released under. So instead of using PDFA's example, how about creating one from scratch, similar to testing/resources/utf-8.in. Let me know if you need help.
Related details
- Andy Phan
- Aryan Krishnan
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 1 comment![]( "Open in Gerrit")
# TODO(https://crbug.com/475855993): Add Brotli source files here onceAryan KrishnanSome formatting still left...
Done
Related details
- Andy Phan
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 1 comment![]( "Open in Gerrit")
Aryan KrishnanWhere exactly are these files from?
Lei ZhangFrom the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
Aryan KrishnanIt's a bit vague. Assuming this is https://pdfa.org/download-area/examples/Brotli-Prototype-PDFs.zip, add that to the commit message to be more specific?
Lei ZhangAdded the link.
Thanks. It's unclear what license those files are released under. So instead of using PDFA's example, how about creating one from scratch, similar to testing/resources/utf-8.in. Let me know if you need help.
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 3 comments![]( "Open in Gerrit")
Aryan KrishnanAlso needed to add some local modifications to brotli to get the gn gen to work locally.
Main changes:
1. Default argument for building with fuzzers as true in brotli. This can be changed to false by PDFium args
2. Including pdfium.gni into the file for symbols related to ubsan.2. Cant be upstreamed.
But I did upload 1 in case any other embedders who use brotli are facing a similar issue, if not perhaps this can be added as a local modification.
https://chromium-review.googlesource.com/c/chromium/src/+/7882000Before (without local changes):
```
ERROR at //third_party/brotli/BUILD.gn:10:3: Unable to load "~/repo/pdfium/testing/libfuzzer/fuzzer_test.gni".
import("//testing/libfuzzer/fuzzer_test.gni")
^-------------------------------------------
See //core/fxcodec/BUILD.gn:149:17: which caused the file to be included.
deps += [ "../../third_party/brotli:dec" ]
^-----------------------------
```
After (with local changes except setting default to false instead - same effect as us overriding from the PDFium side):
```
Done. Made 811 targets from 221 files in 602ms
```
Have another modification:
https://chromium-review.googlesource.com/c/chromium/src/+/7882511
TEST_F(BrotliEmbedderTest, DISABLED_PrototypeA) {Aryan KrishnanDo the tests currently fail if they run?
Well now my new samples fail at OpenDocument for some reason but I added extra steps which BrotliDecode should pass so we can keep using the existing tests even after we fix the issues.
Aryan KrishnanWhere exactly are these files from?
Lei ZhangFrom the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
Aryan KrishnanIt's a bit vague. Assuming this is https://pdfa.org/download-area/examples/Brotli-Prototype-PDFs.zip, add that to the commit message to be more specific?
Lei ZhangAdded the link.
Aryan KrishnanThanks. It's unclear what license those files are released under. So instead of using PDFA's example, how about creating one from scratch, similar to testing/resources/utf-8.in. Let me know if you need help.
Will do, but it might take me a while to get it done.
Done
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 1 comment![]( "Open in Gerrit")
Aryan KrishnanWhere exactly are these files from?
Lei ZhangFrom the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
Aryan KrishnanIt's a bit vague. Assuming this is https://pdfa.org/download-area/examples/Brotli-Prototype-PDFs.zip, add that to the commit message to be more specific?
Lei ZhangAdded the link.
Aryan KrishnanThanks. It's unclear what license those files are released under. So instead of using PDFA's example, how about creating one from scratch, similar to testing/resources/utf-8.in. Let me know if you need help.
Aryan KrishnanWill do, but it might take me a while to get it done.
Done
Not quite sure if it would really work and it is kind of hard to tell if the OpenDocument() failure is from Brotli or from something else with the strutcure of the PDF.
Aryan Krishnan (Gerrit)
Aryan Krishnan added 1 comment![]( "Open in Gerrit")
Aryan KrishnanAlso needed to add some local modifications to brotli to get the gn gen to work locally.
Main changes:
1. Default argument for building with fuzzers as true in brotli. This can be changed to false by PDFium args
2. Including pdfium.gni into the file for symbols related to ubsan.2. Cant be upstreamed.
But I did upload 1 in case any other embedders who use brotli are facing a similar issue, if not perhaps this can be added as a local modification.
https://chromium-review.googlesource.com/c/chromium/src/+/7882000Before (without local changes):
```
ERROR at //third_party/brotli/BUILD.gn:10:3: Unable to load "~/repo/pdfium/testing/libfuzzer/fuzzer_test.gni".
import("//testing/libfuzzer/fuzzer_test.gni")
^-------------------------------------------
See //core/fxcodec/BUILD.gn:149:17: which caused the file to be included.
deps += [ "../../third_party/brotli:dec" ]
^-----------------------------
```
After (with local changes except setting default to false instead - same effect as us overriding from the PDFium side):
```
Done. Made 811 targets from 221 files in 602ms
```
Have another modification:
https://chromium-review.googlesource.com/c/chromium/src/+/7882511
Done
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 1 comment![]( "Open in Gerrit")
Ok seems like these samples are a bit broken or something - will upload a fix soon...
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 1 comment![]( "Open in Gerrit")
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aryan Krishnan
| Commit-Queue | +1 |
1 comment
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aryan Krishnan
| Commit-Queue | +1 |
1 comment
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 1 comment![]( "Open in Gerrit")
TEST_F(BrotliEmbedderTest, DISABLED_PrototypeA) {Aryan KrishnanDo the tests currently fail if they run?
Well now my new samples fail at OpenDocument for some reason but I added extra steps which BrotliDecode should pass so we can keep using the existing tests even after we fix the issues.
Fixed the issue (turns out my pdfs were broken earlier but they should work now).
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 4 comments![]( "Open in Gerrit")
if (pdf_enable_brotli) {nit: Move above line 141 to keep the list sorted.
pdf_enable_brotli = falseCL description should mention this.
pdf_enable_brotli = falseTry to keep the list somewhat sorted. Move above `pdf_enable_click_logging`.
Lei ZhangDone
Above, not below.
Aryan KrishnanWhere exactly are these files from?
Lei ZhangFrom the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
Aryan KrishnanIt's a bit vague. Assuming this is https://pdfa.org/download-area/examples/Brotli-Prototype-PDFs.zip, add that to the commit message to be more specific?
Lei ZhangAdded the link.
Aryan KrishnanThanks. It's unclear what license those files are released under. So instead of using PDFA's example, how about creating one from scratch, similar to testing/resources/utf-8.in. Let me know if you need help.
Aryan KrishnanWill do, but it might take me a while to get it done.
Aryan KrishnanDone
Not quite sure if it would really work and it is kind of hard to tell if the OpenDocument() failure is from Brotli or from something else with the strutcure of the PDF.
I can't actually view the .in files in the web review tool. I suspect it's because the files have binary data. Consider chaining `/ASCII85Decode` to make the content streams ASCII. This is similar to what testing/tools/encode_pdf_filter.py produces, but with `/BrotliDecode` instead of `/FlateDecode`.
Related details
- Andy Phan
- Aryan Krishnan
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 3 comments![]( "Open in Gerrit")
nit: Move above line 141 to keep the list sorted.
Done
CL description should mention this.
Wait why is this here?
The previous CL already does that.
Removed.
pdf_enable_brotli = falseAryan KrishnanTry to keep the list somewhat sorted. Move above `pdf_enable_click_logging`.
Lei ZhangDone
Above, not below.
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan added 1 comment![]( "Open in Gerrit")
Aryan KrishnanWhere exactly are these files from?
Lei ZhangFrom the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
Aryan KrishnanIt's a bit vague. Assuming this is https://pdfa.org/download-area/examples/Brotli-Prototype-PDFs.zip, add that to the commit message to be more specific?
Lei ZhangAdded the link.
Aryan KrishnanThanks. It's unclear what license those files are released under. So instead of using PDFA's example, how about creating one from scratch, similar to testing/resources/utf-8.in. Let me know if you need help.
Aryan KrishnanWill do, but it might take me a while to get it done.
Aryan KrishnanDone
Lei ZhangNot quite sure if it would really work and it is kind of hard to tell if the OpenDocument() failure is from Brotli or from something else with the strutcure of the PDF.
I can't actually view the .in files in the web review tool. I suspect it's because the files have binary data. Consider chaining `/ASCII85Decode` to make the content streams ASCII. This is similar to what testing/tools/encode_pdf_filter.py produces, but with `/BrotliDecode` instead of `/FlateDecode`.
Encode_pdf_filter.py is helpful. I can just hotwire the script a bit to work with BrotliDecode on my local fork to help make the PDFs.
Will work on it.
Aryan Krishnan (Gerrit)
Aryan Krishnan added 2 comments![]( "Open in Gerrit")
Aryan KrishnanWhere exactly are these files from?
Lei ZhangFrom the PDF Association: (as per commit msg)
https://pdfa.org/brotli-compression-coming-to-pdf/#public-prototype-implementation
Aryan KrishnanIt's a bit vague. Assuming this is https://pdfa.org/download-area/examples/Brotli-Prototype-PDFs.zip, add that to the commit message to be more specific?
Lei ZhangAdded the link.
Aryan KrishnanThanks. It's unclear what license those files are released under. So instead of using PDFA's example, how about creating one from scratch, similar to testing/resources/utf-8.in. Let me know if you need help.
Aryan KrishnanWill do, but it might take me a while to get it done.
Aryan KrishnanDone
Lei ZhangNot quite sure if it would really work and it is kind of hard to tell if the OpenDocument() failure is from Brotli or from something else with the strutcure of the PDF.
Aryan KrishnanI can't actually view the .in files in the web review tool. I suspect it's because the files have binary data. Consider chaining `/ASCII85Decode` to make the content streams ASCII. This is similar to what testing/tools/encode_pdf_filter.py produces, but with `/BrotliDecode` instead of `/FlateDecode`.
Encode_pdf_filter.py is helpful. I can just hotwire the script a bit to work with BrotliDecode on my local fork to help make the PDFs.
Will work on it.
Nevermind decided to just make another python script to do that. But should be done now I think.
/Length /Length 92Why are there 2 lengths... Anyway.
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aryan Krishnan
| Commit-Queue | +1 |
1 comment
Why are there 2 lengths... Anyway.
Done
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aryan Krishnan (Gerrit)
Aryan Krishnan voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andy Phan
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |