Fix bug where &crop=x1,y1,x2,y2 values are not adjusted for decoder p…

…re-shrinking
imazen · Jul 4, 2020 · 696fea2 · 696fea2 · github-actions · Jul 4, 2020
1 parent ae6f1bf
commit 696fea2
Show file tree

Hide file tree

Showing 17 changed files with 194 additions and 51 deletions.
diff --git a/imageflow_core/src/clients/stateless.rs b/imageflow_core/src/clients/stateless.rs
@@ -83,7 +83,7 @@ impl LibClient {
      fn get_image_info_inner(context: &mut Context, bytes: &[u8])
                           -> std::result::Result<s::ImageInfo, FlowError> {
         context.add_input_bytes(0, bytes).map_err(|e| e.at(here!()))?;
-        Ok(context.get_image_info(0).map_err(|e| e.at(here!()))?)
+        Ok(context.get_unscaled_image_info(0).map_err(|e| e.at(here!()))?)
 
     }
     pub fn get_image_info(&mut self, bytes: &[u8])

diff --git a/imageflow_core/src/codecs/gif/mod.rs b/imageflow_core/src/codecs/gif/mod.rs
@@ -92,8 +92,10 @@ impl Decoder for GifDecoder {
         Ok(())
     }
 
-
-    fn get_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
+    fn get_scaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo>{
+        self.get_unscaled_image_info(c)
+    }
+    fn get_unscaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
         Ok(s::ImageInfo {
             frame_decodes_into: s::PixelFormat::Bgra32,
             image_width: i32::from(self.reader.width()),

diff --git a/imageflow_core/src/codecs/jpeg_decoder.rs b/imageflow_core/src/codecs/jpeg_decoder.rs
@@ -45,8 +45,11 @@ impl Decoder for JpegDecoder {
         Ok(())
     }
 
+    fn get_scaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo>{
+        self.get_unscaled_image_info(c)
+    }
 
-    fn get_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
+    fn get_unscaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
 
         self.decoder.read_info()?;
         let info = self.decoder.info().expect("error handling not yet implemented for jpeg");
@@ -77,7 +80,7 @@ impl Decoder for JpegDecoder {
     fn read_frame(&mut self, c: &Context) -> Result<*mut BitmapBgra> {
 
         if self.width.is_none() {
-            let _ = self.get_image_info(c)?;
+            let _ = self.get_scaled_image_info(c)?;
         }
         let pixels = self.decoder.decode()?;
 

diff --git a/imageflow_core/src/codecs/libpng_decoder.rs b/imageflow_core/src/codecs/libpng_decoder.rs
@@ -38,7 +38,11 @@ impl Decoder for LibPngDecoder {
         Ok(())
     }
 
-    fn get_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
+    fn get_scaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo>{
+        self.get_unscaled_image_info(c)
+    }
+
+    fn get_unscaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
         let (w,h,fmt) = self.decoder.get_info()?;
 
         Ok(s::ImageInfo {

diff --git a/imageflow_core/src/codecs/mod.rs b/imageflow_core/src/codecs/mod.rs
@@ -36,7 +36,8 @@ pub trait DecoderFactory{
 }
 pub trait Decoder : Any{
     fn initialize(&mut self, c: &Context) -> Result<()>;
-    fn get_image_info(&mut self, c: &Context) -> Result<s::ImageInfo>;
+    fn get_unscaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo>;
+    fn get_scaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo>;
     fn get_exif_rotation_flag(&mut self, c: &Context) -> Result<Option<i32>>;
     fn tell_decoder(&mut self, c: &Context, tell: s::DecoderCommand) -> Result<()>;
     fn read_frame(&mut self, c: &Context) -> Result<*mut BitmapBgra>;

diff --git a/imageflow_core/src/codecs/mozjpeg_decoder.rs b/imageflow_core/src/codecs/mozjpeg_decoder.rs
@@ -40,7 +40,20 @@ impl Decoder for MozJpegDecoder {
         Ok(())
     }
 
-    fn get_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
+
+    fn get_unscaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
+        let (w,h) = self.decoder.get_original_size()?;
+
+        Ok(s::ImageInfo {
+            frame_decodes_into: s::PixelFormat::Bgr32,
+            image_width: w as i32,
+            image_height: h as i32,
+            preferred_mime_type: "image/jpeg".to_owned(),
+            preferred_extension: "jpg".to_owned()
+        })
+    }
+
+    fn get_scaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
         let (w,h) = self.decoder.get_final_size()?;
 
         Ok(s::ImageInfo {
@@ -231,6 +244,10 @@ impl MzDec{
         self.apply_downscaling();
         Ok((self.w, self.h))
     }
+    fn get_original_size(&mut self) -> Result<(u32,u32)>{
+        self.read_header()?;
+        Ok((self.original_width, self.original_height))
+    }
 
     fn get_exif_rotation_flag(&mut self) -> Result<Option<i32>>{
         self.read_header()?;

diff --git a/imageflow_core/src/codecs/webp.rs b/imageflow_core/src/codecs/webp.rs
@@ -36,7 +36,7 @@ impl WebPDecoder {
         })
     }
 
-    pub fn ensure_data_buffered(&mut self, c: &Context) -> Result<()>{
+    fn ensure_data_buffered(&mut self) -> Result<()>{
         if self.bytes.is_none() {
             let mut bytes = Vec::with_capacity(2048);
             let _ = self.io.read_to_end(&mut bytes).map_err(|e| FlowError::from_decoder(e));
@@ -73,17 +73,9 @@ impl WebPDecoder {
             self.input_height()
         }
     }
-}
-
-
-impl Decoder for WebPDecoder {
-    fn initialize(&mut self, c: &Context) -> Result<()> {
-        Ok(())
-    }
 
-
-    fn get_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
-        self.ensure_data_buffered(c)?;
+    fn ensure_features_read(&mut self) -> Result<()>{
+        self.ensure_data_buffered()?;
         if !self.features_read {
             let buf = &self.bytes.as_ref().unwrap(); //Cannot fail after ensure_data_buffered
             let len = buf.len();
@@ -95,6 +87,32 @@ impl Decoder for WebPDecoder {
             }
             self.features_read = true;
         }
+        Ok(())
+    }
+}
+
+
+impl Decoder for WebPDecoder {
+    fn initialize(&mut self, c: &Context) -> Result<()> {
+        Ok(())
+    }
+
+
+    fn get_scaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
+        self.ensure_features_read()?;
+
+        Ok(s::ImageInfo {
+            frame_decodes_into: s::PixelFormat::Bgra32,
+            image_width: self.output_width().unwrap(),
+            image_height: self.output_height().unwrap(),
+            preferred_mime_type: "image/webp".to_owned(),
+            preferred_extension: "webp".to_owned()
+        })
+    }
+
+    fn get_unscaled_image_info(&mut self, c: &Context) -> Result<s::ImageInfo> {
+        self.ensure_features_read()?;
+
         Ok(s::ImageInfo {
             frame_decodes_into: s::PixelFormat::Bgra32,
             image_width: self.input_width().unwrap(),
@@ -119,7 +137,7 @@ impl Decoder for WebPDecoder {
     }
 
     fn read_frame(&mut self, c: &Context) -> Result<*mut BitmapBgra> {
-        let _ = self.get_image_info(c)?;
+        let _ = self.get_scaled_image_info(c)?;
 
         unsafe {
             let w = self.output_width().unwrap();

diff --git a/imageflow_core/src/context.rs b/imageflow_core/src/context.rs
@@ -181,10 +181,12 @@ impl Context {
     }
 
 
-    pub fn get_image_info(&mut self, io_id: i32) -> Result<s::ImageInfo> {
-        self.get_codec(io_id).map_err(|e| e.at(here!()))?.get_decoder().map_err(|e| e.at(here!()))?.get_image_info(self).map_err(|e| e.at(here!()))
+    pub fn get_unscaled_image_info(&mut self, io_id: i32) -> Result<s::ImageInfo> {
+        self.get_codec(io_id).map_err(|e| e.at(here!()))?.get_decoder().map_err(|e| e.at(here!()))?.get_unscaled_image_info(self).map_err(|e| e.at(here!()))
+    }
+    pub fn get_scaled_image_info(&mut self, io_id: i32) -> Result<s::ImageInfo> {
+        self.get_codec(io_id).map_err(|e| e.at(here!()))?.get_decoder().map_err(|e| e.at(here!()))?.get_scaled_image_info(self).map_err(|e| e.at(here!()))
     }
-
     pub fn tell_decoder(&mut self, io_id: i32, tell: s::DecoderCommand) -> Result<()> {
         self.get_codec(io_id).map_err(|e| e.at(here!()))?.get_decoder().map_err(|e| e.at(here!()))?.tell_decoder(self,  tell).map_err(|e| e.at(here!()))
     }

diff --git a/imageflow_core/src/context_methods.rs b/imageflow_core/src/context_methods.rs
@@ -25,11 +25,15 @@ fn create_context_router() -> MethodRouter<'static, Context> {
                     }));
     r.add_responder("v0.1/get_image_info",
                     Box::new(move |context: &mut Context, data: s::GetImageInfo001| {
-                        Ok(s::ResponsePayload::ImageInfo(context.get_image_info(data.io_id).map_err(|e| e.at(here!()))?))
+                        Ok(s::ResponsePayload::ImageInfo(context.get_unscaled_image_info(data.io_id).map_err(|e| e.at(here!()))?))
                     }));
     r.add_responder("v1/get_image_info",
                     Box::new(move |context: &mut Context, data: s::GetImageInfo001| {
-                        Ok(s::ResponsePayload::ImageInfo(context.get_image_info(data.io_id).map_err(|e| e.at(here!()))?))
+                        Ok(s::ResponsePayload::ImageInfo(context.get_unscaled_image_info(data.io_id).map_err(|e| e.at(here!()))?))
+                    }));
+    r.add_responder("v1/get_scaled_image_info",
+                    Box::new(move |context: &mut Context, data: s::GetImageInfo001| {
+                        Ok(s::ResponsePayload::ImageInfo(context.get_scaled_image_info(data.io_id).map_err(|e| e.at(here!()))?))
                     }));
     r.add_responder("v0.1/tell_decoder",
                     Box::new(move |context: &mut Context, data: s::TellDecoder001| {

diff --git a/imageflow_core/src/flow/nodes/codecs_and_pointer.rs b/imageflow_core/src/flow/nodes/codecs_and_pointer.rs
@@ -92,7 +92,7 @@ fn decoder_get_io_id(params: &NodeParams) -> Result<i32> {
 }
 fn decoder_estimate(ctx: &mut OpCtxMut, ix: NodeIndex) -> Result<FrameEstimate> {
     let io_id = decoder_get_io_id(&ctx.weight(ix).params).map_err(|e| e.at(here!()))?;
-    let frame_info = ctx.job.get_image_info(io_id).map_err(|e| e.at(here!()))?;
+    let frame_info = ctx.job.get_scaled_image_info(io_id).map_err(|e| e.at(here!()))?;
 
     Ok(FrameEstimate::Some(FrameInfo {
         fmt: frame_info.frame_decodes_into,

diff --git a/imageflow_core/src/flow/nodes/constrain.rs b/imageflow_core/src/flow/nodes/constrain.rs
@@ -1,6 +1,6 @@
 use super::internal_prelude::*;
 use imageflow_riapi::sizing::{Layout, AspectRatio, LayoutError};
-use imageflow_types::ConstraintMode;
+use imageflow_types::{ConstraintMode, ImageInfo};
 
 pub static CONSTRAIN: ConstrainDef = ConstrainDef{};
 pub static COMMAND_STRING: CommandStringDef = CommandStringDef{};
@@ -12,7 +12,7 @@ pub static EXPANDING_COMMAND_STRING: CommandStringPartiallyExpandedDef = Command
 fn get_decoder_mime(ctx: &mut OpCtxMut, ix: NodeIndex) -> Result<Option<String>>{
     let decoders = ctx.get_decoder_io_ids_and_indexes(ix);
     if let Some((io_id, _)) = decoders.first(){
-        Ok(Some(ctx.c.get_codec(*io_id)?.get_decoder()?.get_image_info(ctx.c)?.preferred_mime_type))
+        Ok(Some(ctx.job.get_unscaled_image_info(*io_id)?.preferred_mime_type))
     }
     else{
         Ok(None)
@@ -22,6 +22,15 @@ fn get_decoder_mime(ctx: &mut OpCtxMut, ix: NodeIndex) -> Result<Option<String>>
 
 fn get_expand(ctx: &mut OpCtxMut, ix: NodeIndex) -> Result<::imageflow_riapi::ir4::Ir4Expand>{
     let input = ctx.first_parent_frame_info_some(ix).ok_or_else(|| nerror!(crate::ErrorKind::InvalidNodeConnections, "CommandString node requires that its parent nodes be perfectly estimable"))?;
+
+    let mut image_info: Option<ImageInfo> = None;
+    if let Some((io_id, decoder_ix)) = ctx.get_decoder_io_ids_and_indexes(ix).first(){
+        image_info = Some(ctx.job.get_unscaled_image_info(*io_id).map_err(|e| e.at(here!()))?);
+
+    }
+
+
+
     let params = &ctx.weight(ix).params;
     if let NodeParams::Json(s::Node::CommandString{ref kind, ref value, ref decode, ref encode, ref watermarks}) =
     *params {
@@ -36,7 +45,10 @@ fn get_expand(ctx: &mut OpCtxMut, ix: NodeIndex) -> Result<::imageflow_riapi::ir
                         h: input.h,
                         fmt: input.fmt,
                         original_mime: get_decoder_mime(ctx,ix)?
-                    }
+                    },
+                    reference_width: image_info.as_ref().map(|i| i.image_width).unwrap_or(input.w),
+                    reference_height: image_info.as_ref().map(|i| i.image_height).unwrap_or(input.h),
+
                 })
             }
         }
@@ -206,7 +218,7 @@ impl NodeDef for CommandStringDef{
                     decode_id: Some(d_id),
                     encode_id: None,
                     watermarks,
-                }.get_decode_node().unwrap();
+                }.get_decode_node_without_commands().unwrap();
                 ctx.replace_node(ix, vec![
                     Node::from(decode_node),
                     Node::n(&EXPANDING_COMMAND_STRING, params)

diff --git a/imageflow_core/src/flow/nodes/watermark.rs b/imageflow_core/src/flow/nodes/watermark.rs
@@ -87,7 +87,7 @@ impl NodeDefOneInputExpand for WatermarkDef {
                         canvas_color: None
                     };
 
-                    let watermark_info = ctx.job.get_image_info(watermark.io_id).map_err(|e| e.at(here!()))?;
+                    let watermark_info = ctx.job.get_scaled_image_info(watermark.io_id).map_err(|e| e.at(here!()))?;
 
                     let constraint_results = imageflow_riapi::ir4::process_constraint(watermark_info.image_width, watermark_info.image_height, &constraint).unwrap(); //TODO: fix unwrap
 

diff --git a/imageflow_core/tests/common/mod.rs b/imageflow_core/tests/common/mod.rs
@@ -752,7 +752,7 @@ pub fn test_with_callback(checksum_name: &str, input: IoTestEnum, callback: fn(&
     unsafe {
         IoTestTranslator{}.add(&mut context, 0, input).unwrap();
 
-        let image_info = context.get_image_info(0).unwrap();
+        let image_info = context.get_unscaled_image_info(0).unwrap();
 
         let (tell_decoder, mut steps): (Option<imageflow_types::DecoderCommand>, Vec<Node>) = callback(&image_info);
 

diff --git a/imageflow_core/tests/visuals.rs b/imageflow_core/tests/visuals.rs
@@ -486,6 +486,22 @@ fn decode_cmyk_jpeg() {
 
 }
 
+#[test]
+fn test_crop_with_preshrink() {
+    let matched = compare(Some(IoTestEnum::Url("https://resizer-images.s3.amazonaws.com/private/cropissue.jpg".to_owned())), 500,
+                          "crop_with_preshrink", POPULATE_CHECKSUMS, DEBUG_GRAPH, vec![
+            Node::CommandString{
+                kind: CommandStringKind::ImageResizer4,
+                value: "w=170&h=220&mode=crop&scale=both&crop=449,0,-472,0".to_owned(),
+                decode: Some(0),
+                encode: None,
+                watermarks: None
+            }
+        ]
+    );
+    assert!(matched);
+
+}
 
 #[test]
 fn webp_lossless_alpha_decode_and_scale() {

diff --git a/imageflow_core/tests/visuals/checksums.json b/imageflow_core/tests/visuals/checksums.json
@@ -35,6 +35,7 @@
   "WatermarkSmall": "09BF9877BE2CB36AC_0DE3336C466C67DD9",
   "WhiteBalanceNight": "0D84206BD2DD308DA_05C9231C6124D62A8",
   "cmyk_decode": "00C5CD1E1ADF2628A_0D58EB335590501F3",
+  "crop_with_preshrink": "04FF87FFA9A645517_09D5B46DC8E13B7B4",
   "encode_frymire": "07FC2C05A26B49BF1.png",
   "encode_gradients": "00D10466A8552DCF6.png",
   "encode_marsRGB": "07800DDF0E598E7E4.png",