bugGNU Octave - Bugs: bug #40669, rgb2ind: uses system ("gm...

 
 

bug #40669: rgb2ind: uses system ("gm convert....") for dither option

Submitted by:  Carnë Draug <carandraug>
Submitted on:  Fri 22 Nov 2013 01:30:05 PM UTC  
 
Category: Octave FunctionSeverity: 4 - Important
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating 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 Jump to the original submission)

Sun 24 Nov 2013 02:27:09 AM UTC, comment #7:

Reverted with cset a40156e8b6a7

http://hg.savannah.gnu.org/hgweb/octave/rev/a40156e8b6a7

Carnë Draug <carandraug>
Project Member
Fri 22 Nov 2013 06:37:43 PM UTC, comment #6:

The original patch that made this change is here, if it helps.

https://savannah.gnu.org/patch/?func=detailitem&item_id=8001#options

Adam H Aitkenhead <adama>
Fri 22 Nov 2013 05:16:51 PM UTC, comment #5:

We're out of time for the 3.8 release so I would revert the changesets as the fastest possible solution.

Rik <rik5>
Project Administrator
Fri 22 Nov 2013 03:24:58 PM UTC, comment #4:

As I mentioned, the missing options should not be very difficult to implement but I won't have the time to do it. If anyone wants to do this, it's basically finding the closest value in the colormap, and then distribute the difference (error) to the pixels ahead. To note that different algorithms exist, the only difference being how the error is distributed so a very interesting thing would be to have the internal function accept some sort of mask specifying how to perform the distribution.

Anyway, for the upcoming release, my opinion would be to revert those changes and not implement this options (unless someone picks up this and implements it on time).

Note that if we keep the current implementation, when we implement this again properly with Matlab compatibility, the results will be different.

I'm adding Adam H Aitkenhead to the mailing list of this bug since he was the author of the change and of bug #40664.

Carnë Draug <carandraug>
Project Member
Fri 22 Nov 2013 03:09:21 PM UTC, comment #3:

What do you think we can do about this for the imminent 3.8 release? I don't think the current situation is ideal to release with, right?

Mike Miller <mtmiller>
Project Administrator
Fri 22 Nov 2013 02:23:05 PM UTC, comment #2:

I would not recommend to make use of GM for the following reasons:

  • because of other issues when reading and writing images, I'm guessing we'll eventually have to replace GM with something else;
  • the Floyd and Steinberg algorithm is quite easy to implement, no need to add a dependency because of that;
  • the algorithm can be done in different ways. The diffusion of the error is usually done from left to right, and top to bottom but other variants do it from bottom to top. My previous attempt at it found that Matlab does not implement the standard one so depending on GM for this will not allow us to be Matlab compatible;
  • GM only does this for 8bit images;

Finally, dithering should not be implemented in rgb2ind, there's a missing function "dither" that rgb2ind should be calling. My guess (from trying to do this last year) is that for the complete rgb2ind to be implemented, the functions imapprox and dither need to be implemented first (each of the functions implementing different methods to perform the conversion).

Carnë Draug <carandraug>
Project Member
Fri 22 Nov 2013 02:02:35 PM UTC, comment #1:

Confirmed. I also don't have the gm executable on my system. But apparently "make check" doesn't exercise this code path because I haven't seen an error yet.

Yes it would be much preferable to reimplement with magick library calls, could be added to _magick_read_.cc. Otherwise this adds an extra runtime dependency that our distributors need to know about, and this is fairly hidden.

If we have to keep the current approach, it at least needs a runtime check for the executable and an appropriate error message if "gm" is not found in PATH. It also doesn't look to me like the temporary files being created are deleted at all.

Mike Miller <mtmiller>
Project Administrator
Fri 22 Nov 2013 01:30:05 PM UTC, original submission:

With cset 5d6243c2acbf, the option dither and others related to it were implemented. However, this is done by making a system call to "gm convert", one of the applications that may get installed with GraphicsMagick. But Octave is not dependent on this application, it's dependent on the Magick++ library and some users use ImageMagick instead.

The current implementation performs the following:
- saving image as a tiff file
- making a system call to "gm convert"
- read back the converted file

If a dependency on GraphicsMagick for this is desirable, then there's a map method http://www.graphicsmagick.org/Magick++/Image.html#map though if I remember correctly it was limited to 8bit images.

But the algorithm is very simple so that should not be necessary (although it will most likely be needed to be implemented as oct file as I don't see a way to vectorize it).

This is slightly related to bug #40664.

Carnë Draug <carandraug>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by adama (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by carandraug (author of the cset in question)
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by carandraug (Submitted the item)
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 24 Nov 2013 02:27:09 AM UTCcarandraugOpen/ClosedOpen=>Closed
    Sun 24 Nov 2013 02:27:08 AM UTCcarandraugStatusConfirmed=>Fixed
    Fri 22 Nov 2013 03:24:58 PM UTCcarandraugCarbon-Copy-=>Added adama
    Fri 22 Nov 2013 02:02:35 PM UTCmtmillerSeverity3 - Normal=>4 - Important
      Item GroupNone=>Incorrect Result
      StatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1