[cg] Address code review

cmyr · cmyr · commit 1b397f16e50e · 2020-05-09T20:21:21.000-04:00
A bunch of small fixes. Larger things include:

- reusing unpremultiply_rgba function
- allowing explicit CGGradientDrawingOptions
- better signatures in FFI
diff --git a/piet-common/src/cg_back.rs b/piet-common/src/cg_back.rs
@@ -87,7 +87,7 @@ impl Device {
             core_graphics::base::kCGImageAlphaPremultipliedLast,
         );
         ctx.scale(pix_scale, pix_scale);
-        let height = height as f64 * pix_scale;
+        let height = height as f64 * pix_scale.recip();
         Ok(BitmapTarget {
             ctx,
             height,
@@ -112,27 +112,8 @@ impl<'a> BitmapTarget<'a> {
             return Err(Error::NotSupported);
         }
 
-        let width = self.ctx.width() as usize;
-        let height = self.ctx.height() as usize;
-        let stride = self.ctx.bytes_per_row() as usize;
-
         let data = self.ctx.data();
-        if stride != width {
-            let mut raw_data = vec![0; width * height * 4];
-            for y in 0..height {
-                let src_off = y * stride;
-                let dst_off = y * width * 4;
-                for x in 0..width {
-                    raw_data[dst_off + x * 4 + 0] = data[src_off + x * 4 + 2];
-                    raw_data[dst_off + x * 4 + 1] = data[src_off + x * 4 + 1];
-                    raw_data[dst_off + x * 4 + 2] = data[src_off + x * 4 + 0];
-                    raw_data[dst_off + x * 4 + 3] = data[src_off + x * 4 + 3];
-                }
-            }
-            Ok(raw_data)
-        } else {
-            Ok(data.to_owned())
-        }
+        Ok(data.to_owned())
     }
 
     /// Save bitmap to RGBA PNG file
@@ -141,7 +122,7 @@ impl<'a> BitmapTarget<'a> {
         let width = self.ctx.width() as usize;
         let height = self.ctx.height() as usize;
         let mut data = self.into_raw_pixels(ImageFormat::RgbaPremul)?;
-        unpremultiply(&mut data);
+        piet_coregraphics::unpremultiply(&mut data);
         let file = BufWriter::new(File::create(path).map_err(|e| Into::<Box<_>>::into(e))?);
         let mut encoder = Encoder::new(file, width as u32, height as u32);
         encoder.set_color(ColorType::RGBA);
@@ -160,15 +141,3 @@ impl<'a> BitmapTarget<'a> {
         Err(Error::MissingFeature)
     }
 }
-#[cfg(feature = "png")]
-fn unpremultiply(data: &mut [u8]) {
-    for i in (0..data.len()).step_by(4) {
-        let a = data[i + 3];
-        if a != 0 {
-            let scale = 255.0 / (a as f64);
-            data[i] = (scale * (data[i] as f64)).round() as u8;
-            data[i + 1] = (scale * (data[i + 1] as f64)).round() as u8;
-            data[i + 2] = (scale * (data[i + 2] as f64)).round() as u8;
-        }
-    }
-}
diff --git a/piet-coregraphics/examples/basic-cg.rs b/piet-coregraphics/examples/basic-cg.rs
@@ -68,7 +68,7 @@ fn main() {
     piet.finish().unwrap();
     std::mem::drop(piet);
 
-    unpremultiply(cg_ctx.data());
+    piet_coregraphics::unpremultiply_rgba(cg_ctx.data());
 
     // Write image as PNG file.
     let path = Path::new("image.png");
@@ -82,15 +82,3 @@ fn main() {
 
     writer.write_image_data(cg_ctx.data()).unwrap();
 }
-
-fn unpremultiply(data: &mut [u8]) {
-    for i in (0..data.len()).step_by(4) {
-        let a = data[i + 3];
-        if a != 0 {
-            let scale = 255.0 / (a as f64);
-            data[i] = (scale * (data[i] as f64)).round() as u8;
-            data[i + 1] = (scale * (data[i + 1] as f64)).round() as u8;
-            data[i + 2] = (scale * (data[i + 2] as f64)).round() as u8;
-        }
-    }
-}
diff --git a/piet-coregraphics/examples/test-picture.rs b/piet-coregraphics/examples/test-picture.rs
@@ -42,5 +42,6 @@ fn main() {
     encoder.set_depth(png::BitDepth::Eight);
     let mut writer = encoder.write_header().unwrap();
 
+    piet_coregraphics::unpremultiply_rgba(cg_ctx.data());
     writer.write_image_data(cg_ctx.data()).unwrap();
 }
diff --git a/piet-coregraphics/src/gradient.rs b/piet-coregraphics/src/gradient.rs
@@ -1,3 +1,5 @@
+#![allow(non_upper_case_globals)]
+
 //! core graphics gradient support
 
 use core_foundation::{
@@ -16,14 +18,15 @@ use core_graphics::{
 use piet::kurbo::Point;
 use piet::{Color, FixedGradient, FixedLinearGradient, FixedRadialGradient, GradientStop};
 
+//FIXME: remove all this when core-graphics 0.20.0 is released
 // core-graphics does not provide a CGGradient type
 pub enum CGGradientT {}
 pub type CGGradientRef = *mut CGGradientT;
+pub type CGGradientDrawingOptions = u32;
+pub const CGGradientDrawsBeforeStartLocation: CGGradientDrawingOptions = 1;
+pub const CGGradientDrawsAfterEndLocation: CGGradientDrawingOptions = 1 << 1;
 
-declare_TCFType! {
-    CGGradient, CGGradientRef
-}
-
+declare_TCFType!(CGGradient, CGGradientRef);
 impl_TCFType!(CGGradient, CGGradientRef, CGGradientGetTypeID);
 
 /// A wrapper around CGGradient
@@ -53,8 +56,7 @@ impl Gradient {
         .unwrap_or(Color::BLACK)
     }
 
-    pub(crate) fn fill(&self, ctx: &mut CGContextRef) {
-        let context_ref: *mut u8 = ctx as *mut CGContextRef as *mut u8;
+    pub(crate) fn fill(&self, ctx: &mut CGContextRef, options: CGGradientDrawingOptions) {
         match self.piet_grad {
             FixedGradient::Radial(FixedRadialGradient {
                 center,
@@ -63,16 +65,16 @@ impl Gradient {
                 ..
             }) => {
                 let start_center = to_cgpoint(center + origin_offset);
-                let center = to_cgpoint(center);
+                let end_center = to_cgpoint(center);
                 unsafe {
                     CGContextDrawRadialGradient(
-                        context_ref,
+                        ctx,
                         self.cg_grad.as_concrete_TypeRef(),
                         start_center,
-                        0.0,
-                        center,
+                        0.0, // start_radius
+                        end_center,
                         radius as CGFloat,
-                        0,
+                        options,
                     )
                 }
             }
@@ -81,7 +83,7 @@ impl Gradient {
                 let end = to_cgpoint(end);
                 unsafe {
                     CGContextDrawLinearGradient(
-                        context_ref,
+                        ctx,
                         self.cg_grad.as_concrete_TypeRef(),
                         start,
                         end,
@@ -97,23 +99,19 @@ fn new_cg_gradient(stops: &[GradientStop]) -> CGGradient {
     unsafe {
         //FIXME: is this expensive enough we should be reusing it?
         let space = CGColorSpace::create_with_name(kCGColorSpaceSRGB).unwrap();
-        let space_ref: *const u8 = &*space as *const CGColorSpaceRef as *const u8;
         let mut colors = Vec::<CGColor>::new();
         let mut locations = Vec::<CGFloat>::new();
         for GradientStop { pos, color } in stops {
             let (r, g, b, a) = Color::as_rgba(&color);
-            let color = CGColorCreate(space_ref as *const u8, [r, g, b, a].as_ptr());
+            let color = CGColorCreate(&*space, [r, g, b, a].as_ptr());
             let color = CGColor::wrap_under_create_rule(color);
             colors.push(color);
             locations.push(*pos as CGFloat);
         }
 
         let colors = CFArray::from_CFTypes(&colors);
-        let gradient = CGGradientCreateWithColors(
-            space_ref as *const u8,
-            colors.as_concrete_TypeRef(),
-            locations.as_ptr(),
-        );
+        let gradient =
+            CGGradientCreateWithColors(&*space, colors.as_concrete_TypeRef(), locations.as_ptr());
 
         CGGradient::wrap_under_create_rule(gradient)
     }
@@ -126,21 +124,26 @@ fn to_cgpoint(point: Point) -> CGPoint {
 #[link(name = "CoreGraphics", kind = "framework")]
 extern "C" {
     fn CGGradientGetTypeID() -> CFTypeID;
+    //CGColorSpaceRef is missing repr(c).
+    #[allow(improper_ctypes)]
     fn CGGradientCreateWithColors(
-        space: *const u8,
+        space: *const CGColorSpaceRef,
         colors: CFArrayRef,
         locations: *const CGFloat,
     ) -> CGGradientRef;
-    fn CGColorCreate(space: *const u8, components: *const CGFloat) -> SysCGColorRef;
+    #[allow(improper_ctypes)]
+    fn CGColorCreate(space: *const CGColorSpaceRef, components: *const CGFloat) -> SysCGColorRef;
+    #[allow(improper_ctypes)]
     fn CGContextDrawLinearGradient(
-        ctx: *mut u8,
+        ctx: *mut CGContextRef,
         gradient: CGGradientRef,
         startPoint: CGPoint,
         endPoint: CGPoint,
         options: u32,
     );
+    #[allow(improper_ctypes)]
     fn CGContextDrawRadialGradient(
-        ctx: *mut u8,
+        ctx: *mut CGContextRef,
         gradient: CGGradientRef,
         startCenter: CGPoint,
         startRadius: CGFloat,
diff --git a/piet-coregraphics/src/lib.rs b/piet-coregraphics/src/lib.rs
@@ -29,13 +29,18 @@ pub use crate::text::{
     CoreGraphicsTextLayoutBuilder,
 };
 
-use gradient::Gradient;
+use gradient::{
+    CGGradientDrawingOptions, CGGradientDrawsAfterEndLocation, CGGradientDrawsBeforeStartLocation,
+    Gradient,
+};
+
+const GRADIENT_DRAW_BEFORE_AND_AFTER: CGGradientDrawingOptions =
+    CGGradientDrawsAfterEndLocation | CGGradientDrawsBeforeStartLocation;
 
 pub struct CoreGraphicsContext<'a> {
     // Cairo has this as Clone and with &self methods, but we do this to avoid
     // concurrency problems.
     ctx: &'a mut CGContextRef,
-    // the height of the context; we need this in order to correctly flip the coordinate space
     text: CoreGraphicsText<'a>,
     // because of the relationship between cocoa and coregraphics (where cocoa
     // may be asked to flip the y-axis) we cannot trust the transform returned
@@ -96,13 +101,8 @@ impl<'a> RenderContext for CoreGraphicsContext<'a> {
     //type StrokeStyle = StrokeStyle;
 
     fn clear(&mut self, color: Color) {
-        let rgba = color.as_rgba_u32();
-        self.ctx.set_rgb_fill_color(
-            byte_to_frac(rgba >> 24),
-            byte_to_frac(rgba >> 16),
-            byte_to_frac(rgba >> 8),
-            byte_to_frac(rgba),
-        );
+        let (r, g, b, a) = color.as_rgba();
+        self.ctx.set_rgb_fill_color(r, g, b, a);
         self.ctx.fill_rect(self.ctx.clip_bounding_box());
     }
 
@@ -127,7 +127,7 @@ impl<'a> RenderContext for CoreGraphicsContext<'a> {
             Brush::Gradient(grad) => {
                 self.ctx.save();
                 self.ctx.clip();
-                grad.fill(self.ctx);
+                grad.fill(self.ctx, GRADIENT_DRAW_BEFORE_AND_AFTER);
                 self.ctx.restore();
             }
         }
@@ -144,7 +144,7 @@ impl<'a> RenderContext for CoreGraphicsContext<'a> {
             Brush::Gradient(grad) => {
                 self.ctx.save();
                 self.ctx.eo_clip();
-                grad.fill(self.ctx);
+                grad.fill(self.ctx, GRADIENT_DRAW_BEFORE_AND_AFTER);
                 self.ctx.restore();
             }
         }
@@ -168,7 +168,7 @@ impl<'a> RenderContext for CoreGraphicsContext<'a> {
                 self.ctx.save();
                 self.ctx.replace_path_with_stroked_path();
                 self.ctx.clip();
-                grad.fill(self.ctx);
+                grad.fill(self.ctx, GRADIENT_DRAW_BEFORE_AND_AFTER);
                 self.ctx.restore();
             }
         }
@@ -193,7 +193,7 @@ impl<'a> RenderContext for CoreGraphicsContext<'a> {
                 self.ctx.save();
                 self.ctx.replace_path_with_stroked_path();
                 self.ctx.clip();
-                grad.fill(self.ctx);
+                grad.fill(self.ctx, GRADIENT_DRAW_BEFORE_AND_AFTER);
                 self.ctx.restore();
             }
         }
@@ -256,7 +256,7 @@ impl<'a> RenderContext for CoreGraphicsContext<'a> {
 
     fn transform(&mut self, transform: Affine) {
         if let Some(last) = self.transform_stack.last_mut() {
-            *last = *last * transform;
+            *last *= transform;
         } else {
             self.transform_stack.push(transform);
         }
@@ -439,10 +439,6 @@ impl<'a> CoreGraphicsContext<'a> {
     }
 }
 
-fn byte_to_frac(byte: u32) -> f64 {
-    ((byte & 255) as f64) * (1.0 / 255.0)
-}
-
 fn to_cgpoint(point: Point) -> CGPoint {
     CGPoint::new(point.x as CGFloat, point.y as CGFloat)
 }
@@ -461,6 +457,19 @@ fn to_cgaffine(affine: Affine) -> CGAffineTransform {
     CGAffineTransform::new(a, b, c, d, tx, ty)
 }
 
+#[allow(dead_code)]
+pub fn unpremultiply_rgba(data: &mut [u8]) {
+    for i in (0..data.len()).step_by(4) {
+        let a = data[i + 3];
+        if a != 0 {
+            let scale = 255.0 / (a as f64);
+            data[i] = (scale * (data[i] as f64)).round() as u8;
+            data[i + 1] = (scale * (data[i + 1] as f64)).round() as u8;
+            data[i + 2] = (scale * (data[i + 2] as f64)).round() as u8;
+        }
+    }
+}
+
 #[link(name = "CoreGraphics", kind = "framework")]
 extern "C" {
     fn CGContextClipToMask(ctx: *mut u8, rect: CGRect, mask: *const u8);
diff --git a/piet-coregraphics/src/text.rs b/piet-coregraphics/src/text.rs
@@ -17,7 +17,6 @@ use piet::{
 
 use crate::ct_helpers::{AttributedString, Frame, Framesetter, Line};
 
-// inner is an nsfont.
 #[derive(Debug, Clone)]
 pub struct CoreGraphicsFont(CTFont);
 
@@ -188,24 +187,13 @@ impl TextLayout for CoreGraphicsTextLayout {
         let offset_utf16 = line.get_string_index_for_position(point_in_string_space);
         let offset = match offset_utf16 {
             // this is 'kCFNotFound'.
-            // if nothing is found just go end of string? should this be len - 1? do we have an
-            // implicit newline at end of file? so many mysteries
             -1 => self.string.len(),
             n if n >= 0 => {
                 let utf16_range = line.get_string_range();
                 let utf8_range = self.line_range(line_num).unwrap();
                 let line_txt = self.line_text(line_num).unwrap();
                 let rel_offset = (n - utf16_range.location) as usize;
-                let mut off16 = 0;
-                let mut off8 = 0;
-                for c in line_txt.chars() {
-                    if rel_offset == off16 {
-                        break;
-                    }
-                    off16 += c.len_utf16();
-                    off8 += c.len_utf8();
-                }
-                utf8_range.0 + off8
+                utf8_range.0 + utf8_offset_for_utf16_offset(line_txt, rel_offset)
             }
             // some other value; should never happen
             _ => panic!("gross violation of api contract"),
@@ -323,6 +311,19 @@ impl CoreGraphicsTextLayout {
     }
 }
 
+fn utf8_offset_for_utf16_offset(text: &str, utf16_offset: usize) -> usize {
+    let mut off16 = 0;
+    let mut off8 = 0;
+    for c in text.chars() {
+        if utf16_offset == off16 {
+            break;
+        }
+        off16 += c.len_utf16();
+        off8 += c.len_utf8();
+    }
+    off8
+}
+
 #[cfg(test)]
 #[allow(clippy::float_cmp)]
 mod tests {

Original file line number	Diff line number	Diff line change
`@@ -42,5 +42,6 @@ fn main() {`
`42`	`42`	`encoder.set_depth(png::BitDepth::Eight);`
`43`	`43`	`let mut writer = encoder.write_header().unwrap();`
`44`	`44`
	`45`	`+ piet_coregraphics::unpremultiply_rgba(cg_ctx.data());`
`45`	`46`	`writer.write_image_data(cg_ctx.data()).unwrap();`
`46`	`47`	`}`