imread

[thomas.l.scofield]

I have modified imread to use GraphicsMagick libraries in place of
ImageMagick ones.  The version of imread I worked from is the one
in the latest stable(?) release from octave-forge, not the one
in development.  (I'm not yet admitted to the developers list.)

The main edits involved changing library and header file names
and locations.  I did this on a linux machine, and there it was
the case that the header file Magick++.h used in the C++ API for
ImageMagick was located in /usr/include/, subsidiary header files
were found in /usr/include/Magick++/, and the libraries were in
/usr/lib; the corresponding files for the C++ API for GraphicsMagick
were found in /usr/include/GraphicsMagick/ and /usr/lib.  I'm not
versed in how to make the corresponding changes work equally well
on other platforms.

I have dropped the exceptional handling of .jpg and .png files in
imread.m, so that GraphicsMagick takes care of importing all image
formats.  I ran a couple of tests while both implementations were
possible with code like

   octave:32> x = cputime(); im = imread("owl.png"); cputime()-x

and found that .png files seem to get loaded about twice as quickly
with GraphicsMagick as they were when handled separately; .jpg files
(older ones, I believe---I don't think I'm talking about jpeg2000
ones; has the extension changed for the newer format?), on the other
hand, appear to require about 25% more time to load than when handled
using the prior exceptional method.

To be clear, thus far I've only undertaken the task of making imread()
work using GraphicsMagick over Imagemagick which, in the opinion of
John Eaton, Soren Hauberg (and perhaps others), has a more stable API.
Besides that (and another minor change), imread() works as it did
before.  Perhaps the grafting of imread() into core Octave should,
right from the start, employ a more systematic look at whether the
function behaves in all respects like its Matlab counterpart.  That
is something I am willing to do, but have not yet.  I next will look
at imwrite(), which seems like it ought to go smoothly.

So, just how and to whom should I submit the changed files for review
by others?  As I said, I'm not yet on the developers list at
octave-forge.  I'm also not yet versed in the use of subversion nor
mercurial (any efforts I made to use mercurial were for a mac, and not
terribly successful; I presume it is easier from the linux machine I
am using right now).  Just what should I send?  People on the list
talk about changelogs and diffs (or, at least, they used to when CVS
was being used).  I get the impression that these diffs were not
produced from the unix command line as in

  $ diff file1 file2

Thanks.

Thomas Scofield

[Michael Goffioul-2]

On Tue, Jul 1, 2008 at 6:41 PM, Thomas Scofield <scofield@...> wrote:

> So, just how and to whom should I submit the changed files for review
> by others?  As I said, I'm not yet on the developers list at
> octave-forge.  I'm also not yet versed in the use of subversion nor
> mercurial (any efforts I made to use mercurial were for a mac, and not
> terribly successful; I presume it is easier from the linux machine I
> am using right now).  Just what should I send?  People on the list
> talk about changelogs and diffs (or, at least, they used to when CVS
> was being used).  I get the impression that these diffs were not
> produced from the unix command line as in
>
>  $ diff file1 file2

There are produced with mercurial (hg export)

Michael.

[Søren Hauberg]

tir, 01 07 2008 kl. 12:41 -0400, skrev Thomas Scofield:
> I have modified imread to use GraphicsMagick libraries in place of
> ImageMagick ones.  The version of imread I worked from is the one
> in the latest stable(?) release from octave-forge, not the one
> in development.  (I'm not yet admitted to the developers list.)

Great to see somebody take on this task :-)
I don't think there has been any changes to imread/write in a while so
it's probably fine to use the version in the latest 'image' package. But
even if you don't have a developers account for Octave Forge, you can do
a check out of the SVN -- you just can't check things in.

> So, just how and to whom should I submit the changed files for review
> by others?

If you're looking for code reviews, I think it's okay to simply post the
source code on this list. I don't think you're modifying any files in
Octave, so I'm guessing it's okay if you don't send a changeset at first
(when you have the final version ready, a changeset will probably be
required...)

Søren

[Søren Hauberg]

tir, 01 07 2008 kl. 17:00 -0400, skrev Thomas Scofield:

> I'm attaching the changed code as .m (imread.m) and .cc (__magick_read__.cc)
> files, which I hope is workable.  The way I compile __magick_read__.cc is
> with the command
>
>    mkoctfile __magick_read__.cc `GraphicsMagick++-config --cppflags` -lGraphicsMagick++ -lGraphicsMagick
>
> which presumes you have the GraphicsMagick libraries in the right places
> which, for me, was as specified in the note I sent earlier today starting
> this thread.  GraphicsMagick++-config is a script whose purpose I don't
> quite understand (it may not be necessary).  On my debian-based machine
> it came with GraphicsMagick, and parallels the script Magick++-config that
> came with one of the ImageMagick packages.  I include the script in the
> mkoctfile command above because in the Makefile for the image-1.0.6 package
> the ImageMagick version of the script appears in the same position.

I've only played with this for a couple of seconds for Octave 3.0.0, so
this'll be brief. I've tried

  I = imread ("some_pic.something");
  imshow (I)

and the result looks wrong with all the images I've tried. This might be
a bug in 'imshow', but I haven't had the time to investigate.

As to the code, I have a few comments:

  1) When I compile with '-Wall' I get a warning at line 94. Changing
line 22 from 'int i = mapsize;' to 'unsigned int i = mapsize;', fixes
this.

  2) The code formatting doesn't seem to match Octaves. Instead of
writing

for (...) {
    ...
}

please write

for (...)
  {
    ...
  }

and same for if statements, while loops, etc. If the loop body only
contains one line, then omit the curly brackets.

Instead of writing

for(...)

please write

for (...)

notice the extra space before the parenthesis. Same goes for if
statements, function calls, etc.

I'll report back when I know if my problems are related to 'imread' or
'imshow'.

Søren

[Jaroslav Hajek-2]

On Wed, Jul 2, 2008 at 9:46 PM, Thomas Scofield <scofield@...> wrote:

>
>
> On Wed, 2 Jul 2008, Søren Hauberg wrote:
>
>> tir, 01 07 2008 kl. 17:00 -0400, skrev Thomas Scofield:
>>>
>>> I'm attaching the changed code as .m (imread.m) and .cc
>>> (__magick_read__.cc)
>>> files, which I hope is workable.  The way I compile __magick_read__.cc is
>>> with the command
>>>
>>>   mkoctfile __magick_read__.cc `GraphicsMagick++-config --cppflags`
>>> -lGraphicsMagick++ -lGraphicsMagick
>>>
>>> which presumes you have the GraphicsMagick libraries in the right places
>>> which, for me, was as specified in the note I sent earlier today starting
>>> this thread.  GraphicsMagick++-config is a script whose purpose I don't
>>> quite understand (it may not be necessary).  On my debian-based machine
>>> it came with GraphicsMagick, and parallels the script Magick++-config
>>> that
>>> came with one of the ImageMagick packages.  I include the script in the
>>> mkoctfile command above because in the Makefile for the image-1.0.6
>>> package
>>> the ImageMagick version of the script appears in the same position.
>>
>> I've only played with this for a couple of seconds for Octave 3.0.0, so
>> this'll be brief. I've tried
>>
>>  I = imread ("some_pic.something");
>>  imshow (I)
>>
>> and the result looks wrong with all the images I've tried. This might be
>> a bug in 'imshow', but I haven't had the time to investigate.
>
> Can you say more about what you mean by "looks wrong"?  I have noticed
> that imshow(im) displays images as quite washed out on my linux
> machine---far
> different than if I use "display <imageFile>" or "gm display <imageFile>" to
> display the image from a terminal---but that the results are fairly
> comparable
> between the two methods on my mac.  But, then, it is on the linux machine
> that
> I have been making changes to imread().  When I re-install the image-1.0.6
> package from scratch (so as to return to unmodified code), the problem
> persists.  Does this sound like what you're experiencing?
>
>> As to the code, I have a few comments:
>>
>>  1) When I compile with '-Wall' I get a warning at line 94. Changing
>> line 22 from 'int i = mapsize;' to 'unsigned int i = mapsize;', fixes
>> this.
>>
>>  2) The code formatting doesn't seem to match Octaves. Instead of
>> writing
>>
>> for (...) {
>>   ...
>> }
>>
>> please write
>>
>> for (...)
>>  {
>>   ...
>>  }
>>
>> and same for if statements, while loops, etc. If the loop body only
>> contains one line, then omit the curly brackets.
>>
>> Instead of writing
>>
>> for(...)
>>
>> please write
>>
>> for (...)
>>
>> notice the extra space before the parenthesis. Same goes for if
>> statements, function calls, etc.
>>
>> I'll report back when I know if my problems are related to 'imread' or
>> 'imshow'.
>>
>> Søren
>
> These comments pertain to lines of code which were there before I began
> working on imread().  Nevertheless, reading through messages to
> octave-maintainers has made me aware that there are code conventions,
> at least for code that goes into core Octave, and I am happy to make
> such changes.  There was some discussion in April (perhaps other months
> too) of putting together a page of "octave contributing guidelines", but
> I've not found the result anywhere.  Do you know of the existence of a
> webpage or other document that summarizes coding conventions?
>
> Thomas Scofield
>

I had actually started with this; however, the problem with summarizing existing
practice was that I quickly ran out of inspiration. Since then I've
been working on other projects and I've almost forgotten about this.
Attached is what I've put together so far.
If more people think it's already big enough for a changeset I'll make
one and submit. Also, I'll welcome further suggestions.

regards,


--
RNDr. Jaroslav Hajek
computing expert
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz

[Thomas Weber-8]

Am Mittwoch, den 02.07.2008, 22:27 +0200 schrieb Jaroslav Hajek:
> Attached is what I've put together so far.
> If more people think it's already big enough for a changeset I'll make
> one and submit.

I'm for its inclusion.

        Thomas

[Søren Hauberg]

Quoting Søren Hauberg <soren@...>:
> I've only played with this for a couple of seconds for Octave 3.0.0, so
> this'll be brief. I've tried
>
>   I = imread ("some_pic.something");
>   imshow (I)
>
> and the result looks wrong with all the images I've tried. This might be
> a bug in 'imshow', but I haven't had the time to investigate.

Okay, I've tried the same thing on my linux machine at work, and it  
works just fine. So, I guess my gnuplot at home is broken. So, it  
seems your changes work just fine.

Søren

[John W. Eaton]

On Wed, Jul 2, 2008 at 9:46 PM, Thomas Scofield <scofield@...> wrote:

| These comments pertain to lines of code which were there before I began
| working on imread().  Nevertheless, reading through messages to
| octave-maintainers has made me aware that there are code conventions,
| at least for code that goes into core Octave, and I am happy to make
| such changes.

Are you working on that now, or should I check in your current version
of the code with the understanding that someone will clean it up later
(I could possibly do some of that as I review it).

jwe

[thomas.l.scofield]

Sorry about not including the entire list.

I'll re-submit what I already did but with indentation cleaned up, and do so today.

Thomas Scofield

On Jul 11, 2008, at 9:22 AM, John W. Eaton wrote:

On 11-Jul-2008, Thomas L. Scofield wrote:

| I've been away from this for several days, but consider this  

| something that still needs work.  I think the code I sent reads  

| images fine (passed a few tests I ran on it)---it's the things it  

| does afterward that I'm not completely understanding yet.  Thanks for  

| asking.

Are you planning to work on it in the next day or so?  If not, I will

take the code you posted and included it in Octave, now.  Then you can

submit patches to it from the Mercurial archive later.

| I never received an answer about how to read the .txi file that  

| Jaroslav Hajek sent.  If there is another way than just viewing it as  

| a text file, that would be helpful.

The file uses Texinfo markup, but lacks the header info necessary for

processing.  It is intended to be included as part of Octave's manual.

In the future, please respond on the list so that others may comment.

Thanks,

jwe

Thomas L. Scofield

--------------------------------------------------------

Associate Professor

Department of Mathematics and Statistics

Calvin College

--------------------------------------------------------

[thomas.l.scofield]

I'll soon be ready to submit modified versions of the files I sent before related to imread() so that now they (hopefully) follow Octave coding conventions.  The next question is how do you want me to do it.  Jaroslav Hajek's instructions were

  hg clone http://www.octave.org/hg/octave

  cd octave

  # change some sources...

  hg commit -m "make Octave the coolest software ever"

  hg export ../cool.diff

  # send ../cool.diff via email

Is this what you want me to do.  (I might as well figure out what to do now, I suppose.)  If so, where should these files go in the "directory tree" (they have not been part of Octave before)---this, I suppose, will influence where I record a changeset.

His instructions say "The ChangeLog entries should describe what is changed, not why.  The reason of the change should appear in the commit message."  Is the "commit message" something generated by mercurial, or is it a message I write and send to the maintainers list?

Thomas Scofield

On Jul 10, 2008, at 1:31 PM, John W. Eaton wrote:

On Wed, Jul 2, 2008 at 9:46 PM, Thomas Scofield <scofield@...> wrote:

| These comments pertain to lines of code which were there before I began

| working on imread().  Nevertheless, reading through messages to

| octave-maintainers has made me aware that there are code conventions,

| at least for code that goes into core Octave, and I am happy to make

| such changes.

Are you working on that now, or should I check in your current version

of the code with the understanding that someone will clean it up later

(I could possibly do some of that as I review it).

jwe

Thomas L. Scofield

--------------------------------------------------------

Associate Professor

Department of Mathematics and Statistics

Calvin College

--------------------------------------------------------

[John W. Eaton]

On 11-Jul-2008, Thomas L. Scofield wrote:

|
| I'll soon be ready to submit modified versions of the files I sent  
| before related to imread() so that now they (hopefully) follow Octave  
| coding conventions.  The next question is how do you want me to do  
| it.  Jaroslav Hajek's instructions were
|
|    hg clone http://www.octave.org/hg/octave
|    cd octave
|    # change some sources...
|    hg commit -m "make Octave the coolest software ever"
|    hg export ../cool.diff
|    # send ../cool.diff via email
|
| Is this what you want me to do.

Yes, submitting hg changesets is best.

| (I might as well figure out what to  
| do now, I suppose.)  If so, where should these files go in the  
| "directory tree" (they have not been part of Octave before)---this, I  
| suppose, will influence where I record a changeset.

The files that define the imread/imwrite functions probably belong in
the src/DLD-FUNCTIONS subdirectory.

| His instructions say "The ChangeLog entries should describe what is  
| changed, not why.  The reason of the change should appear in the  
| commit message."  Is the "commit message" something generated by  
| mercurial, or is it a message I write and send to the maintainers list?

I disagree.  The commit message is just a short ChangeLog type note.
If there are reasons for a change that are not obvious, that
information belongs in comments in the source code itself, where it is
more likely to be noticed and useful.

jwe

[thomas.l.scofield]

I know I could probably do a lot of reading (and certainly should!) to find the answer to my questions---I'm just sure someone out there can give me the basic idea much quicker.  So that you know, this is my first time doing shared program development, so I don't have any experience with CVS, svn, etc. to draw on.

I started with the commands

   hg clone http://www.octave.org/hg/octave

   hg clone octave octave-imread1

The files I've worked on thus far (and wish to submit their changes) were new to Octave; until now they have been part of the image package (in octave-forge).  After some fiddling, I decided that I should add these two files in their unaltered (octave-forge) form first.  I placed them in src/DLD_FUNCTIONS, then ran

   hg add <file1>

   hg add <file2>

I then replaced both of these files with my altered copies.  Then I ran

   hg diff       (actually produces something meaningful, because of the procedure I followed)

   hg ci

Now I run "hg export" (or "hg export -o imread.diff"), and get the message

   abort: export requires at least one changeset

What should I be doing differently?  Also, will "hg export", when run properly, be the last step in the process?  Or will I need to send an email to the maintainers list with a .diff file attached?

Thanks.

Thomas L. Scofield

--------------------------------------------------------

Associate Professor

Department of Mathematics and Statistics

Calvin College

--------------------------------------------------------

[John W. Eaton]

On 12-Jul-2008, Thomas L. Scofield wrote:

| Now I run "hg export" (or "hg export -o imread.diff"), and get the  
| message
|
|     abort: export requires at least one changeset
|
| What should I be doing differently?

  hg export tip > FILE

or

  hg export REVISION > FILE

if REVISION is not the latest changeset in your archive.  Then send
the resulting file to the list, preferably as a text/plain attachment.

jwe

[John W. Eaton]

On 12-Jul-2008, Thomas L. Scofield wrote:

|
| I know I could probably do a lot of reading (and certainly should!)  
| to find the answer to my questions---I'm just sure someone out there  
| can give me the basic idea much quicker.  So that you know, this is  
| my first time doing shared program development, so I don't have any  
| experience with CVS, svn, etc. to draw on.
|
| I started with the commands
|
|     hg clone http://www.octave.org/hg/octave
|     hg clone octave octave-imread1
|
| The files I've worked on thus far (and wish to submit their changes)  
| were new to Octave; until now they have been part of the image  
| package (in octave-forge).  After some fiddling, I decided that I  
| should add these two files in their unaltered (octave-forge) form  
| first.  I placed them in src/DLD_FUNCTIONS, then ran
|
|     hg add <file1>
|     hg add <file2>
|
| I then replaced both of these files with my altered copies.  Then I ran
|
|     hg diff       (actually produces something meaningful, because of  
| the procedure I followed)
|     hg ci
|
| Now I run "hg export" (or "hg export -o imread.diff"), and get the  

Your changeset made this way won't be useful since my copy doesn't
have the files yet and your diffs relative to the added files won't
apply to my sources.  I'd also need the changeset that adds the files
to the archive.

jwe