[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

Hi,
On Nov 20, 2008, at 4:36 PM, Reynaldo H. Verdejo Pinochet wrote:

> Hello
>
> Reynaldo H. Verdejo Pinochet wrote:
>>
>> and then qcelp_bits_per_rate was exactly what its name would made
>> you think it was. Can you remind me why did you change that enum's
>> order? If that change is avoided this one would not be needed.
>
> I searched the thread and discovered you had changed the enum
> quite a few times to account for troubles with the original
> change, and that original change was totally your idea...
>
>> I reorder the enum on the 09/07/2008, way before submitting my first
>> patch to
>>     RATE_FULL   = 0,
>>     RATE_HALF   = 1,
>>     RATE_QUARTER= 2,
>>     RATE_OCTAVE = 3,
>>     I_F_Q,          /*!< insufficient frame quality */
>>     BLANK,
>>     RATE_UNKNOWN
>> to
>>     SILENCE = 0,
>>     RATE_OCTAVE,
>>     RATE_QUARTER,
>>     RATE_HALF,
>>     RATE_FULL,
>>     I_F_Q,          /*!< insufficient frame quality */
>>     RATE_UNKNOWN
>> in order to reflect the rate byte in the QCELP frame.
>
> I think that change was the wrong one. Can you please make
> sure its really needed? I personally don't think so.


this original changes was made so that
         if((claimed_rate ==  0 && q->rate != BLANK       ) ||
            (claimed_rate ==  1 && q->rate != RATE_OCTAVE ) ||
            (claimed_rate ==  2 && q->rate != RATE_QUARTER) ||
            (claimed_rate ==  3 && q->rate != RATE_HALF   ) ||
            (claimed_rate ==  4 && q->rate != RATE_FULL   ))

would be more readable and replaced by something like

	if (claimed_rate != q->rate)

it had a small impact on the code and make it look more readable to me,
since a newbie to the code, i did not have to know that the hard coded 0
was corresponding to value of the first byte for a BLANK packet ...




>
>
>> and I changed on the 10/27/2008 to
>>     RATE_UNKNOWN = -2,
>>     I_F_Q,             /*!< insufficient frame quality */
>>     SILENCE,
>>     RATE_OCTAVE,
>>     RATE_QUARTER,
>>     RATE_HALF,
>>     RATE_FULL
>> when you asked me to change the
>> switch (framerate)
>>   case RATE_FULL:
>>   case RATE_QUARTER:
>>   case RATE_OCTAVE:
>> }
>> to (framerate >= RATE_QUARTER)
>>
>> After sending the patch round 10, I also added a check to make sure
>> the buffer
>> contains enough data for the the frame to be decoded without reading
>> garbage.
>
> I dont think that change is needed neither as that should be
> guaranteed by your demuxer - parser(?) chain.

In order to read the two samples h263.mov and blue_earth.mov,
we need to look at the rate byte in the frame (as the spec describes)
and not just rely on the buffer_size
since for those files, the buffer_size is always 35 but they contains
RATE_FULL, RATE_HALF, RATE_QUARTER and RATE_OCTAVE.

Since we have to use the rate byte to determine the the type of frame,
I thought checking the buffer has enough data for the corresponding
rate would be more robust.

My check is ATM clearly wrong as Michael pointed out.


>
>
>> Futhermore, I am dropping RATE_UNKNOWN and replace it by I_F_Q
>>  since the specification says at TIA/EIA/IS-733 2.4.8.7.1:
>> "If the received packet type cannot be satisfactorily determined, the
>> multiplex sublayer
>> informs the receiving speech codec of an erasure (see 2.3.2.2)."
>>
>> attached the round 11:
>
> The idea of had a RATE_UNKNOWN was to be able to reflect an state.
> Nothing wrong with you geting rid of it though but then again I'm
> not sure you're gaining anything out of it.

just trying to abide by the specs


have a great day/evening

Kenan