scene and number_scenes ignored by ImageMagick 6.5.4-10

[snaury]

I've been using RMagick with ImageMagick 6.5.1 and I've been using BlobToImage (via ImageList#from_blob) with setting info.scene and info.number_scenes (usually just to 0 and 1 respectively). This worked find, but as soon as I tried upgrading to ImageMagick 6.5.4 it stopped working, and I was getting many images instead of just one (which, for a psd with 22 layers takes A LOT MORE time than I'd like). I traced it to CloneImageInfo, where scene and number_scenes are blatantly overwritten by DEPRECATED subimage and subrange (it seems that RMagick believed in this deprecation and does not allow setting neither subimage or subrange, scene and number_scenes are the only members exposed to ruby). I think that this should either be the other way around (subimage and subrange is set from scene and number_scenes), or like this:

Code: Select all

diff --git a/magick/image.c b/magick/image.c
index 1abd8ac..fe71f8c 100644
--- a/magick/image.c
+++ b/magick/image.c
@@ -1186,8 +1186,8 @@ MagickExport ImageInfo *CloneImageInfo(const ImageInfo *image_info)
   clone_info->temporary=image_info->temporary;
   clone_info->adjoin=image_info->adjoin;
   clone_info->antialias=image_info->antialias;
-  clone_info->scene=image_info->subimage;
-  clone_info->number_scenes=image_info->subrange;
+  clone_info->scene=image_info->scene;
+  clone_info->number_scenes=image_info->number_scenes;
   clone_info->depth=image_info->depth;
   if (image_info->size != (char *) NULL)
     (void) CloneString(&clone_info->size,image_info->size);

Or, if the intention was to support legacy code, then maybe it should be something like this:

Code: Select all

diff --git a/magick/image.c b/magick/image.c
index 1abd8ac..41e4529 100644
--- a/magick/image.c
+++ b/magick/image.c
@@ -1186,8 +1186,16 @@ MagickExport ImageInfo *CloneImageInfo(const ImageInfo *image_info)
   clone_info->temporary=image_info->temporary;
   clone_info->adjoin=image_info->adjoin;
   clone_info->antialias=image_info->antialias;
-  clone_info->scene=image_info->subimage;
-  clone_info->number_scenes=image_info->subrange;
+  if (image_info->scene == 0 && image_info->number_scenes == 0)
+    {
+      clone_info->scene=image_info->subimage;
+      clone_info->number_scenes=image_info->subrange;
+    }
+  else
+    {
+      clone_info->scene=image_info->scene;
+      clone_info->number_scenes=image_info->number_scenes;
+    }
   clone_info->depth=image_info->depth;
   if (image_info->size != (char *) NULL)
     (void) CloneString(&clone_info->size,image_info->size);

Now subimage and subrange would only be used when scene and number_scenes is not set to something explicitly.

Please fix this, because as it is in 6.5.4-10, the code that actually uses new members is punished, while code that uses deprecated subimage/subrange is unaffected.

[magick]

We can reproduce the problem you posted and will have a patch in the Subversion trunk by tomorrow. Thanks.

[snaury]

Just a minor correction, subimage and subrange are assigned at the end of CloneImageInfo, which overwrite what you assigned before, and it should be this:

Code: Select all

diff --git a/magick/image.c b/magick/image.c
index eb94efe..da99928 100644
--- a/magick/image.c
+++ b/magick/image.c
@@ -1186,8 +1186,6 @@ MagickExport ImageInfo *CloneImageInfo(const ImageInfo *image_info)
   clone_info->antialias=image_info->antialias;
   clone_info->scene=image_info->scene;
   clone_info->number_scenes=image_info->number_scenes;
-  clone_info->subimage=image_info->scene;  /* deprecated */
-  clone_info->subrange=image_info->number_scenes;  /* deprecated */
   clone_info->depth=image_info->depth;
   if (image_info->size != (char *) NULL)
     (void) CloneString(&clone_info->size,image_info->size);
@@ -1251,8 +1249,8 @@ MagickExport ImageInfo *CloneImageInfo(const ImageInfo *image_info)
   (void) CopyMagickString(clone_info->zero,image_info->zero,MaxTextExtent);
   (void) CopyMagickString(clone_info->filename,image_info->filename,
     MaxTextExtent);
-  clone_info->subimage=image_info->subimage;
-  clone_info->subrange=image_info->subrange;
+  clone_info->subimage=image_info->scene;  /* deprecated */
+  clone_info->subrange=image_info->number_scenes;  /* deprecated */
   clone_info->channel=image_info->channel;
   clone_info->debug=IsEventLogging();
   clone_info->signature=image_info->signature;

Also, Magick++ has only subImage and subRange accessors, and doesn't know anything about scene/number_scenes, it needs some fixing to work correctly with these changes:

Code: Select all

diff --git a/Magick++/lib/Options.cpp b/Magick++/lib/Options.cpp
index ea68902..5bae6c6 100644
--- a/Magick++/lib/Options.cpp
+++ b/Magick++/lib/Options.cpp
@@ -607,20 +607,20 @@ double Magick::Options::strokeWidth ( void ) const
 
 void Magick::Options::subImage ( unsigned int subImage_ )
 {
-  _imageInfo->subimage = subImage_;
+  _imageInfo->scene = subImage_;
 }
 unsigned int Magick::Options::subImage ( void ) const
 {
-  return _imageInfo->subimage;
+  return _imageInfo->scene;
 }
 
 void Magick::Options::subRange ( unsigned int subRange_ )
 {
-  _imageInfo->subrange = subRange_;
+  _imageInfo->number_scenes = subRange_;
 }
 unsigned int Magick::Options::subRange ( void ) const
 {
-  return _imageInfo->subrange;
+  return _imageInfo->number_scenes;
 }
 
 // Annotation text encoding (e.g. "UTF-16")

Now there are only two places where ImageMagick uses deprecated subrange field. It's in coders/avi.c and coders/jpeg.c, in both cases it's for the lines like these:

Code: Select all

jpeg_info.scale_denom=(unsigned int) image_info->subrange;

Could you please look into what it really means? It seems fishy to me, especially for the case of avi files, where number_scenes would mean both, number of frames to extract and scale_denom for MJPG frames.

[magick]

Good work. We'll get your updates into the latest Subversion trunk by sometime tomorrow. Thanks.