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

 
 

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

Submitter:  None
Submitted:  Mon 25 Feb 2019 05:31:06 PM UTC
   
 
Category:  Octave 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 Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 23 Jan 2020 01:26:48 PM UTC, comment #12: 

I have found now that the pushed fixed had broken the computation of "valid", the second output argument of imremap and imperspectivewrap, and the third output argument of imresize.

For whatever reason, the warnings during the tests were only triggered on Octave 4.2.1

I have pushed http://hg.code.sf.net/p/octave/image/rev/04ab95ae4750 to fix this

Carnë Draug <carandraug>
Group Member
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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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:

sz = size (im);
sz(1:2) = size (XI);
warped = zeros(sz);


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

  valid = !isna(warped);
  warped(!valid) = extrapval;

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:

  ## Interpolate
  sz = size (im);
  n_planes = prod (sz(3:end));
  warped = zeros(sz);     % PREALLOCATE!!!!
  for i = 1:n_planes
    warped(:,:,i) = interp2 (double(im(:,:,i)), XI, YI, interp, extrapval); % JUST CALL INTERP2 DIRECTLY HERE, AND USE EXTRAPVAL
  endfor

  ## we return image on same class as input
  warped = cast (warped, class (im));


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>
Group 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>
Group Member
Wed 27 Feb 2019 06:07:51 PM UTC, comment #1: 

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


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


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>
Group 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.


pkg load image
a = imread('https://i.stack.imgur.com/Au3YK.png');
b = imresize(a,16,'cubic');
imshow(b)


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)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group 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.13-f8d8.
    Corresponding source code