On 13.06.2015 22:46, Luca Barbato wrote: > On 13/06/15 20:19, Andreas Cadhalpun wrote: >> On 13.06.2015 14:15, Luca Barbato wrote: >>> On 13/06/15 13:06, Andreas Cadhalpun wrote: >>>> On 12.06.2015 23:05, Vittorio Giovara wrote: >>>>> Do you have a test case where using the API exposes some kind of problem? >>>> >>>> Yes. As the commit message says, it can crash the demuxing_decoding >>>> example [1]. >>> >>> Bad examples should be fixed, I couldn't see the problem in the examples >>> we have here... >> >> Sigh, I'll try to explain it one last time: >> The demuxing_decoding example is not the problem, because it uses the API in >> the documented way. > > That is what you said and I was agreeing with that, then I read the > documentation and the problem is misreading it (thus should be clarified).
Clarifying the documentation is always good, but I doubt it would help much in this case, because it has been documented differently for a long time. >> What the h264 decoder does is not consistent with the API documentation and >> thus causes unspecified behavior in programs using the API, which can range >> from harmless to arbitrary code execution. > > It is the kind of issues that come from misuse of functions and fields. Or from libraries that don't act according to their API documentation. >> Thus the h264 decoder should be fixed. > > I'm afraid not, let's have a look at the documentation. Well, then this would be one of those bugs that gets fixed in FFmpeg, but not in Libav. > /* video only */ > /** > * picture width / height. > * - encoding: MUST be set by user. > * - decoding: May be set by the user before opening the decoder if > known e.g. > * from the container. Some decoders will require the > dimensions > * to be set by the caller. During decoding, the decoder may > * overwrite those values as required. > */ > int width, height; > > In other words. > > Those values MUST set by the user for encoding, MAY be set by the user > for decoding if known, the decoder may change them as it sees fit. It implies that the decoder makes sure that the field is correct, even by overwriting the user supplied value, if that is required. So the user shouldn't rely on the value he sets, because the decoder might produce differently sized frames. And before commit 2d6edb2b7 this just said: * - decoding: Set by libavcodec. * Note: For compatibility it is possible to set this instead of * coded_width/height before decoding. It doesn't mention at all that the field can be wrong. > /** > * Pixel format, see AV_PIX_FMT_xxx. > * May be set by the demuxer if known from headers. > * May be overriden by the decoder if it knows better. > * - encoding: Set by user. > * - decoding: Set by user if known, overridden by libavcodec if known > */ > enum AVPixelFormat pix_fmt; > > This should be clarified some more, but it is again along the lines of > "do not expect it to be present, please set it if you know what it is, > since the decoder might not.". This one is very clear: If libavcodec knows the pixel format, it sets this field. The h264 decoder knows the correct pixel format and thus should set this field correctly. > I'd clarify those fields are not per-frame reliable if reliable at all. > > I have no responsibility for wrong example code belonging to another > project. > > You can have similar issues with pretty much any other codec supporting > frame parameters changes Do you have any sample that shows a similar issue for another codec? > and threads Threading is not the problem, because the issue in the h264 decoder can also be seen, when using only one decoding thread. > and "fixing" it to match the bad example isn't exactly trivial. The h264 decoder is the "bad example" producing inconsistencies. I'm not aware of any other decoder doing that. > Well thinking about it, it is trivial now, you duplicate those fields > internally and you update the user-facing ones to match the last > outputted frame. > > It won't work at ALL with the new API and I'm not really fond of the idea. New API is not the problem here, because that can be properly documented, before people start using it. Best regards, Andreas _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
