Clean up and abstract shader creation code

Co-authored-by: Christian Duerr <contact@christianduerr.com>

alacritty/src/renderer/mod.rs

@@ -1,8 +1,7 @@
 use std::collections::HashMap;
-use std::fmt::{self, Display, Formatter};
 use std::hash::BuildHasherDefault;
 use std::mem::size_of;
-use std::{io, ptr};
+use std::{fmt, ptr};
 
 use bitflags::bitflags;
 use crossfont::{
@@ -24,9 +23,18 @@ use crate::display::content::RenderableCell;
 use crate::gl;
 use crate::gl::types::*;
 use crate::renderer::rects::{RectRenderer, RenderRect};
+use crate::renderer::shader::{ShaderError, ShaderProgram};
 
 pub mod builtin_font;
 pub mod rects;
+mod shader;
+
+macro_rules! cstr {
+    ($s:literal) => {
+        // This can be optimized into an no-op with pre-allocated NUL-terminated bytes.
+        unsafe { std::ffi::CStr::from_ptr(concat!($s, "\0").as_ptr().cast()) }
+    };
+}
 
 // Shader source.
 static TEXT_SHADER_F: &str = include_str!("../../res/text.f.glsl");
@@ -45,30 +53,31 @@ pub trait LoadGlyph {
 
 #[derive(Debug)]
 pub enum Error {
-    ShaderCreation(ShaderCreationError),
+    /// Shader error.
+    Shader(ShaderError),
 }
 
 impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match self {
-            Error::ShaderCreation(err) => err.source(),
+            Error::Shader(err) => err.source(),
         }
     }
 }
 
-impl Display for Error {
-    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+impl fmt::Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            Error::ShaderCreation(err) => {
+            Error::Shader(err) => {
                 write!(f, "There was an error initializing the shaders: {}", err)
             },
         }
     }
 }
 
-impl From<ShaderCreationError> for Error {
-    fn from(val: ShaderCreationError) -> Self {
-        Error::ShaderCreation(val)
+impl From<ShaderError> for Error {
+    fn from(val: ShaderError) -> Self {
+        Error::Shader(val)
     }
 }
 
@@ -77,8 +86,8 @@ impl From<ShaderCreationError> for Error {
 /// Uniforms are prefixed with "u", and vertex attributes are prefixed with "a".
 #[derive(Debug)]
 pub struct TextShaderProgram {
-    /// Program id.
-    id: GLuint,
+    /// Shader program.
+    program: ShaderProgram,
 
     /// Projection scale and offset uniform.
     u_projection: GLint,
@@ -707,7 +716,7 @@ impl QuadRenderer {
         F: FnOnce(RenderApi<'_>) -> T,
     {
         unsafe {
-            gl::UseProgram(self.program.id);
+            gl::UseProgram(self.program.id());
             self.program.set_term_uniforms(props);
 
             gl::BindVertexArray(self.vao);
@@ -756,7 +765,7 @@ impl QuadRenderer {
             self.set_viewport(size);
 
             // Update projection.
-            gl::UseProgram(self.program.id);
+            gl::UseProgram(self.program.id());
             self.program.update_projection(
                 size.width(),
                 size.height(),
@@ -1004,56 +1013,18 @@ impl<'a> Drop for RenderApi<'a> {
 }
 
 impl TextShaderProgram {
-    pub fn new() -> Result<TextShaderProgram, ShaderCreationError> {
-        let vertex_shader = create_shader(gl::VERTEX_SHADER, TEXT_SHADER_V)?;
-        let fragment_shader = create_shader(gl::FRAGMENT_SHADER, TEXT_SHADER_F)?;
-        let program = create_program(vertex_shader, fragment_shader)?;
+    pub fn new() -> Result<TextShaderProgram, Error> {
+        let program = ShaderProgram::new(TEXT_SHADER_V, TEXT_SHADER_F)?;
+        Ok(Self {
+            u_projection: program.get_uniform_location(cstr!("projection"))?,
+            u_cell_dim: program.get_uniform_location(cstr!("cellDim"))?,
+            u_background: program.get_uniform_location(cstr!("backgroundPass"))?,
+            program,
+        })
+    }
 
-        unsafe {
-            gl::DeleteShader(fragment_shader);
-            gl::DeleteShader(vertex_shader);
-            gl::UseProgram(program);
-        }
-
-        macro_rules! cptr {
-            ($thing:expr) => {
-                $thing.as_ptr() as *const _
-            };
-        }
-
-        macro_rules! assert_uniform_valid {
-            ($uniform:expr) => {
-                assert!($uniform != gl::INVALID_VALUE as i32);
-                assert!($uniform != gl::INVALID_OPERATION as i32);
-            };
-            ( $( $uniform:expr ),* ) => {
-                $( assert_uniform_valid!($uniform); )*
-            };
-        }
-
-        // get uniform locations
-        let (projection, cell_dim, background) = unsafe {
-            (
-                gl::GetUniformLocation(program, cptr!(b"projection\0")),
-                gl::GetUniformLocation(program, cptr!(b"cellDim\0")),
-                gl::GetUniformLocation(program, cptr!(b"backgroundPass\0")),
-            )
-        };
-
-        assert_uniform_valid!(projection, cell_dim, background);
-
-        let shader = Self {
-            id: program,
-            u_projection: projection,
-            u_cell_dim: cell_dim,
-            u_background: background,
-        };
-
-        unsafe {
-            gl::UseProgram(0);
-        }
-
-        Ok(shader)
+    fn id(&self) -> GLuint {
+        self.program.id()
     }
 
     fn update_projection(&self, width: f32, height: f32, padding_x: f32, padding_y: f32) {
@@ -1090,147 +1061,6 @@ impl TextShaderProgram {
     }
 }
 
-impl Drop for TextShaderProgram {
-    fn drop(&mut self) {
-        unsafe {
-            gl::DeleteProgram(self.id);
-        }
-    }
-}
-
-pub fn create_program(vertex: GLuint, fragment: GLuint) -> Result<GLuint, ShaderCreationError> {
-    unsafe {
-        let program = gl::CreateProgram();
-        gl::AttachShader(program, vertex);
-        gl::AttachShader(program, fragment);
-        gl::LinkProgram(program);
-
-        let mut success: GLint = 0;
-        gl::GetProgramiv(program, gl::LINK_STATUS, &mut success);
-
-        if success == i32::from(gl::TRUE) {
-            Ok(program)
-        } else {
-            Err(ShaderCreationError::Link(get_program_info_log(program)))
-        }
-    }
-}
-
-pub fn create_shader(kind: GLenum, source: &'static str) -> Result<GLuint, ShaderCreationError> {
-    let len: [GLint; 1] = [source.len() as GLint];
-
-    let shader = unsafe {
-        let shader = gl::CreateShader(kind);
-        gl::ShaderSource(shader, 1, &(source.as_ptr() as *const _), len.as_ptr());
-        gl::CompileShader(shader);
-        shader
-    };
-
-    let mut success: GLint = 0;
-    unsafe {
-        gl::GetShaderiv(shader, gl::COMPILE_STATUS, &mut success);
-    }
-
-    if success == GLint::from(gl::TRUE) {
-        Ok(shader)
-    } else {
-        // Read log.
-        let log = get_shader_info_log(shader);
-
-        // Cleanup.
-        unsafe {
-            gl::DeleteShader(shader);
-        }
-
-        Err(ShaderCreationError::Compile(log))
-    }
-}
-
-fn get_program_info_log(program: GLuint) -> String {
-    // Get expected log length.
-    let mut max_length: GLint = 0;
-    unsafe {
-        gl::GetProgramiv(program, gl::INFO_LOG_LENGTH, &mut max_length);
-    }
-
-    // Read the info log.
-    let mut actual_length: GLint = 0;
-    let mut buf: Vec<u8> = Vec::with_capacity(max_length as usize);
-    unsafe {
-        gl::GetProgramInfoLog(program, max_length, &mut actual_length, buf.as_mut_ptr() as *mut _);
-    }
-
-    // Build a string.
-    unsafe {
-        buf.set_len(actual_length as usize);
-    }
-
-    // XXX should we expect OpenGL to return garbage?
-    String::from_utf8(buf).unwrap()
-}
-
-fn get_shader_info_log(shader: GLuint) -> String {
-    // Get expected log length.
-    let mut max_length: GLint = 0;
-    unsafe {
-        gl::GetShaderiv(shader, gl::INFO_LOG_LENGTH, &mut max_length);
-    }
-
-    // Read the info log.
-    let mut actual_length: GLint = 0;
-    let mut buf: Vec<u8> = Vec::with_capacity(max_length as usize);
-    unsafe {
-        gl::GetShaderInfoLog(shader, max_length, &mut actual_length, buf.as_mut_ptr() as *mut _);
-    }
-
-    // Build a string.
-    unsafe {
-        buf.set_len(actual_length as usize);
-    }
-
-    // XXX should we expect OpenGL to return garbage?
-    String::from_utf8(buf).unwrap()
-}
-
-#[derive(Debug)]
-pub enum ShaderCreationError {
-    /// Error reading file.
-    Io(io::Error),
-
-    /// Error compiling shader.
-    Compile(String),
-
-    /// Problem linking.
-    Link(String),
-}
-
-impl std::error::Error for ShaderCreationError {
-    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
-        match self {
-            ShaderCreationError::Io(err) => err.source(),
-            _ => None,
-        }
-    }
-}
-
-impl Display for ShaderCreationError {
-    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        match self {
-            ShaderCreationError::Io(err) => write!(f, "Unable to read shader: {}", err),
-            ShaderCreationError::Compile(log) => {
-                write!(f, "Failed compiling shader: {}", log)
-            },
-            ShaderCreationError::Link(log) => write!(f, "Failed linking shader: {}", log),
-        }
-    }
-}
-
-impl From<io::Error> for ShaderCreationError {
-    fn from(val: io::Error) -> Self {
-        ShaderCreationError::Io(val)
-    }
-}
-
 /// Manages a single texture atlas.
 ///
 /// The strategy for filling an atlas looks roughly like this:

alacritty/src/renderer/rects.rs

@@ -11,6 +11,7 @@ use alacritty_terminal::term::SizeInfo;
 
 use crate::display::content::RenderableCell;
 use crate::gl::types::*;
+use crate::renderer::shader::ShaderProgram;
 use crate::{gl, renderer};
 
 #[derive(Debug, Copy, Clone)]
@@ -221,7 +222,7 @@ pub struct RectRenderer {
     vao: GLuint,
     vbo: GLuint,
 
-    program: RectShaderProgram,
+    program: ShaderProgram,
 
     vertices: Vec<Vertex>,
 }
@@ -230,7 +231,7 @@ impl RectRenderer {
     pub fn new() -> Result<Self, renderer::Error> {
         let mut vao: GLuint = 0;
         let mut vbo: GLuint = 0;
-        let program = RectShaderProgram::new()?;
+        let program = ShaderProgram::new(RECT_SHADER_V, RECT_SHADER_F)?;
 
         unsafe {
             // Allocate buffers.
@@ -283,7 +284,7 @@ impl RectRenderer {
             // Bind VBO only once for buffer data upload only.
             gl::BindBuffer(gl::ARRAY_BUFFER, self.vbo);
 
-            gl::UseProgram(self.program.id);
+            gl::UseProgram(self.program.id());
         }
 
         let half_width = size_info.width() / 2.;
@@ -352,38 +353,3 @@ impl Drop for RectRenderer {
         }
     }
 }
-
-/// Rectangle drawing program.
-#[derive(Debug)]
-pub struct RectShaderProgram {
-    /// Program id.
-    id: GLuint,
-}
-
-impl RectShaderProgram {
-    pub fn new() -> Result<Self, renderer::ShaderCreationError> {
-        let vertex_shader = renderer::create_shader(gl::VERTEX_SHADER, RECT_SHADER_V)?;
-        let fragment_shader = renderer::create_shader(gl::FRAGMENT_SHADER, RECT_SHADER_F)?;
-        let program = renderer::create_program(vertex_shader, fragment_shader)?;
-
-        unsafe {
-            gl::DeleteShader(fragment_shader);
-            gl::DeleteShader(vertex_shader);
-            gl::UseProgram(program);
-        }
-
-        let shader = Self { id: program };
-
-        unsafe { gl::UseProgram(0) }
-
-        Ok(shader)
-    }
-}
-
-impl Drop for RectShaderProgram {
-    fn drop(&mut self) {
-        unsafe {
-            gl::DeleteProgram(self.id);
-        }
-    }
-}

alacritty/src/renderer/shader.rs

@@ -0,0 +1,159 @@
+use std::ffi::CStr;
+use std::fmt;
+
+use crate::gl;
+use crate::gl::types::*;
+
+/// A wrapper for a shader program id, with automatic lifetime management.
+#[derive(Debug)]
+pub struct ShaderProgram(GLuint);
+
+impl ShaderProgram {
+    pub fn new(
+        vertex_shader: &'static str,
+        fragment_shader: &'static str,
+    ) -> Result<Self, ShaderError> {
+        let vertex_shader = Shader::new(gl::VERTEX_SHADER, vertex_shader)?;
+        let fragment_shader = Shader::new(gl::FRAGMENT_SHADER, fragment_shader)?;
+
+        let program = unsafe { Self(gl::CreateProgram()) };
+
+        let mut success: GLint = 0;
+        unsafe {
+            gl::AttachShader(program.id(), vertex_shader.id());
+            gl::AttachShader(program.id(), fragment_shader.id());
+            gl::LinkProgram(program.id());
+            gl::GetProgramiv(program.id(), gl::LINK_STATUS, &mut success);
+        }
+
+        if success != i32::from(gl::TRUE) {
+            return Err(ShaderError::Link(get_program_info_log(program.id())));
+        }
+
+        Ok(program)
+    }
+
+    /// Get uniform location by name. Panic if failed.
+    pub fn get_uniform_location(&self, name: &'static CStr) -> Result<GLint, ShaderError> {
+        // This call doesn't require `UseProgram`.
+        let ret = unsafe { gl::GetUniformLocation(self.id(), name.as_ptr()) };
+        if ret == -1 {
+            return Err(ShaderError::Uniform(name));
+        }
+        Ok(ret)
+    }
+
+    /// Get the shader program id.
+    pub fn id(&self) -> GLuint {
+        self.0
+    }
+}
+
+impl Drop for ShaderProgram {
+    fn drop(&mut self) {
+        unsafe { gl::DeleteProgram(self.0) }
+    }
+}
+
+/// A wrapper for a shader id, with automatic lifetime management.
+#[derive(Debug)]
+struct Shader(GLuint);
+
+impl Shader {
+    fn new(kind: GLenum, source: &'static str) -> Result<Self, ShaderError> {
+        let len: [GLint; 1] = [source.len() as GLint];
+
+        let shader = unsafe { Self(gl::CreateShader(kind)) };
+
+        let mut success: GLint = 0;
+        unsafe {
+            gl::ShaderSource(shader.id(), 1, &(source.as_ptr() as *const _), len.as_ptr());
+            gl::CompileShader(shader.id());
+            gl::GetShaderiv(shader.id(), gl::COMPILE_STATUS, &mut success);
+        }
+
+        if success != GLint::from(gl::TRUE) {
+            return Err(ShaderError::Compile(get_shader_info_log(shader.id())));
+        }
+
+        Ok(shader)
+    }
+
+    fn id(&self) -> GLuint {
+        self.0
+    }
+}
+
+impl Drop for Shader {
+    fn drop(&mut self) {
+        unsafe { gl::DeleteShader(self.0) }
+    }
+}
+
+fn get_program_info_log(program: GLuint) -> String {
+    // Get expected log length.
+    let mut max_length: GLint = 0;
+    unsafe {
+        gl::GetProgramiv(program, gl::INFO_LOG_LENGTH, &mut max_length);
+    }
+
+    // Read the info log.
+    let mut actual_length: GLint = 0;
+    let mut buf: Vec<u8> = Vec::with_capacity(max_length as usize);
+    unsafe {
+        gl::GetProgramInfoLog(program, max_length, &mut actual_length, buf.as_mut_ptr() as *mut _);
+    }
+
+    // Build a string.
+    unsafe {
+        buf.set_len(actual_length as usize);
+    }
+
+    String::from_utf8_lossy(&buf).to_string()
+}
+
+fn get_shader_info_log(shader: GLuint) -> String {
+    // Get expected log length.
+    let mut max_length: GLint = 0;
+    unsafe {
+        gl::GetShaderiv(shader, gl::INFO_LOG_LENGTH, &mut max_length);
+    }
+
+    // Read the info log.
+    let mut actual_length: GLint = 0;
+    let mut buf: Vec<u8> = Vec::with_capacity(max_length as usize);
+    unsafe {
+        gl::GetShaderInfoLog(shader, max_length, &mut actual_length, buf.as_mut_ptr() as *mut _);
+    }
+
+    // Build a string.
+    unsafe {
+        buf.set_len(actual_length as usize);
+    }
+
+    String::from_utf8_lossy(&buf).to_string()
+}
+
+#[derive(Debug)]
+pub enum ShaderError {
+    /// Error compiling shader.
+    Compile(String),
+
+    /// Error linking shader.
+    Link(String),
+
+    /// Error getting uniform location.
+    Uniform(&'static CStr),
+}
+
+impl std::error::Error for ShaderError {}
+
+impl fmt::Display for ShaderError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::Compile(reason) => write!(f, "Failed compiling shader: {}", reason),
+            Self::Link(reason) => write!(f, "Failed linking shader: {}", reason),
+            Self::Uniform(name) => write!(f, "Failed to get uniform location of {:?}", name),
+        }
+    }
+}