opj_stream_destroy() should not be deprecated

[GoogleCodeExporter]

THE USUAL CASE
==============

  opj_stream_create_file_stream_v3(const char *fname,
   OPJ_SIZE_T p_size, OPJ_BOOL p_is_read_stream)

contains

  opj_stream_set_user_data(l_stream, p_file);

with

  FILE *p_file = fopen(fname, mode);

  opj_stream_set_user_data(opj_stream_t* p_stream, void * p_data)
  {
      opj_stream_private_t* l_stream = (opj_stream_private_t*) p_stream;
      l_stream->m_user_data = p_data;
  }

In short: 'l_stream->m_user_data = file_pointer;'

This allows

  opj_stream_destroy_v3(opj_stream_t* p_stream)

to close that file:

  FILE *fp = (FILE*)l_stream->m_user_data;

  if(fp) fclose(fp);

THE PROBLEM
===========

  opj_stream_create_buffer_stream(BufInfo* p_file,
      OPJ_UINT32 p_size, OPJ_BOOL p_is_read_stream)
  {
  opj_stream_set_user_data(l_stream, p_file);
  }

In short: 'l_stream->m_user_data = address_of_BufInfo;'

As this is no file_pointer I must use

  OPJ_DEPRECATED(OPJ_API void OPJ_CALLCONV opj_stream_destroy(opj_stream_t* p_stream));

RESULT
======

opj_stream_destroy() should not be deprecated. Or renamed.

winfried

Original issue reported on code.google.com by szukw...@arcor.de on 29 Jun 2013 at 3:52

[GoogleCodeExporter]

I agree. In the mupdf project (mupdf.com) we're calling 
opj_stream_default_create() to create a stream and then 
opj_stream_set_user_data() to set the user data pointer to point a struct of 
our own. The end result is a case similar to winfried's above.

I propose that you add a callback when setting the user data and then call this 
callback from inside opj_stream_destroy() if the callback is set. 

This way opj_stream_create_file_stream_v3() may set the userdata to be a 
pointer to a FILE* and set the callback to fclose(). In winfried's case he can 
set the userdata to BufInfo* and a suitable callback to free BufInfo* if 
necessary. In our case we can set the userdata to our stack-allocated struct 
and set the callback to NULL (since no freeing is required).

Something along the lines of this:

void opj_stream_set_user_data(opj_stream_t * stream, void *user_data, void 
(*free_user_data)(void *))
{
  stream->m_user_data = user_data;
  stream->m_free_user_data = free_user_data;
}

void opj_stream_destroy(opj_stream_t *stream)
{
    if (stream->m_free_user_data)
    {
        stream->m_free_user_data(stream->m_user_data);
    }
}

Does this seem reasonable to you?

Original comment by seb...@gmail.com on 26 Jul 2013 at 9:27

[GoogleCodeExporter]

I made an example patch to illustrate my point even further. openjpeg compiles 
with this patch applied, but I haven't tested it further. Feel free to use it 
and modify it to fit with your liking. :)

Original comment by seb...@gmail.com on 26 Jul 2013 at 9:50

Attachments:

• destroy.diff

[GoogleCodeExporter]

Original comment by mathieu.malaterre on 25 Feb 2014 at 3:32

• Added labels: Milestone-Release2.1, Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

destroy.diff patch is incomplete it does not call fclose anymore when calling 
opj_stream_create_file_stream_v3

Original comment by mathieu.malaterre on 27 Feb 2014 at 1:18

• Changed state: Started

[GoogleCodeExporter]

This issue was closed by revision r2610.

Original comment by mathieu.malaterre on 7 Mar 2014 at 9:58

• Changed state: Fixed