[FFmpeg-devel] [PATCH] FLAC parser

[Alexander Strange]

On Dec 6, 2010, at 2:45 PM, Justin Ruggles wrote:

> On 11/30/2010 09:28 PM, Michael Chinen wrote:
> 
>> Hi,
>> 
>> On Tue, Nov 30, 2010 at 1:59 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>> On Wed, Nov 24, 2010 at 11:44:34PM +0100, Michael Chinen wrote:
>>>> On Tue, Nov 23, 2010 at 10:25 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>>> On Mon, Nov 22, 2010 at 05:19:23PM -0500, Justin Ruggles wrote:
>>> [...]
>>>>>> As for the AV_LOG_MAX patch, I think it's good but Michael N. needs to
>>>>>> ok it.
>>>>> 
>>>>> probably a max of 127 should be more than enough
>>>> 
>>>> Ok, here is the updated patches with comment to flac_read_fifo_wrap,
>>>> changelog, and minor version bump.
>>>> 
>>>> Michael
>>> 
>>> [...]
>>>> @@ -109,6 +109,12 @@ typedef struct {
>>>> #define AV_LOG_DEBUG    48
>>>> 
>>>> /**
>>>> + * The maximum AV_LOG value.
>>>> + * This can be useful as an offset to silence the log messages
>>>> + **/
>>>> +#define AV_LOG_MAX      255
>>> 
>>> thats not 127 but rethinking i see no need for this, offseting by appropriate
>>> values for the code used is better because with several levels of calls there can
>>> be no globally correct max/min offset
>> 
>> Ok, here are the patches without AV_LOG_MAX.
>> 
> 
>> +static int frame_header_is_valid(AVCodecContext *avctx, const uint8_t *buf,
>> +                                 FLACFrameInfo *fi)
>> +{
>> +    GetBitContext gb;
>> +    init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8);
>> +    return !ff_flac_decode_frame_header(avctx, &gb, fi, AV_LOG_DEBUG);
>> +}
> 
> I could be wrong, but I think that based on what Michael N. said, you
> should add 127 here instead of AV_LOG_DEBUG.  The higher number is more
> appropriate for this code since you want it to always silence the output
> and you already know where and how the offset is being used.
> 
> Other than that, patch set still looks good.
> 
> -Justin

I'd hate to see this thread end, but if you think it's OK, could you apply it with that change?