[FFmpeg-devel] [PATCH] FLAC parser
Justin Ruggles
justin.ruggles
Mon Nov 22 23:19:23 CET 2010
On 11/16/2010 08:50 AM, Michael Chinen wrote:
> On Mon, Nov 15, 2010 at 10:57 PM, Justin Ruggles
> <justin.ruggles at gmail.com> wrote:
>> Michael Chinen wrote:
>>
>>> Hi,
>>>
>>> On Thu, Nov 11, 2010 at 1:05 AM, Justin Ruggles
>>> <justin.ruggles at gmail.com> wrote:
>>>> Hi,
>>>>
>>>> Michael Chinen wrote:
>>>>
>>>>> On Sun, Oct 24, 2010 at 3:27 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>>>> On Sat, Oct 23, 2010 at 04:08:59AM +0200, Michael Chinen wrote:
>>>>>> [...]
>>>>>>> +static int find_new_headers(FLACParseContext *fpc, int search_start)
>>>>>>> +{
>>>>>>> + FLACHeaderMarker *end;
>>>>>>> + int search_end, size = 0, read_len, temp;
>>>>>>> + uint8_t *buf;
>>>>>>> + fpc->nb_headers_found = 0;
>>>>>>> +
>>>>>>> + /* Search for a new header of at most 16 bytes. */
>>>>>>> + search_end = av_fifo_size(fpc->fifo_buf) - (MAX_FRAME_HEADER_SIZE - 1);
>>>>>>> + read_len = search_end - search_start + 1;
>>>>>>> + buf = flac_fifo_read(fpc, search_start, &read_len);
>>>>>>> + size = find_headers_search(fpc, buf, read_len, search_start);
>>>>>>> + search_start += read_len - 1;
>>>>>>> +
>>>>>>> + /* if we hit the fifo end we need a wrap buffer for the two byte wrap around */
>>>>>>> + if (search_start != search_end) {
>>>>>>> + buf = flac_fifo_read_wrap(fpc, search_start, 2,
>>>>>>> + &fpc->wrap_buf, &fpc->wrap_buf_allocated_size);
>>>>>>> + temp = find_headers_search(fpc, buf, 2, search_start);
>>>>>>> + size = FFMAX(size, temp);
>>>>>>> + search_start += 1;
>>>>>>> + }
>>>>>> the 2 bytes can be checked directly and find_headers_search_validate() be called
>>>>> ok, I added this.
>>>>>
>>>>>> also i leave further review to the flac maintainer
>>>>>>
>>>>>> a bit of further simplification and cleanup would be nice though
>>>>> Also removed the error codes and used offsets for logs where applicable.
>>>> I think maybe AV_LOG_MAX should be added, which would match the highest
>>>> log level in case more levels above AV_LOG_DEBUG are added. Otherwise
>>>> your code would stop working as intended.
>>>
>>> I can't find a reference to AV_LOG_MAX with grep and AV_LOG_DEBUG is
>>> the largest value in log.h.
>>
>> Hence, "AV_LOG_MAX should be added". That's my suggested solution anyway.
>
> Ah, I thought you meant 'add' as in offset.
>
> added as new 0001 patch.
As for the AV_LOG_MAX patch, I think it's good but Michael N. needs to
ok it.
Patch 2 and 3 are fine.
Patch 4:
I would add something in the documentation of flac_fifo_read_wrap()
about what function in fifo.c it is based on. That way if someone
wonders about that memory barrier comment they will know where it came from.
I think the added parser warrants a minor bump to lavc and an entry in
the ChangeLog.
The reset looks ok.
-Justin
More information about the ffmpeg-devel
mailing list