On Thu, 2 Apr 2015, Vittorio Giovara wrote:
On Thu, Apr 2, 2015 at 2:17 PM, Martin Storsjö <[email protected]> wrote:
The previous documentation was very vague and almost misleading.
---
libavcodec/internal.h | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index a681329..c3dfaa1 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -34,11 +34,16 @@
#include "config.h"
/**
- * Codec is thread safe.
+ * The codec doesn't touch global variables in the init function, allowing
+ * running the init function without locking any global mutexes.
How about "The codec doesn't access any global variable in the init
variables, not variable
function, allowing to call the init function without locking any
global mutexes."?
Sure
*/
#define FF_CODEC_CAP_INIT_THREADSAFE (1 << 0)
/**
- * Codec cleans up memory on init failure.
+ * The codec allows (and requires) calling the close function for deallocation
+ * even if the init function returned a failure. (Previously, without this
+ * capability flag, a codec does such cleanup internally when returning
+ * failures from the init function and does not expect the close function
+ * to be called at all.)
Not sure about this, is it the right place for documenting the current
behaviour?
Yes, I think so, because otherwise it is hard to know what exactly it
means unless you spell it out what the difference is to when it is not
set.
Also, previously, when init returned a failure, it could expect that close
would _not_ be called (it could leave dangling pointers in the context
without any problem, since close wouldn't be called). Now when the
framework calls it, it's not just all fine and dandy, this instead
strictly require you to make sure it's ok to call close after init (not
leaving any dangling pointers) - this should be emphasized, because it is
exactly the one thing that one needs to keep track of when adding this
flag to existing codecs.
I think something along the lines of "The codec automatically calls
the close function for deallocation should the init function fail for
any reason." would be better.
If you think it's an improvement the second part imho could be slightly
reworded as "Otherwise, without this capability flag, a codec does such
cleanup within the init function and does not expect the close function
to be called at all."
What I strongly dislike in your wording is the use of the word "codec".
When calling avcodec_open2() and it fails, there's three different layers
involved:
1. The user code, calling avcodec_open2()
2. The lavc generic code in utils.c, implementing avcodec_open2()
3. The individual codec itself, implementing the init/close/decode/encode
functions
Now when you say "The codec automatically calls the close function", you
mean that layer 2 calls close when necessary. Now think of who will read
this documentation - the developer of the user code in layer 1 should not
need to read this, because the behaviour outwards is the same regardless -
if avcodec_open2() failed, you don't need to (and shouldn't) call
avcodec_close. This documentation of the flags is intended for the person
implementing layer 3 here, and then the word "the codec" sounds like it is
the individual codec implementation itself that does it, not the
framework.
Now think again of what the documentation originally said: "Codec cleans
up memory on init failure." So when I'm implementing the G2M7 decoder or
whatever, this reads like "my init function cleans up memory on init
failures, so I can set this flag", while it actually is the total
opposite.
The text that you suggested,
The codec automatically calls the close function for deallocation should
the init function fail for any reason. Otherwise, without this
capability flag, a codec does such cleanup within the init function and
does not expect the close function to be called at all.
mixes what you mean with "codec" to first mean that the _framework_
automatically calls close, while you in the second instance mean that the
individual codec code does it within the init function.
Also I prefer writing this from the point of view of the codec
implementation, i.e., what does my codec code guarantee to the framework
(because the consumer of this flag is the framework), not what the
framework+codec together do when this is set.
My updated suggested text is this:
The codec allows calling the close function for deallocation even if the
init function returned a failure. (Previously, without this capability
flag, a codec does such cleanup internally when returning failures from
the init function and does not expect the close function to be called at
all.)
Compared to the previous version, I removed the parentheses saying that it
requires calling close since that's not strictly true.
I'm open to other alternative wordings as well, but they should be crystal
clear to the implementer of a codec what they imply.
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
