bugGNU Octave - Bugs: bug #55780, [octave forge] (image) imresize...

 
 

bug #55780: [octave forge] (image) imresize with 'cubic' option is very wrong

Submitted by:  None
Submitted on:  Mon 25 Feb 2019 05:31:06 PM UTC  
 
Category:  Octave Forge Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Cris Luengo Originator Email:  -email is unavailable-
Open/Closed:  Closed Release:  dev
Operating System:  Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Mon 18 Mar 2019 02:57:41 PM UTC, comment #11:

Those changes had been made to the stable branch. I back them out but left them on the default branch. They will be aprt of the next minor release but not part of the next patch release.

Carnë Draug <carandraug>
Project Member
Mon 18 Mar 2019 05:41:13 AM UTC, comment #10:

Thanks Cris for fixing this bug.
I pushed your change in http://hg.code.sf.net/p/octave/image/rev/226a0e30ebc4,
and added a simple test in http://hg.code.sf.net/p/octave/image/rev/5fa9ceca562b,
as Carnë recommended.

Closing

Avinoam Kalma <avinoam>
Project Member
Mon 11 Mar 2019 11:38:01 AM UTC, comment #9:

Avinoam wrote:

> imremap was created in http://hg.code.sf.net/p/octave/image/rev/1a4b84759d2c, Jan 2007.
>
> Maybe interp2 did not support (bi)cubic interpolation.


I already checked and it did exist in core back in 2006. But it had a few FIXME notes on the source at the time, and maybe those affected its use for image. Anyway, if you make the change to fix, please add a simple test that only passes after the fix you apply.

Carnë Draug <carandraug>
Project Member
Fri 08 Mar 2019 04:55:43 PM UTC, comment #8:

imremap was created in http://hg.code.sf.net/p/octave/image/rev/1a4b84759d2c, Jan 2007.

Maybe interp2 did not support (bi)cubic interpolation.

Avinoam Kalma <avinoam>
Project Member
Tue 05 Mar 2019 11:33:45 AM UTC, comment #7:

> Carnë makes a good point, there should always be a comment when special-casing something.


I have come to find that a test for that special case works best. Comments can become out of date.

> Is the original author still around to explain this special case?


He's no longer involved with Octave. I would guess that he wouldn't remember why the special case anyway, it's been 15 years.

---

Since you're talking about cleaning up imremap, I would advise in investing your time in implementing imwarp instead. That would be matlab compatible. imremap is Octave only and is meant to do a very similar job. Eventually, imremap should be throw out and replaced with the Matlab compatible function, so you might as well do that now.

Carnë Draug <carandraug>
Project Member
Mon 04 Mar 2019 06:29:20 PM UTC, comment #6:

I've attached my modified version of the file `imremap.m`, I've tested it with the demos at the bottom, and I've modified one of the demos to show that the extrapolation value actually works correctly.

Cheers,
Cris.

Anonymous
Mon 04 Mar 2019 06:20:34 PM UTC, comment #5:

Sorry, preallocation should be:

Obviously I haven't tested any of the code I'm suggesting... :)

Cris.

Anonymous
Mon 04 Mar 2019 06:17:24 PM UTC, comment #4:

Carnë makes a good point, there should always be a comment when special-casing something. Is the original author still around to explain this special case?

If cleaning up `imremap` anyway, I suggest you also remove the redundant segment
This bit is repeated twice: once inside `grayinterp`, applied to each channel, and then again after the loop over the channels in `imremap` itself. Except that `grayinterp` is called with `extrapval=0` (is this a bug? Why not pass `extrapval`?).

Actually, it looks like `interp2` also should be called with `extrapval` instead of `0`, then the non-valid parts will be filled with the right value there. Does `interp2` really return NA values if passing `extrap=0`?

Ouch! There's no pre-allocation for `warped` either!

Looking at this code without the special case, I'd remove `grayinterp` altogether, it does nothing. I'd leave the relevant bit in `imremap` as follows:

When removing `graybicubic`, `bc` also becomes irrelevant (`sym_sub2ind` is removed in the patch). The functions `isgriddata` and `isequallyspaced` are never used in the current code, I would remove them in this patch too.

Cheers,
Cris Luengo.

Anonymous
Mon 04 Mar 2019 05:35:26 PM UTC, comment #3:

Would be nice to know why there was a special implementation for the cubic interpolation. This method already existed in core's interp2 back in 2006 when imremap was written. It may be working around a bug in core or maybe it's done for performance.

Anyway, we don't know why it was done so I guess it's fine to remove if it fixes a problem now. Can you add a minimal test case so that it will fail if someone tries to revert this in the future?

Carnë Draug <carandraug>
Project Member
Fri 01 Mar 2019 07:21:11 PM UTC, comment #2:

Thanks for reporting.

Please review the attached patch

(file #46386)

Avinoam Kalma <avinoam>
Project Member
Wed 27 Feb 2019 06:07:51 PM UTC, comment #1:

The problem seems to be from graybicubic call in grayinterp (imremap.m)

If we use interp2 in the case of cubic interpolation:

function [warped, valid] = grayinterp(im, XI, YI, interp, extrapval)
% if (strcmp(interp, "cubic"))
% warped = graybicubic(double(im), XI, YI, 0);
% else
warped = interp2(double(im), XI, YI, interp, 0);
% endif
valid = !isna(warped);
warped(!valid) = extrapval;
endfunction

The result looks better.

Avinoam Kalma <avinoam>
Project Member
Mon 25 Feb 2019 05:31:06 PM UTC, original submission:

When upscaling an image with imresize and the 'cubic' option, the output looks almost like a nearest neighbor interpolation, except it has smooth transitions from patch to patch. Cubic interpolation should yield a very smooth output.

The code below illustrates the problem. This is a small image, 16x16 pixels, being scaled 16x to 256x256 pixels.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #46415:  imremap.m added by None (4KiB - text/x-objcsrc - I've attached my modified version of the file `imremap.m`, I've tested it with the demos at the bottom, and I've modified one of the demos to show that the extrapolation value actually works correctly.)
file #46386:  imremap.diff added by avinoam (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by None (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-18 avinoam Open/ClosedOpen => Closed
    2019-03-18 avinoam StatusPatch Submitted => Fixed
    2019-03-04 None Attached File- => Added imremap.m, #46415
    2019-03-01 avinoam Attached File- => Added imremap.diff, #46386
        StatusConfirmed => Patch Submitted
    2019-02-27 avinoam StatusNone => Confirmed
        Release4.4.1 => dev
        Operating SystemGNU/Linux => Any
        Carbon-Copy- => Added hardy
        Carbon-Copy- => Added carandraug
    2019-02-25 mtmiller Summaryimresize with 'cubic' option is very wrong => [octave forge] (image) imresize with 'cubic' option is very wrong

    Back to the top


    Powered by Savane 3.4