upgrade openjpeg to commit# cf352af

upgrade openjpeg to commit# cf352af

BUG=457480, 497355
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/c212b684cb028a5d98e57f711c9eed931b853a44

[jun_fang, 2015-09-14 08:43:02]

jun_fang@foxitsoftware.com changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[jun_fang, 2015-09-14 08:43:02]

Hi Tom and Lei,

Please start to review this upgrade for openjpeg.

[Tom Sepez, 2015-09-14 15:53:41]

On 2015/09/14 08:43:02, jun_fang wrote:
> Hi Tom and Lei,
> 
> Please start to review this upgrade for openjpeg.

Looking at the existing 0000-use-colorspace.patch, that modifies only 2 files,
but the delta between PS#2 and PS#1 is three files (presumably correctly so). 
Did you apply the patch for issue 71 by hand?  It would be much better if the
patch were applied via the patch utility and a working .patch file.

Its probably about time that we had a README.pdfium file that detailed the patch
to be applied, and the issue it solved (link back to bug).

Otherwise, the updates are all from source control upstream, right?

[jun_fang, 2015-09-15 03:38:07]

On 2015/09/14 15:53:41, Tom Sepez wrote:
> On 2015/09/14 08:43:02, jun_fang wrote:
> > Hi Tom and Lei,
> > 
> > Please start to review this upgrade for openjpeg.
> 
> Looking at the existing 0000-use-colorspace.patch, that modifies only 2 files,
> but the delta between PS#2 and PS#1 is three files (presumably correctly so). 
> Did you apply the patch for issue 71 by hand?  It would be much better if the
> patch were applied via the patch utility and a working .patch file.
> 
> Its probably about time that we had a README.pdfium file that detailed the
patch
> to be applied, and the issue it solved (link back to bug).
> 
> Otherwise, the updates are all from source control upstream, right?

I applied the patch using a comparing and merging tool. The reason why the delta
between PS#2 and PS1 is one more file than the original patch is because I
deleted
the reference to a new variable in fx_codec_jpx_opj.cpp introduced in 
0000-use-colorspace.patch when I upgraded openjpeg. I need to recover this file
after I apply the original patch. When I recovered fx_codec_jpx_opj.cpp, a blank
line was removed. 
I didn't apply the patch via the patch utility because I got the following error
when I tried to apply it:
c:\Work\Code\pdfium\third_party\libopenjpeg20>git apply
0000-use-colorspace.patch
error: patch failed: third_party/libopenjpeg20/jp2.c:1429
error: third_party/libopenjpeg20/jp2.c: patch does not apply

[jun_fang, 2015-09-15 03:45:33]

On 2015/09/15 03:38:07, jun_fang wrote:
> On 2015/09/14 15:53:41, Tom Sepez wrote:
> > On 2015/09/14 08:43:02, jun_fang wrote:
> > > Hi Tom and Lei,
> > > 
> > > Please start to review this upgrade for openjpeg.
> > 
> > Looking at the existing 0000-use-colorspace.patch, that modifies only 2
files,
> > but the delta between PS#2 and PS#1 is three files (presumably correctly
so). 
> > Did you apply the patch for issue 71 by hand?  It would be much better if
the
> > patch were applied via the patch utility and a working .patch file.
> > 
> > Its probably about time that we had a README.pdfium file that detailed the
> patch
> > to be applied, and the issue it solved (link back to bug).
> > 
> > Otherwise, the updates are all from source control upstream, right?
> 
> I applied the patch using a comparing and merging tool. The reason why the
delta
> between PS#2 and PS1 is one more file than the original patch is because I
> deleted
> the reference to a new variable in fx_codec_jpx_opj.cpp introduced in 
> 0000-use-colorspace.patch when I upgraded openjpeg. I need to recover this
file
> after I apply the original patch. When I recovered fx_codec_jpx_opj.cpp, a
blank
> line was removed. 
> I didn't apply the patch via the patch utility because I got the following
error
> when I tried to apply it:
> c:\Work\Code\pdfium\third_party\libopenjpeg20>git apply
> 0000-use-colorspace.patch
> error: patch failed: third_party/libopenjpeg20/jp2.c:1429
> error: third_party/libopenjpeg20/jp2.c: patch does not apply

The possible reason of the failure of applying patch may be that jp2.c has been 
modified in the upgrade of jpeg2000. There is a conflicting so that it can't be
merged automatically.

[Tom Sepez, 2015-09-15 16:56:09]

> The possible reason of the failure of applying patch may be that jp2.c has
been 
> modified in the upgrade of jpeg2000. There is a conflicting so that it can't
be
> merged automatically.
Ah, OK.  These are the steps I would recommend in this situation:

0. Start clean, and make a new branch.
1. Retrieve the new version of the upstream code, and git commit (locally).
2. Make the changes manually that you want.
3. Produce a diff file that contains those changes (git diff), and commit only
it (locally).
4. Revert to the clean version of the code for the the modified files (git
checkout -- j2k.c etc.)
6. Apply the diff file (git apply)
7. commit locally the new modified files
8. Upload everything to code review as normal.

[Tom Sepez, 2015-09-15 16:57:51]

The idea, of course being that if we're lucky, merge conflicts will be rare
and subsequent updates will just patch automatically.

[jun_fang, 2015-09-29 13:23:43]

On 2015/09/15 16:57:51, Tom Sepez wrote:
> The idea, of course being that if we're lucky, merge conflicts will be rare
> and subsequent updates will just patch automatically.

Hi Tom and Lei,

Please review patch 4. I uploaded a patch and updated readme.

[Tom Sepez, 2015-09-29 15:12:28]

> Hi Tom and Lei,
> 
> Please review patch 4. I uploaded a patch and updated readme.

Rather than unifying the patches, I'd expect that you'd have added a set of
.patch files for each independent issue that you are correcting, something like:

0001-issue-name
0002-another-issue

The idea is that these may become fixed upstream at any time, so that the
ability to independently delete one without havin to disentangle it from the
others is helpful.  Thanks!

[jun_fang, 2015-10-08 12:25:08]

On 2015/09/29 15:12:28, Tom Sepez wrote:
> > Hi Tom and Lei,
> > 
> > Please review patch 4. I uploaded a patch and updated readme.
> 
> Rather than unifying the patches, I'd expect that you'd have added a set of
> .patch files for each independent issue that you are correcting, something
like:
> 
> 0001-issue-name
> 0002-another-issue
> 
> The idea is that these may become fixed upstream at any time, so that the
> ability to independently delete one without havin to disentangle it from the
> others is helpful.  Thanks!

I tried to create a set of .patch files for each independent issue. However,
there is a problem that every .patch will update the same file, README.pdfium.
It will introduce the conflicting. The index of patch will introduce the 
dependency too. Can you advise me on this problem? Thanks!

[Tom Sepez, 2015-10-08 18:18:38]

On 2015/10/08 12:25:08, jun_fang wrote:
> On 2015/09/29 15:12:28, Tom Sepez wrote:
> > > Hi Tom and Lei,
> > > 
> > > Please review patch 4. I uploaded a patch and updated readme.
> > 
> > Rather than unifying the patches, I'd expect that you'd have added a set of
> > .patch files for each independent issue that you are correcting, something
> like:
> > 
> > 0001-issue-name
> > 0002-another-issue
> > 
> > The idea is that these may become fixed upstream at any time, so that the
> > ability to independently delete one without havin to disentangle it from the
> > others is helpful.  Thanks!
> 
> I tried to create a set of .patch files for each independent issue. However,
> there is a problem that every .patch will update the same file, README.pdfium.
> It will introduce the conflicting. The index of patch will introduce the 
> dependency too. Can you advise me on this problem? Thanks!
No, the patch files shouldn't update readme.pdfium, that gets managed manually.
The patch file should only update the source code.
The indices of the patch e.g. 0000 show what order they should be applied in.
I'm not sure what else you're asking.

[jun_fang, 2015-10-09 03:05:17]

On 2015/10/08 18:18:38, Tom Sepez wrote:
> On 2015/10/08 12:25:08, jun_fang wrote:
> > On 2015/09/29 15:12:28, Tom Sepez wrote:
> > > > Hi Tom and Lei,
> > > > 
> > > > Please review patch 4. I uploaded a patch and updated readme.
> > > 
> > > Rather than unifying the patches, I'd expect that you'd have added a set
of
> > > .patch files for each independent issue that you are correcting, something
> > like:
> > > 
> > > 0001-issue-name
> > > 0002-another-issue
> > > 
> > > The idea is that these may become fixed upstream at any time, so that the
> > > ability to independently delete one without havin to disentangle it from
the
> > > others is helpful.  Thanks!
> > 
> > I tried to create a set of .patch files for each independent issue. However,
> > there is a problem that every .patch will update the same file,
README.pdfium.
> > It will introduce the conflicting. The index of patch will introduce the 
> > dependency too. Can you advise me on this problem? Thanks!
> No, the patch files shouldn't update readme.pdfium, that gets managed
manually.
> The patch file should only update the source code.
> The indices of the patch e.g. 0000 show what order they should be applied in.
> I'm not sure what else you're asking.

It's fine if readme.pdfium is updated manually.
Let me ask another a question by the following example:
Assume that we have 3 patches:
0000-issue.patch
0001-issue.patch
0002-issue.patch

Someday, 0001-issue is fixed in the upstream, openjpeg.
In next upgrade of openjpeg, we need to remove the description
from readme.pdfium. In this case, shall we update the indices 
or just keep them?

[jun_fang, 2015-10-09 13:02:03]

On 2015/10/09 03:05:17, jun_fang wrote:
> On 2015/10/08 18:18:38, Tom Sepez wrote:
> > On 2015/10/08 12:25:08, jun_fang wrote:
> > > On 2015/09/29 15:12:28, Tom Sepez wrote:
> > > > > Hi Tom and Lei,
> > > > > 
> > > > > Please review patch 4. I uploaded a patch and updated readme.
> > > > 
> > > > Rather than unifying the patches, I'd expect that you'd have added a set
> of
> > > > .patch files for each independent issue that you are correcting,
something
> > > like:
> > > > 
> > > > 0001-issue-name
> > > > 0002-another-issue
> > > > 
> > > > The idea is that these may become fixed upstream at any time, so that
the
> > > > ability to independently delete one without havin to disentangle it from
> the
> > > > others is helpful.  Thanks!
> > > 
> > > I tried to create a set of .patch files for each independent issue.
However,
> > > there is a problem that every .patch will update the same file,
> README.pdfium.
> > > It will introduce the conflicting. The index of patch will introduce the 
> > > dependency too. Can you advise me on this problem? Thanks!
> > No, the patch files shouldn't update readme.pdfium, that gets managed
> manually.
> > The patch file should only update the source code.
> > The indices of the patch e.g. 0000 show what order they should be applied
in.
> > I'm not sure what else you're asking.
> 
> It's fine if readme.pdfium is updated manually.
> Let me ask another a question by the following example:
> Assume that we have 3 patches:
> 0000-issue.patch
> 0001-issue.patch
> 0002-issue.patch
> 
> Someday, 0001-issue is fixed in the upstream, openjpeg.
> In next upgrade of openjpeg, we need to remove the description
> from readme.pdfium. In this case, shall we update the indices 
> or just keep them?

please review patch 6.

[jun_fang, 2015-10-09 13:52:02]

https://codereview.chromium.org/1338973005/diff/100001/third_party/libopenjpe...
File third_party/libopenjpeg20/README.pdfium (right):

https://codereview.chromium.org/1338973005/diff/100001/third_party/libopenjpe...
third_party/libopenjpeg20/README.pdfium:14: 0002-packet-iterator.patch: Fix an
integer overflow issue in opj_pi_create_decode().
please let me know whether the length should be less than 80 chars even for
texts. If yes, I will update this description to follow this rule.

[Tom Sepez, 2015-10-09 15:50:53]

On 2015/10/09 13:52:02, jun_fang wrote:
>
https://codereview.chromium.org/1338973005/diff/100001/third_party/libopenjpe...
> File third_party/libopenjpeg20/README.pdfium (right):
> 
>
https://codereview.chromium.org/1338973005/diff/100001/third_party/libopenjpe...
> third_party/libopenjpeg20/README.pdfium:14: 0002-packet-iterator.patch: Fix an
> integer overflow issue in opj_pi_create_decode().
> please let me know whether the length should be less than 80 chars even for
> texts. If yes, I will update this description to follow this rule.
Yes, it should be 80 cols.  Also, the patch files in this directory should only
be updating the upstream files, not things that we own like
fpdfapi/fpdf_render/fpdf_render_loadimage.cpp. Git will take care of those for
us.

[jun_fang, 2015-10-10 09:49:22]

On 2015/10/09 15:50:53, Tom Sepez wrote:
> On 2015/10/09 13:52:02, jun_fang wrote:
> >
>
https://codereview.chromium.org/1338973005/diff/100001/third_party/libopenjpe...
> > File third_party/libopenjpeg20/README.pdfium (right):
> > 
> >
>
https://codereview.chromium.org/1338973005/diff/100001/third_party/libopenjpe...
> > third_party/libopenjpeg20/README.pdfium:14: 0002-packet-iterator.patch: Fix
an
> > integer overflow issue in opj_pi_create_decode().
> > please let me know whether the length should be less than 80 chars even for
> > texts. If yes, I will update this description to follow this rule.
> Yes, it should be 80 cols.  Also, the patch files in this directory should
only
> be updating the upstream files, not things that we own like
> fpdfapi/fpdf_render/fpdf_render_loadimage.cpp. Git will take care of those for
> us.

OK. Please see patch 7.

[Tom Sepez, 2015-10-12 15:48:00]

LGTM.  Confirmed that the patches line up with cf352af.

[jun_fang, 2015-10-13 07:29:46]

Committed patchset #7 (id:120001) manually as
c212b684cb028a5d98e57f711c9eed931b853a44 (presubmit successful).

[npm, 2017-09-01 17:27:40]

npm@chromium.org changed reviewers:
+ npm@chromium.org

[npm, 2017-09-01 17:27:41]

https://codereview.chromium.org/1338973005/diff/120001/third_party/libopenjpe...
File third_party/libopenjpeg20/dwt.c (right):

https://codereview.chromium.org/1338973005/diff/120001/third_party/libopenjpe...
third_party/libopenjpeg20/dwt.c:416: l_data_count =
opj_dwt_max_resolution(tilec->resolutions, tilec->numresolutions) *
(OPJ_UINT32)sizeof(OPJ_INT32);
This seems to be a mistake.