[FFmpeg-devel] [PATCH] FLAC parser

[Michael Chinen]

Hi,

On Fri, Aug 13, 2010 at 6:38 PM, Justin Ruggles
<justin.ruggles at gmail.com> wrote:
> Hi,
>
> I'm writing this from my work computer, so I don't have a way to reply
> directly to the list. ?I'm sending it only to you instead so it
> doesn't break the thread. ?Sorry about not being able to quote
> properly either... ?Feel free to include the full text in your reply
> to the list so others can see it.
>
> Also, I'm going out-of-town this weekend so I won't be able to reply
> again until Monday.
>
> +static void update_sequences(FLACParseContext *fpc, int start_index,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? int start_distance, FLACHeaderMarker *end)
> +{
> + ? ?int distance ? ? ? ? ?= start_distance - 1;
> + ? ?FLACHeaderMarker *mid = fpc->headers;
> +
> + ? ?while (start_index-- > 0)
> + ? ? ? ?mid = mid->next;
> +
> + ? ?update_sequences_header(fpc, mid, end, distance);
> +}
>
> distance variable not needed.

removed.

>
> + ? ? ? ? ? ?/* The header may have already been found in the last search. This
> + ? ? ? ? ? ? ? happens when header size is less than
> MAX_FRAME_HEADER_SIZE. ? */
> + ? ? ? ? ? ?if (end_offset >= i) {
> + ? ? ? ? ? ? ? ?av_log(fpc->avctx, AV_LOG_DEBUG,
> + ? ? ? ? ? ? ? ? ? ? ? "found header a second time, skipping\n");
> + ? ? ? ? ? ? ? ?continue;
> + ? ? ? ? ? ?}
>
> shouldn't this be:
> if (end_offset == i)

I've convinced myself that this case will not happen so I removed it.

Looking at this also lead to finding some bugs that I've fixed and tested:
-A case where the parser would do the wrong thing if seeked to a
position near the end of the file with less than the minimum number of
headers
-A missing case where a header lies in the last 16 bytes of the file.
-The chains were being set wrong.

>
> + ? ?} else if (fpc->best_header) {
> + ? ? ? ?/* No end frame no need to delete the buffer; probably eof */
> + ? ? ? ?FLACHeaderMarker *curr = fpc->headers, *temp;
> + ? ? ? ?while (curr != fpc->best_header) {
> + ? ? ? ? ? ?temp = curr->next;
> + ? ? ? ? ? ?av_free(curr);
> + ? ? ? ? ? ?curr = temp;
> + ? ? ? ?}
> + ? ? ? ?fpc->headers ? ? = fpc->best_header->next;
> + ? ? ? ?av_free(fpc->best_header);
> + ? ? ? ?fpc->best_header = NULL;
> + ? ?}
>
> av_freep(&fpc->best_header);
> then you don't need to set it to NULL.

Done.

>
> + ? ?if (!fpc->best_header) {
> + ? ? ? ?FLACHeaderMarker *curr = fpc->headers;
> + ? ? ? ?int best_score = FLAC_HEADER_NOT_SCORED_YET;
> +
> + ? ? ? ?while (curr) {
> + ? ? ? ? ? ?if (curr->max_score > best_score) {
> + ? ? ? ? ? ? ? ?fpc->best_header = curr;
> + ? ? ? ? ? ? ? ?best_score ? ? ? = curr->max_score;
> + ? ? ? ? ? ?}
> + ? ? ? ? ? ?curr = curr->next;
> + ? ? ? ?}
> + ? ?}
>
> it appears to me that fpc->best_header is always NULL at the point of
> this check. am i missing something?

score_headers used to set it to non-NULL, but this was code
duplication, so I refactored some things and got rid of the if
statement.

>
> + ? ?if (fpc->best_header) {
> + ? ? ? ?if (fpc->best_header->offset > 0) {
> + ? ? ? ? ? ?/* Output a junk frame. */
> + ? ? ? ? ? ?av_log(avctx, AV_LOG_DEBUG, "Junk frame till offset %i\n",
> + ? ? ? ? ? ? ? ? ? fpc->best_header->offset);
> +
> + ? ? ? ? ? ?/* Set frame_size to 0. It is unknown or invalid in a
> junk frame. */
> + ? ? ? ? ? ?avctx->frame_size ? ? ?= 0;
> + ? ? ? ? ? ?*poutbuf ? ? ? ? ? ? ? = fpc->buffer;
> + ? ? ? ? ? ?*poutbuf_size ? ? ? ? ?= fpc->best_header->offset;
> + ? ? ? ?}
> + ? ? ? ?fpc->best_header_valid = 1;
> + ? ? ? ?if(!buf_size)
> + ? ? ? ? ? ?return get_best_header(fpc, avctx, poutbuf, poutbuf_size);
> + ? ?}
>
> too much space before the 3 '='.
> missing space after 'if'

done.

>
> +handle_error:
> + ? ?*poutbuf ? ? ? ? ?= NULL;
> + ? ?*poutbuf_size ? ? = 0;
> + ? ?return buf_size;
>
> too much space before the '='.

done.

>
> Thank you for being so patient and persistent in getting this patch approved.

Thank you for the review!

Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-FLAC-Parser.patch
Type: application/octet-stream
Size: 39089 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100814/03baa527/attachment.obj>