[Bug 751334] FLAC: memory leak on a specific media file

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=751334

--- Comment #28 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> ---
(In reply to Vineeth from comment #27)
> This will work only with decodebin right?
> If i still use gst-launch-1.0 ... ! flacparse ! flacdec ! ....
> it will still fail..

Obviously.

> 
> One thing i dont understand here is, in flacparse, the header is being set
> to caps, which is being used in set_format, and the same header is again
> pushed to baseparse, which means it is again being processed by the libflac.
> Do we really need to handle the header twice?

Streamheader is "special". Parser will extra stream header as a convenience
from a stream of type byte-stream. Though, it won't remove it because otherwise
it would make the stream "incompatbile" with what is is supposed to be. Decoder
for these format have the option of looking at the stream header, and skipping
these from the stream, or ignoring the streamheader and only dealing with the
stream. This is just a convenience as decoder API can vary a lot. There is also
the benefit that whenever the header changes, caps will change, so
renegotiation is made easier (kind of).

> 
> > Now, *if* we want to support this broken file (because we know it's slightly
> > broken), the solution should resides in flacparse. The parser should detect
> > that the stream header has twice the stream info (hence the stream header is
> > like two headers in one). Base on that, the parser could "fix" the header by
> > only keeping the most recent one. Again, this is all about if we care
> > supporting this broken file.
> 
> This file does not actually have two valid streaminfo. The first streaminfo
> is valid. While the second is not valid(it just has type as streaminfo while
> infact it is not) and it throws errors while parsing.

So maybe the workaround for flacparse is to validate the stream info. Then it
would figure-out that the second one is incomplete and skip it. The thing with
stream errors is that you can't assume that first is always the right one, or
second is always the right one.

> There is already code in flacparse to check this scenario and just ignore
> the other parts of the header. This is the one set to streamheader and which
> libflac is not able to process..

I think libflac should try and remove the unusable part (if we want to not say
this stream is broken, we don't support it).

> Now i did some simple experiments to determine what does libflac accept as a
> valid metadata.
> In case of a flac header, the end of header is determined by 
> is_last = ((map.data[0] & 0x80) == 0x80)
> so in the present case, since we are truncating the header and passing it to
> libflac it fails.
> Now when i try not to truncate and append all header till is_last is true,
> it still fails because, there are two stream data of type streaminfo.
> 
> While parsing in flacparse, if i just remove the second part of streaminfo
> and add everything else till is_last is true, to streamheader, this is being
> accepted by libflac as a valid metadata. But the size of this stream header
> is too big. 
> 
> But this fails when processing the actual frame :)... i guess this is
> because the header does not end properly with is_last(0x80) value, and we
> still keep searching in the actual data until we find something which we
> think is end of header...
> 
> 
> Not exactly sure if i am making any sense :)... too big a message!!!!
> But the patch i attached now does not make any drastic change, other than
> give libflac some more chances to prove itself, being worthy in good plugins
> :P

Yeah, I think I understand. You endup having to re-encode things, which is
falling in the too much wor imho (for a broken stream). Let do first thing
first. Ensure the flacdec fails properly and not leak. Initially we said to
simply return false, then you endup reverting everything touching the
streamheader. I still don't understand, since there must have been good reason
to more to using streamheader in the first place.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.