[FFmpeg-devel] [PATCH] lavu: header and documentation for AVWriter

[Nicolas George]

Stefano Sabatini (12022-09-01):
> serializeS

Locally fixed.

> This "something" is vague and can be confused with the "something" in
> "av_something_write", maybe something as:
> when presented with invalid arguments, if the flag is not set the
> called method should leave...

No, the first interpretation was exactly what it meant, it is the same
"something". For exemple, with "something" = "sample format",
av_get_sample_fmt_name(42) returns NULL, av_sample_fmt_write(wr, 42, 0)
writes nothing but av_sample_fmt_write(wr, 42, AV_WRITER_FALLBACK)
writes "invalid".

> what variable? => in the AVWriter data?

Locally changed to:

+ * The buffer is initially kept in the AVWriter data itself, it switches to
+ * heap allocation if it grows too large. In that case,
+ * av_writer_get_error() can return AVERROR(ENOMEM) if the allocation fails.

> > +char *av_dynbuf_writer_get_buffer(AVWriter wr, size_t size, size_t *rsize);

> > + * size must be <= *rsize on a previous call to
> > + * av_dynbuf_writer_get_buffer().
> what is *rsize?

One of the arguments to av_dynbuf_writer_get_buffer(), explained just
above.

> is called => if called

Fixed.

> > +#define av_buf_writer_array(array) \
> > +    av_buf_writer(array, sizeof (array))
> av_buf_writer_from_array?

I would rather keep the name concise, but I will change if other agree
your version is better or if you insist.

> > +AVWriter av_stdio_writer(FILE *out);
> av_file_writer() ?

That would be ambiguous, it would suggest writing to an actual file.

> I'd use a verb for consistency/clarify.
> notify_impossibility or notify_impossible_op?

Locally changed to:

-    void (*impossible)(AVWriter wr, const char *message);
+    void (*notify_impossible)(AVWriter wr, const char *message);

> > +     * If *size is returned < min_size, data may get discarded.
> If the returned *size is < min_size... ?

Locally changed to:

-     * If *size is returned < min_size, data may get discarded.
+     * If the returned *size is < min_size, data may get discarded.

> > +     * size is guaranteed <= the size value returned by get_buffer().
> size is or should be guaranteed?

Is. The documentation is written for somebody who implements the
methods, and it tells them they do not have to bother checking size
here (although an assert would not be bad in this kind of situation). I
added a paragraph in the introduction:

  * Applications that want to implement other kinds of AVWriter
  * need to create a structure with some of these methods implemented.
  *
+ * These methods should only be called directly by the implementation of
+ * AVWriter. Their documentation is written from the point of view of
+ * implementing them.
+ *
  * Every type of AVWriter is supposed to have a pair of functions:
  * av_something_writer_get_methods() returns the methods for that type.
  * av_something_writer_checks() returns 1 if the writer is of that type.

Thanks for the review. I do not think it is necessary to send an updated
patch for these changes, is it?

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220901/4686346c/attachment.sig>