# HG changeset patch
# User Carnë Draug <carandraug@octave.org>
# Date 1373715719 -3600
# Node ID 997efb8d0b1935987ede77547d49413ec18ac768
# Parent  4660d047955e21ed170fcb8afba688ad3c6268c9
imread: implement options Index, Frames, and Info.

* imread.m: write documentation about options of param/key style.
* private/core_imread.m: write parsing of options in the param/key style.
  Implement the option for Index, Frames, and Info. Also write parsing
  for PixelRegion but comment out the code until it's implemented in
   __magick_read__(). Add comments about, and deprecate, Octave's native
  image format which is completely undocumented. Remove tests since they
  are duplicated from the ones in imread.m.
* __magick_read__.cc: move input check to the m file calling it. Rewrite
  parsing of options which are now in a non-optional struct (default values
  are now set on the m function).
* private/imageIO.m: fix call to other functions with varargout with
  multiple output values.

diff -r 4660d047955e -r 997efb8d0b19 libinterp/dldfcn/__magick_read__.cc
--- a/libinterp/dldfcn/__magick_read__.cc	Sat Jul 13 01:33:17 2013 +0100
+++ b/libinterp/dldfcn/__magick_read__.cc	Sat Jul 13 12:41:59 2013 +0100
@@ -1,5 +1,6 @@
 /*
 
+Copyright (C) 2013 Carnë Draug
 Copyright (C) 2002-2012 Andy Adler
 Copyright (C) 2008 Thomas L. Scofield
 Copyright (C) 2010 David Grundberg
@@ -49,16 +50,11 @@
 {
   octave_value_list output;
 
-  int rows = imvec[0].baseRows ();
-  int columns = imvec[0].baseColumns ();
-  int nframes = frameidx.length ();
+  const int rows    = imvec[0].baseRows ();
+  const int columns = imvec[0].baseColumns ();
+  const int nframes = frameidx.length ();
 
-  dim_vector idim = dim_vector ();
-  idim.resize (4);
-  idim(0) = rows;
-  idim(1) = columns;
-  idim(2) = 1;
-  idim(3) = nframes;
+  const dim_vector idim = dim_vector (rows, columns, 1, nframes);
 
   Array<int> idx (dim_vector (4, 1));
 
@@ -200,16 +196,11 @@
 
   T im;
 
-  int rows = imvec[0].baseRows ();
-  int columns = imvec[0].baseColumns ();
-  int nframes = frameidx.length ();
+  const int rows    = imvec[0].baseRows ();
+  const int columns = imvec[0].baseColumns ();
+  const int nframes = frameidx.length ();
 
-  dim_vector idim = dim_vector ();
-  idim.resize (4);
-  idim(0) = rows;
-  idim(1) = columns;
-  idim(2) = 1;
-  idim(3) = nframes;
+  dim_vector idim = dim_vector (rows, columns, 1, nframes);
 
   Magick::ImageType type = imvec[0].type ();
   const int divisor = ((uint64_t (1) << QuantumDepth) - 1) / 
@@ -217,8 +208,8 @@
 
   switch (type)
     {
-    case Magick::BilevelType:
-    case Magick::GrayscaleType:
+    case Magick::BilevelType:           // Monochrome bi-level image
+    case Magick::GrayscaleType:         // Grayscale image
       {
         im = T (idim);
         P *vec = im.fortran_vec ();
@@ -246,7 +237,7 @@
         }
       break;
 
-    case Magick::GrayscaleMatteType:
+    case Magick::GrayscaleMatteType:    // Grayscale image with opacity
       {
         idim(2) = 2;
         im = T (idim);
@@ -279,8 +270,8 @@
         }
       break;
 
-    case Magick::PaletteType:
-    case Magick::TrueColorType:
+    case Magick::PaletteType:           // Indexed color (palette) image
+    case Magick::TrueColorType:         // Truecolor image
       {
         idim(2) = 3;
         im = T (idim);
@@ -317,9 +308,9 @@
         }
       break;
 
-    case Magick::PaletteMatteType:
-    case Magick::TrueColorMatteType:
-    case Magick::ColorSeparationType:
+    case Magick::PaletteMatteType:      // Indexed color (palette) image with opacity
+    case Magick::TrueColorMatteType:    // Truecolor image with opacity
+    case Magick::ColorSeparationType:   // Cyan/Yellow/Magenta/Black (CYMK) image
       {
         idim(2) = 4;
         im = T (idim);
@@ -361,7 +352,7 @@
       break;
 
     default:
-      error ("__magick_read__: undefined ImageMagick image type");
+      error ("__magick_read__: undefined Magick++ image type");
       return retval;
     }
 
@@ -399,12 +390,13 @@
 
 DEFUN_DLD (__magick_read__, args, nargout,
   "-*- texinfo -*-\n\
-@deftypefn  {Loadable Function} {@var{m} =} __magick_read__ (@var{fname}, @var{index})\n\
-@deftypefnx {Loadable Function} {[@var{m}, @var{colormap}] =} __magick_read__ (@var{fname}, @var{index})\n\
-@deftypefnx {Loadable Function} {[@var{m}, @var{colormap}, @var{alpha}] =} __magick_read__ (@var{fname}, @var{index})\n\
-Read images with ImageMagick++.  In general you should not be using this\n\
-function.  Instead use @code{imread}.\n\
-@seealso{imread}\n\
+@deftypefn {Loadable Function} {[@var{img}, @var{map}, @var{alpha}] =} __magick_read__ (@var{fname}, @var{options})\n\
+Read image with GraphicsMagick or ImageMagick.\n\
+\n\
+This is a private internal function not intended for direct use.  Instead\n\
+use @code{imread}.\n\
+\n\
+@seealso{imfinfo, imformats, imread, imwrite}\n\
 @end deftypefn")
 {
   octave_value_list output;
@@ -415,34 +407,19 @@
 
   maybe_initialize_magick ();
 
-  if (args.length () > 3 || args.length () < 1 || ! args(0).is_string ()
-      || nargout > 3)
+  if (args.length () != 2 || ! args(0).is_string ())
     {
       print_usage ();
       return output;
     }
 
-  Array<int> frameidx;
-  bool all_frames = false;
-
-  if (args.length () == 2 && args(1).is_real_type ())
-    frameidx = args(1).int_vector_value ();
-  else if (args.length () == 3 && args(1).is_string ()
-           && args(1).string_value () == "frames")
+  octave_map options = args(1).map_value ();
+  if (error_state)
     {
-      if (args(2).is_string () && args(2).string_value () == "all")
-        all_frames = true;
-      else if (args(2).is_real_type ())
-        frameidx = args(2).int_vector_value ();
-    }
-  else
-    {
-      frameidx = Array<int> (dim_vector (1, 1));
-      frameidx(0) = 1;
+      error ("__magick_read__: OPTIONS must be a struct");
     }
 
   std::vector<Magick::Image> imvec;
-
   try
     {
       // Read a file into vector of image objects
@@ -454,6 +431,8 @@
     }
   catch (Magick::ErrorCoder& e)
     {
+      // FIXME: there's a WarningCoder and ErrorCoder. Shouldn't this
+      // exception cause an error?
       warning ("Magick++ coder error: %s", e.what ());
     }
   catch (Magick::Exception& e)
@@ -462,22 +441,34 @@
       return output;
     }
 
-  int nframes = imvec.size ();
-  if (all_frames)
+  const int nframes = imvec.size ();
+  Array<int> frameidx;
+
+  octave_value indexes = options.getfield ("index")(0);
+  if (indexes.is_string () && indexes.string_value () == "all")
     {
       frameidx = Array<int> (dim_vector (1, nframes));
-      for (int i = 0; i < frameidx.length (); i++)
-        frameidx(i) = i;
+      for (int i = 0; i < nframes; i++)
+        {
+          frameidx(i) = i;
+        }
     }
   else
     {
-      for (int i = 0; i < frameidx.length (); i++)
+      frameidx = indexes.int_vector_value ();
+      if (error_state)
         {
-          frameidx(i) = frameidx(i) - 1;
-
-          if (frameidx(i) >= nframes || frameidx(i) < 0)
+          error ("__magick_read__: invalid value for Index/Frame");
+        }
+      // Fix indexex from base 1 to base 0, and at the same time, make
+      // sure none of the indexes is outside the range of image number.
+      int n = frameidx.nelem ();
+      for (int i = 0; i < n; i++)
+        {
+          frameidx(i)--;
+          if (frameidx(i) < 0 || frameidx(i) > nframes - 1)
             {
-              error ("__magick_read__: invalid INDEX vector");
+              error ("imread: index/frames specified are outside the number of images");
               return output;
             }
         }
@@ -485,8 +476,14 @@
 
   Magick::ClassType klass = imvec[0].classType ();
 
+  // PseudoClass:
+  // Image is composed of pixels which specify an index in a color palette.
   if (klass == Magick::PseudoClass && nargout > 1)
-    output = read_indexed_images (imvec, frameidx, (nargout == 3));
+    {
+      output = read_indexed_images (imvec, frameidx, (nargout == 3));
+    }
+  // If not PseudoClass then it must be DirectClass: Image is composed of
+  // pixels which represent literal color values.
   else
     {
       unsigned int depth = imvec[0].modulusDepth ();
@@ -867,9 +864,12 @@
   "-*- texinfo -*-\n\
 @deftypefn  {Loadable Function} {} __magick_write__ (@var{fname}, @var{fmt}, @var{img})\n\
 @deftypefnx {Loadable Function} {} __magick_write__ (@var{fname}, @var{fmt}, @var{img}, @var{map})\n\
-Write images with ImageMagick++.  In general you should not be using this\n\
-function.  Instead use @code{imwrite}.\n\
-@seealso{imread}\n\
+Write image with GraphicsMagick or ImageMagick.\n\
+\n\
+This is a private internal function not intended for direct use.  Instead\n\
+use @code{imwrite}.\n\
+\n\
+@seealso{imfinfo, imformats, imread, imwrite}\n\
 @end deftypefn")
 {
   octave_value_list retval;
@@ -1001,9 +1001,12 @@
 DEFUN_DLD (__magick_finfo__, args, ,
   "-*- texinfo -*-\n\
 @deftypefn {Loadable Function} {} __magick_finfo__ (@var{fname})\n\
-Read image information with GraphicsMagick++.  In general you should\n\
-not be using this function.  Instead use @code{imfinfo}.\n\
-@seealso{imfinfo, imread}\n\
+Read image information with GraphicsMagick or ImageMagick.\n\
+\n\
+This is a private internal function not intended for direct use.  Instead\n\
+use @code{imfinfo}.\n\
+\n\
+@seealso{imfinfo, imformats, imread, imwrite}\n\
 @end deftypefn")
 {
   octave_value retval;
@@ -1145,6 +1148,8 @@
   "-*- texinfo -*-\n\
 @deftypefn {Loadable Function} {} __magick_imformats__ (@var{formats})\n\
 Fill formats info with GraphicsMagick CoderInfo.\n\
+\n\
+@seealso{imfinfo, imformats, imread, imwrite}\n\
 @end deftypefn")
 {
   octave_value retval;
diff -r 4660d047955e -r 997efb8d0b19 scripts/image/imread.m
--- a/scripts/image/imread.m	Sat Jul 13 01:33:17 2013 +0100
+++ b/scripts/image/imread.m	Sat Jul 13 12:41:59 2013 +0100
@@ -1,3 +1,4 @@
+## Copyright (C) 2013 Carnë Draug
 ## Copyright (C) 2008-2012 Thomas L. Scofield
 ## Copyright (C) 2008 Kristian Rumberg
 ## Copyright (C) 2006 Thomas Weber
@@ -25,6 +26,7 @@
 ## @deftypefnx {Function File} {[@dots{}] =} imread (@var{filename}, @var{ext})
 ## @deftypefnx {Function File} {[@dots{}] =} imread (@var{url})
 ## @deftypefnx {Function File} {[@dots{}] =} imread (@dots{}, @var{idx})
+## @deftypefnx {Function File} {[@dots{}] =} imread (@dots{}, @var{param1}, @var{val1}, @dots{})
 ## Read images from various file formats.
 ##
 ## Reads an image as a matrix from the file @var{filename}.  If there is
@@ -47,9 +49,25 @@
 ## specifying the index of the images to read.  By default, Octave
 ## will only read the first page.
 ##
+## Depending on the file format, it is possible to configure the reading
+## of images with @var{param}, @var{val} pairs.  The following options
+## are supported:
+##
+## @table @asis
+## @item Frames or Index
+## This is an alternative method to specify @var{idx}.  When specifying it
+## in this way, its value can also be the string "all".
+##
+## @item Info
+## This option exists for @sc{Matlab} compatibility and has no effect.  For
+## maximum performance while reading multiple images from a single file,
+## use the Index option.
+## @end table
+##
 ## @seealso{imwrite, imfinfo, imformats}
 ## @end deftypefn
 
+## Author: Carnë Draug <carandraug@octave.org>
 ## Author: Thomas L. Scofield <scofield@calvin.edu>
 ## Author: Kristian Rumberg <kristianrumberg@gmail.com>
 ## Author: Thomas Weber <thomas.weber.mail@gmail.com>
@@ -72,7 +90,7 @@
     filename{2} = varargin{2};
   endif
 
-  varargout{1:nargout} = imageIO (@core_imread, "read", filename, varargin{:});
+  [varargout{1:nargout}] = imageIO (@core_imread, "read", filename, varargin{:});
 endfunction
 
 %!testif HAVE_MAGICK
diff -r 4660d047955e -r 997efb8d0b19 scripts/image/private/core_imread.m
--- a/scripts/image/private/core_imread.m	Sat Jul 13 01:33:17 2013 +0100
+++ b/scripts/image/private/core_imread.m	Sat Jul 13 12:41:59 2013 +0100
@@ -41,32 +41,112 @@
     error ("imread: FILENAME must be a string");
   endif
 
+  ## keep track of the varargin offset we're looking at each moment
+  offset    = 1;
+
   filename  = tilde_expand (filename);
   fn        = file_in_path (IMAGE_PATH, filename);
-  if (isempty (fn) && nargin >= 2 && ischar (varargin{1}))
+  if (isempty (fn) && nargin >= offset + 1 && ischar (varargin{offset}))
     ## if we can't find the file, check if the next input is the file extension
-    filename  = [filename "." varargin{1}];
+    filename  = [filename "." varargin{offset}];
     fn        = file_in_path (IMAGE_PATH, filename);
+    offset++;
   endif
   if (isempty (fn))
     error ("imread: cannot find %s", filename);
   endif
 
+  ## set default for options
+  options = struct ("index", 1);
+
+  ## Index is the only option that can be defined without the parameter/value
+  ## pair style. When defining it here, the string "all" is invalid though.
+  if (nargin >= offset + 1 && ! ischar (varargin{offset}))
+    if (! is_valid_index_option (options.index))
+      error ("imread: IDX must be a numeric vector");
+    endif
+    options.index = varargin{offset};
+    offset++;
+  endif
+
+  if (rem (numel (varargin) - offset + 1, 2) != 0)
+    error ("imread: no pair for all arguments (even number left)");
+  endif
+
+  for idx = offset:2:(numel (varargin) - offset + 1)
+
+    switch (tolower (varargin{idx}))
+
+      case {"frames", "index"},
+        options.index = varargin{idx+1};
+        if (! (is_valid_index_option (options.index)) &&
+            ! (ischar (options.index) && strcmpi (options.index, "all")))
+          error ("imread: value for %s must be a vector or the string `all'");
+        endif
+
+## FIXME: commented until it's implemented in __magick_read__
+##      case "pixelregion",
+##        options.region = varargin{idx+1};
+##        if (! iscell (options.region) || numel (options.region) != 2)
+##          error ("imread: value for %s must be a 2 element cell array",
+##                 varargin{idx});
+##        endif
+##        for reg_idx = 1:2
+##          if (numel (options.region{reg_idx}) == 3)
+##            ## do nothing
+##          elseif (numel (options.region{reg_idx}) == 2)
+##            options.region{reg_idx}(3) = options.region{reg_idx}(2);
+##            options.region{reg_idx}(2) = 1;
+##          else
+##            error ("imread: range for %s must be a 2 or 3 element vector",
+##                   varargin{idx});
+##          endif
+##          options.region{reg_idx} = floor (options.region{reg_idx}(1)): ...
+##                                    floor (options.region{reg_idx}(2)): ...
+##                                    floor (options.region{reg_idx}(3));
+##        endfor
+
+      case "info",
+        ## We ignore this option. This parameter exists in Matlab to
+        ## speed up the reading of multipage TIFF.  It makes no difference
+        ## for us since we're already quite efficient.
+
+      otherwise
+        error ("imread: invalid PARAMETER `%s'", varargin{idx});
+
+    endswitch
+  endfor
+
   try
-    [varargout{1:nargout}] = __magick_read__ (fn, varargin{:});
+    [varargout{1:nargout}] = __magick_read__ (fn, options);
+
   catch
+    ## If we can't read it with Magick, maybe the image is in Octave's
+    ## native image format.  This is from back before Octave had 'imread'
+    ## and 'imwrite'. Then we had the functions 'loadimage' and 'saveimage'.
+    ##
+    ## This "image format" seems to be any file that can be read with
+    ## load() and contains 2 variables.  The variable named "map" is a
+    ## colormap and must exist whether the image is indexed or not. The
+    ## other variable must be named "img" or "X" for a "normal" or
+    ## indexed image.
+    ##
+    ## FIXME: this has been deprecated for the next major release (3.8 or 4.0).
+    ##        If someone wants to revive this as yet another image format, a
+    ##        separate Octave package can be written for it, that register the
+    ##        format through imformats.
 
     magick_error = lasterr ();
 
     img_field = false;
-    x_field = false;
+    x_field   = false;
     map_field = false;
 
     try
       vars = load (fn);
       if (isstruct (vars))
         img_field = isfield (vars, "img");
-        x_field = isfield (vars, "X");
+        x_field   = isfield (vars, "X");
         map_field = isfield (vars, "map");
       endif
     catch
@@ -80,6 +160,11 @@
       else
         varargout{1} = vars.X;
       endif
+      persistent warned = false;
+      if (! warned)
+        warning ("Octave's native image format has been deprecated.");
+        warned = true;
+      endif
     else
       error ("imread: invalid Octave image file format");
     endif
@@ -88,31 +173,13 @@
 
 endfunction
 
-
-%!testif HAVE_MAGICK
-%! vpng = [ ...
-%!  137,  80,  78,  71,  13,  10,  26,  10,   0,   0, ...
-%!    0,  13,  73,  72,  68,  82,   0,   0,   0,   3, ...
-%!    0,   0,   0,   3,   8,   2,   0,   0,   0, 217, ...
-%!   74,  34, 232,   0,   0,   0,   1, 115,  82,  71, ...
-%!   66,   0, 174, 206,  28, 233,   0,   0,   0,   4, ...
-%!  103,  65,  77,  65,   0,   0, 177, 143,  11, 252, ...
-%!   97,   5,   0,   0,   0,  32,  99,  72,  82,  77, ...
-%!    0,   0, 122,  38,   0,   0, 128, 132,   0,   0, ...
-%!  250,   0,   0,   0, 128, 232,   0,   0, 117,  48, ...
-%!    0,   0, 234,  96,   0,   0,  58, 152,   0,   0, ...
-%!   23, 112, 156, 186,  81,  60,   0,   0,   0,  25, ...
-%!   73,  68,  65,  84,  24,  87,  99,  96,  96,  96, ...
-%!  248, 255, 255,  63, 144,   4,  81, 111, 101,  84, ...
-%!   16,  28, 160,  16,   0, 197, 214,  13,  34,  74, ...
-%!  117, 213,  17,   0,   0,   0,   0,  73,  69,  78, ...
-%!   68, 174,  66,  96, 130];
-%! fid = fopen ("test.png", "wb");
-%! fwrite (fid, vpng);
-%! fclose (fid);
-%! A = imread ("test.png");
-%! delete ("test.png");
-%! assert (A(:,:,1), uint8 ([0, 255, 0; 255, 237, 255; 0, 255, 0]));
-%! assert (A(:,:,2), uint8 ([0, 255, 0; 255,  28, 255; 0, 255, 0]));
-%! assert (A(:,:,3), uint8 ([0, 255, 0; 255,  36, 255; 0, 255, 0]));
-
+## Tests if the value passed to the Index or Frames is valid. This option
+## can be defined in two places, but only in one place can it also be the
+## string "all"
+function bool = is_valid_index_option (arg)
+  ## is the index option
+  bool = false;
+  if (isvector (arg) && isnumeric (arg) && isreal (arg))
+    bool = true;
+  endif
+endfunction
diff -r 4660d047955e -r 997efb8d0b19 scripts/image/private/imageIO.m
--- a/scripts/image/private/imageIO.m	Sat Jul 13 01:33:17 2013 +0100
+++ b/scripts/image/private/imageIO.m	Sat Jul 13 12:41:59 2013 +0100
@@ -61,8 +61,8 @@
   ## When there is no match, fmt will be a 1x1 structure with no fields,
   ## so we can't just use `isempty (fmt)'.
   if (isempty (fieldnames (fmt)))
-    varargout{1:nargout} = core_func (varargin{:});
+    [varargout{1:nargout}] = core_func (varargin{:});
   else
-    varargout{1:nargout} = fmt.(fieldname) (varargin{:});
+    [varargout{1:nargout}] = fmt.(fieldname) (varargin{:});
   endif
 endfunction