Let subimages have multiple references.

Adds function image_copy_ref to image module for getting a new reference
to an image. It can be used instead of image_make_subimage when the
sizes of the original and the subimage are same.

src/encoder_state-ctors_dtors.c

@@ -105,9 +105,6 @@ static void encoder_state_config_tile_finalize(encoder_state_t * const state) {
   yuv_t_free(state->tile->hor_buf_search);
   yuv_t_free(state->tile->ver_buf_search);
   
-  if (state->tile->frame->source) image_free(state->tile->frame->source);
-  if (state->tile->frame->rec) image_free(state->tile->frame->rec);
-
   videoframe_free(state->tile->frame);
   state->tile->frame = NULL;
   

src/encoderstate.c

@@ -879,6 +879,7 @@ int read_one_frame(FILE* file, const encoder_state_t * const state, image_t *img
     int i;
     for (i = 0; i < state->encoder_control->cfg->gop_len; i++) {
       gop_pictures[i].source = image_alloc(array_width, array_height);
+      assert(gop_pictures[i].source);
     }
     state->global->gop_offset = 0;
     gop_init = 1;
@@ -980,7 +981,7 @@ void encoder_next_frame(encoder_state_t *state, image_t *img_in)
   if (state->tile->frame->source) {
     image_free(state->tile->frame->source);
   }
-  state->tile->frame->source = image_make_subimage(img_in, 0, 0, state->tile->frame->width, state->tile->frame->height);
+  state->tile->frame->source = image_copy_ref(img_in);
   
   state->stats_done = 0;
 
@@ -990,6 +991,7 @@ void encoder_next_frame(encoder_state_t *state, image_t *img_in)
     state->global->poc = 0;
     assert(!state->tile->frame->rec);
     state->tile->frame->rec = image_alloc(state->tile->frame->width, state->tile->frame->height);
+    assert(state->tile->frame->rec);
     return;
   }
   
@@ -1004,6 +1006,7 @@ void encoder_next_frame(encoder_state_t *state, image_t *img_in)
     cu_array_free(state->tile->frame->cu_array);
     
     state->tile->frame->rec = image_alloc(state->tile->frame->width, state->tile->frame->height);
+    assert(state->tile->frame->rec);
     {
       // Allocate height_in_scu x width_in_scu x sizeof(CU_info)
       unsigned height_in_scu = state->tile->frame->height_in_lcu << MAX_DEPTH;
@@ -1043,6 +1046,7 @@ void encoder_next_frame(encoder_state_t *state, image_t *img_in)
   image_free(state->tile->frame->rec);
 
   state->tile->frame->rec = image_alloc(state->tile->frame->width, state->tile->frame->height);
+  assert(state->tile->frame->rec);
   videoframe_set_poc(state->tile->frame, state->global->poc);
 }
 

src/image.c

@@ -36,32 +36,34 @@
 #include "sao.h"
 
 /**
- * \brief Allocate new image
- * \return image pointer
+ * \brief Allocate a new image.
+ * \return image pointer or NULL on failure
  */
 image_t *image_alloc(const int32_t width, const int32_t height)
 {
-  image_t *im = MALLOC(image_t, 1);
-  
-  unsigned int luma_size = width * height;
-  unsigned int chroma_size = luma_size / 4;
-  
   //Assert that we have a well defined image
   assert((width % 2) == 0);
   assert((height % 2) == 0);
 
+  image_t *im = MALLOC(image_t, 1);
   if (!im) return NULL;
 
+  unsigned int luma_size = width * height;
+  unsigned int chroma_size = luma_size / 4;
+
+  //Allocate memory
+  im->fulldata = MALLOC(pixel_t, (luma_size + 2 * chroma_size));
+  if (!im->fulldata) {
+    free(im);
+    return NULL;
+  }
+
+  im->base_image = im;
+  im->refcount = 1; //We give a reference to caller
   im->width = width;
   im->height = height;
   im->stride = width;
 
-  im->base_image = im;
-  
-  im->refcount = 1; //We give a reference to caller
-  
-  //Allocate memory
-  im->fulldata = MALLOC(pixel_t, (luma_size + 2 * chroma_size));
   im->y = im->data[COLOR_Y] = &im->fulldata[0];
   im->u = im->data[COLOR_U] = &im->fulldata[luma_size];
   im->v = im->data[COLOR_V] = &im->fulldata[luma_size + chroma_size];
@@ -70,58 +72,77 @@ image_t *image_alloc(const int32_t width, const int32_t height)
 }
 
 /**
- * \brief Free memory allocated to picture (if we have no reference left)
- * \param pic picture pointer
- * \return 1 on success, 0 on failure
+ * \brief Free an image.
+ *
+ * Decrement reference count of the image and deallocate associated memory
+ * if no references exist any more.
+ *
+ * \param im image to free
  */
-int image_free(image_t * const im)
+void image_free(image_t * const im)
 {
-  //Nothing to do
-  if (!im) return 1;
+  if (im == NULL) return;
 
-  //Either we are the base image, or we should have no references
-  assert(im->base_image == im || im->refcount == 0);
+  int32_t new_refcount = ATOMIC_DEC(&(im->refcount));
+  if (new_refcount > 0) {
+    // There are still references so we don't free the data yet.
+    return;
+  }
 
-  int32_t new_refcount = ATOMIC_DEC(&(im->base_image->refcount));
-  //If we're freeing a subimage, then we must free the pointer
-  //Base image may be stored in image_list, and should not be freed
-  //FIXME I don't find this very clean...
-  if (new_refcount > 0 && im->base_image != im) free(im);
-  if (new_refcount > 0) return 1;
-  FREE_POINTER(im->base_image->fulldata);
-  
-  //Just to make the program crash when using those values after the free
-  im->y = im->u = im->v = im->data[COLOR_Y] = im->data[COLOR_U] = im->data[COLOR_V] = NULL;
+  if (im->base_image != im) {
+    // Free our reference to the base image.
+    image_free(im->base_image);
+  } else {
+    free(im->fulldata);
+  }
 
+  // Make sure freed data won't be used.
+  im->base_image = NULL;
+  im->fulldata = NULL;
+  im->y = im->u = im->v = NULL;
+  im->data[COLOR_Y] = im->data[COLOR_U] = im->data[COLOR_V] = NULL;
   free(im);
-
-  return 1;
 }
 
-
-image_t *image_make_subimage(image_t * const orig_image, const unsigned int x_offset, const unsigned int y_offset, const unsigned int width, const unsigned int height)
+/**
+ * \brief Get a new pointer to an image.
+ *
+ * Increment reference count and return the image.
+ */
+image_t *image_copy_ref(image_t *im)
 {
-  image_t *im = MALLOC(image_t, 1);
-  if (!im) return NULL;
+  int32_t new_refcount = ATOMIC_INC(&(im->refcount));
 
-  im->base_image = orig_image->base_image;
-  ATOMIC_INC(&(im->base_image->refcount));
+  // The caller should have had another reference.
+  assert(new_refcount > 1);
 
-  assert(x_offset + width <= orig_image->width);
-  assert(y_offset + height <= orig_image->height);
+  return im;
+}
 
-  //Assert that we have a well defined image
+image_t *image_make_subimage(image_t *const orig_image,
+                             const unsigned x_offset,
+                             const unsigned y_offset,
+                             const unsigned width,
+                             const unsigned height)
+{
+  // Assert that we have a well defined image
   assert((width % 2) == 0);
   assert((height % 2) == 0);
 
   assert((x_offset % 2) == 0);
   assert((y_offset % 2) == 0);
 
-  im->stride = orig_image->stride;
-  im->refcount = 0; //No references on subimages
+  assert(x_offset + width <= orig_image->width);
+  assert(y_offset + height <= orig_image->height);
 
+  image_t *im = MALLOC(image_t, 1);
+  if (!im) return NULL;
+
+  im->base_image = image_copy_ref(orig_image->base_image);
+  im->refcount = 1; // We give a reference to caller
   im->width = width;
   im->height = height;
+  im->stride = orig_image->stride;
 
   im->y = im->data[COLOR_Y] = &orig_image->y[x_offset + y_offset * orig_image->stride];
   im->u = im->data[COLOR_U] = &orig_image->u[x_offset/2 + y_offset/2 * orig_image->stride/2];

src/image.h

@@ -43,8 +43,16 @@ typedef struct {
 
 
 image_t *image_alloc(const int32_t width, const int32_t height);
-int image_free(image_t * im);
-image_t *image_make_subimage(image_t * const orig_image, const unsigned int x_offset, const unsigned int y_offset, const unsigned int width, const unsigned int height);
+
+void image_free(image_t *im);
+
+image_t *image_copy_ref(image_t *im);
+
+image_t *image_make_subimage(image_t *const orig_image,
+                             const unsigned x_offset,
+                             const unsigned y_offset,
+                             const unsigned width,
+                             const unsigned height);
 
 yuv_t * yuv_t_alloc(int luma_size);
 void yuv_t_free(yuv_t * yuv);

src/imagelist.c

@@ -150,11 +150,7 @@ int image_list_rem(image_list_t * const list, const unsigned n)
     return 0;
   }
 
-  if (!image_free(list->images[n])) {
-    fprintf(stderr, "Could not free image!\n");
-    assert(0); //Stop here
-    return 0;
-  }
+  image_free(list->images[n]);
 
   if (!cu_array_free(list->cu_arrays[n])) {
     fprintf(stderr, "Could not free cu_array!\n");

src/kvazaar.c

@@ -141,7 +141,7 @@ static int kvazaar_encode(kvz_encoder *enc, kvz_picture *img_in, kvz_picture **i
       fflush(payload->file.output);
     }
 
-    *img_out = image_make_subimage(state->tile->frame->rec, 0, 0, state->tile->frame->width, state->tile->frame->height);
+    *img_out = image_copy_ref(state->tile->frame->rec);
 
     enc->frames_done += 1;
   }

src/kvazaar.h

@@ -66,7 +66,7 @@ typedef struct image_t {
   int32_t stride;          //!< \brief Luma pixel array width for the full picture (should be used as stride)
 
   struct image_t * base_image; //!< \brief Pointer to the image to which the pixels belong
-  int32_t refcount;        //!< \brief Number of references in reflist to the picture
+  int32_t refcount;        //!< \brief Number of references to the picture
 
 } image_t;
 

src/videoframe.c

@@ -71,8 +71,10 @@ videoframe_t *videoframe_alloc(const int32_t width, const int32_t height, const
  */
 int videoframe_free(videoframe_t * const frame)
 {
-  //image_free(frame->source);
-  //image_free(frame->rec);
+  image_free(frame->source);
+  frame->source = NULL;
+  image_free(frame->rec);
+  frame->rec = NULL;
 
   cu_array_free(frame->cu_array);