[vlc-devel] [PATCH 5/8] opus: factorize error cases in OpenEncoder

[Rafaël Carré]

Le 22/09/2013 07:52, Tristan Matthews a écrit :
> On Sat, Sep 21, 2013 at 10:54 AM, Rafaël Carré <funman at videolan.org> wrote:
> 
>> ---
>>  modules/codec/opus.c | 35 ++++++++++++++++-------------------
>>  1 file changed, 16 insertions(+), 19 deletions(-)
>>
>> diff --git a/modules/codec/opus.c b/modules/codec/opus.c
>> index fa96d2c..2f50b77 100644
>> --- a/modules/codec/opus.c
>> +++ b/modules/codec/opus.c
>> @@ -571,6 +571,9 @@ static int OpenEncoder(vlc_object_t *p_this)
>>      if (!sys)
>>          return VLC_ENOMEM;
>>
>> +    sys->buffer = NULL;
>> +    sys->enc = NULL;
>> +
>>      enc->pf_encode_audio = Encode;
>>      enc->fmt_in.i_codec = VLC_CODEC_FL32;
>>      enc->fmt_in.audio.i_rate = /* Only 48kHz */
>> @@ -584,8 +587,7 @@ static int OpenEncoder(vlc_object_t *p_this)
>>              &header))
>>      {
>>          msg_Err(enc, "Failed to prepare header.");
>> -        free(sys);
>> -        return VLC_ENOMEM;
>> +        goto error;
>>      }
>>
>>      /* needed for max encoded size calculation */
>> @@ -601,8 +603,8 @@ static int OpenEncoder(vlc_object_t *p_this)
>>      if (err != OPUS_OK)
>>      {
>>          msg_Err(enc, "Could not create encoder: error %d", err);
>> -        free(sys);
>> -        return VLC_EGENERIC;
>> +        sys->enc = NULL;
>> +        goto error;
>>      }
>>
>>      /* TODO: vbr, bitrate, fec */
>> @@ -612,11 +614,7 @@ static int OpenEncoder(vlc_object_t *p_this)
>>      enc->p_sys = sys;
>>      sys->buffer = malloc(OPUS_FRAME_SIZE * header.channels *
>> sizeof(float));
>>      if (!sys->buffer)
>> -    {
>> -        opus_multistream_encoder_destroy(sys->enc);
>> -        free(sys);
>> -        return VLC_ENOMEM;
>> -    }
>> +        goto error;
>>
>>      sys->i_nb_samples = 0;
>>
>> @@ -634,10 +632,7 @@ static int OpenEncoder(vlc_object_t *p_this)
>>                            &enc->fmt_out.i_extra, &header))
>>      {
>>          msg_Err(enc, "Failed to write header.");
>> -        free(sys->buffer);
>> -        opus_multistream_encoder_destroy(sys->enc);
>> -        free(sys);
>> -        return VLC_ENOMEM;
>> +        goto error;
>>      }
>>
>>      if (sys->i_samples_delay > 0)
>> @@ -646,12 +641,7 @@ static int OpenEncoder(vlc_object_t *p_this)
>>              enc->fmt_out.audio.i_channels;
>>          sys->padding = block_Alloc(padding_samples * sizeof(float));
>>          if (!sys->padding)
>> -        {
>> -            free(sys->buffer);
>> -            opus_multistream_encoder_destroy(sys->enc);
>> -            free(sys);
>> -            return VLC_ENOMEM;
>> -        }
>> +            goto error;
>>          sys->padding->i_nb_samples = sys->i_samples_delay;
>>          float *pad_ptr = (float *) sys->padding->p_buffer;
>>          memset(pad_ptr, 0, padding_samples * sizeof(float));
>> @@ -662,6 +652,13 @@ static int OpenEncoder(vlc_object_t *p_this)
>>      }
>>
>>      return VLC_SUCCESS;
>> +
>> +error:
>> +    if (sys->enc)
>> +        opus_multistream_encoder_destroy(sys->enc);
>> +    free(sys->buffer);
>> +    free(sys);
>> +    return VLC_EGENERIC;
>>  }
>>
> 
> The only issue I see here is that we're no longer distinguishing between
> out of memory errors and generic error cases. Maybe that distinction isn't
> so important in this situation.

I am not sure if it is important as the error value of modules' Open()
has never been used, but feel free to rename error to enomem and revert
this change for the VLC_EGENERIC case.

> In any case, the patchset looks good, should i resend with your changes
> merged?

Yes please

> Best,
> Tristan