diff --git a/src/encoder_state-ctors_dtors.c b/src/encoder_state-ctors_dtors.c index bc532fbc..067c1b84 100644 --- a/src/encoder_state-ctors_dtors.c +++ b/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; diff --git a/src/encoderstate.c b/src/encoderstate.c index c70709bb..b6011195 100644 --- a/src/encoderstate.c +++ b/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); } diff --git a/src/image.c b/src/image.c index 7e211800..56d811d3 100644 --- a/src/image.c +++ b/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,59 +72,78 @@ 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; - - //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->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; - - free(im); + if (im == NULL) return; - return 1; + 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; + } + + 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); } - -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; - - im->base_image = orig_image->base_image; - ATOMIC_INC(&(im->base_image->refcount)); - - assert(x_offset + width <= orig_image->width); - assert(y_offset + height <= orig_image->height); - - //Assert that we have a well defined image + int32_t new_refcount = ATOMIC_INC(&(im->refcount)); + + // The caller should have had another reference. + assert(new_refcount > 1); + + return 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) +{ + // 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]; im->v = im->data[COLOR_V] = &orig_image->v[x_offset/2 + y_offset/2 * orig_image->stride/2]; diff --git a/src/image.h b/src/image.h index a4330607..eb22523e 100644 --- a/src/image.h +++ b/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); diff --git a/src/imagelist.c b/src/imagelist.c index dbcca9bc..8fe3fc08 100644 --- a/src/imagelist.c +++ b/src/imagelist.c @@ -150,12 +150,8 @@ 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"); assert(0); //Stop here diff --git a/src/kvazaar.c b/src/kvazaar.c index cdd08e73..3251f3ef 100644 --- a/src/kvazaar.c +++ b/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; } diff --git a/src/kvazaar.h b/src/kvazaar.h index da7c527e..f48f1ec9 100644 --- a/src/kvazaar.h +++ b/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; diff --git a/src/videoframe.c b/src/videoframe.c index cf697563..8ae5bace 100644 --- a/src/videoframe.c +++ b/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);