diff mbox series

[FFmpeg-devel,08/10] avformat/avformat: Clarify documentation of av_interleaved_write_frame()

Message ID 20200331123745.6461-9-andreas.rheinhardt@gmail.com
State New
Headers show
Series libavformat/mux patches
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt March 31, 2020, 12:37 p.m. UTC
The earlier documentation claimed that av_interleaved_write_frame()
always orders by dts, which is not necessarily true when using muxers
with custom interleavement functions or the audio_preload option.

Furthermore, the documentation stated that libavformat takes ownership
of the reference of the provided packet (if it is refcounted) and that
the caller may not access the data through this reference after the
function returns. This suggests that the returned packet is not blank,
but instead still contains some set, but invalid fields, which implies
that it would be dangerous to unreference this packet again.

But this is not true: av_interleaved_write_frame()'s actual behaviour
is to always output blank packet (even on error). This commit documents
this fact so that callers know that they can directly reuse this packet.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 doc/APIchanges         |  4 ++++
 libavformat/avformat.h | 15 +++++++--------
 2 files changed, 11 insertions(+), 8 deletions(-)

Comments

Marton Balint March 31, 2020, 10:19 p.m. UTC | #1
On Tue, 31 Mar 2020, Andreas Rheinhardt wrote:

> The earlier documentation claimed that av_interleaved_write_frame()
> always orders by dts, which is not necessarily true when using muxers
> with custom interleavement functions or the audio_preload option.
>
> Furthermore, the documentation stated that libavformat takes ownership
> of the reference of the provided packet (if it is refcounted) and that
> the caller may not access the data through this reference after the
> function returns. This suggests that the returned packet is not blank,
> but instead still contains some set, but invalid fields, which implies
> that it would be dangerous to unreference this packet again.
>
> But this is not true: av_interleaved_write_frame()'s actual behaviour
> is to always output blank packet (even on error). This commit documents
> this fact so that callers know that they can directly reuse this packet.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> doc/APIchanges         |  4 ++++
> libavformat/avformat.h | 15 +++++++--------
> 2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index f1d7eac2ee..31dc6c6c16 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
> 
> API changes, most recent first:
> 
> +2020-03-31 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
> +  av_interleaved_write_frame() now guarantees to always return
> +  blank packets, even on failure.

Bump at least micro version if you add a changelog entry.

> +
> 2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>   av_read_frame() now guarantees to handle uninitialized input packets
>   and to return refcounted packets on success.
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 8f7466931a..9669ded1cd 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -2596,7 +2596,7 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt);
>  * Write a packet to an output media file ensuring correct interleaving.
>  *
>  * This function will buffer the packets internally as needed to make sure the
> - * packets in the output file are properly interleaved in the order of
> + * packets in the output file are properly interleaved, usually ordered by
>  * increasing dts. Callers doing their own interleaving should call
>  * av_write_frame() instead of this function.
>  *
> @@ -2609,10 +2609,10 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt);
>  *            <br>
>  *            If the packet is reference-counted, this function will take
>  *            ownership of this reference and unreference it later when it sees
> - *            fit.
> - *            The caller must not access the data through this reference after
> - *            this function returns. If the packet is not reference-counted,
> - *            libavformat will make a copy.
> + *            fit. If the packet is not reference-counted, libavformat will
> + *            make a copy.
> + *            The returned packet will be blank (as if returned from
> + *            av_packet_alloc()), even on error.
>  *            <br>
>  *            This parameter can be NULL (at any time, not just at the end), to
>  *            flush the interleaving queues.
> @@ -2628,10 +2628,9 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt);
>  *            The dts for subsequent packets in one stream must be strictly
>  *            increasing (unless the output format is flagged with the
>  *            AVFMT_TS_NONSTRICT, then they merely have to be nondecreasing).
> - *            @ref AVPacket.duration "duration") should also be set if known.
> + *            @ref AVPacket.duration "duration" should also be set if known.
>  *
> - * @return 0 on success, a negative AVERROR on error. Libavformat will always
> - *         take care of freeing the packet, even if this function fails.
> + * @return 0 on success, a negative AVERROR on error.
>  *
>  * @see av_write_frame(), AVFormatContext.max_interleave_delta

Otherwise LGTM.

Thanks,
Marton
Andreas Rheinhardt April 18, 2020, 11:27 p.m. UTC | #2
Marton Balint:
> 
> 
> On Tue, 31 Mar 2020, Andreas Rheinhardt wrote:
> 
>> The earlier documentation claimed that av_interleaved_write_frame()
>> always orders by dts, which is not necessarily true when using muxers
>> with custom interleavement functions or the audio_preload option.
>>
>> Furthermore, the documentation stated that libavformat takes ownership
>> of the reference of the provided packet (if it is refcounted) and that
>> the caller may not access the data through this reference after the
>> function returns. This suggests that the returned packet is not blank,
>> but instead still contains some set, but invalid fields, which implies
>> that it would be dangerous to unreference this packet again.
>>
>> But this is not true: av_interleaved_write_frame()'s actual behaviour
>> is to always output blank packet (even on error). This commit documents
>> this fact so that callers know that they can directly reuse this packet.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> doc/APIchanges         |  4 ++++
>> libavformat/avformat.h | 15 +++++++--------
>> 2 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index f1d7eac2ee..31dc6c6c16 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -15,6 +15,10 @@ libavutil:     2017-10-21
>>
>> API changes, most recent first:
>>
>> +2020-03-31 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>> +  av_interleaved_write_frame() now guarantees to always return
>> +  blank packets, even on failure.
> 
> Bump at least micro version if you add a changelog entry.
> 
>> +
>> 2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
>>   av_read_frame() now guarantees to handle uninitialized input packets
>>   and to return refcounted packets on success.
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index 8f7466931a..9669ded1cd 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -2596,7 +2596,7 @@ int av_write_frame(AVFormatContext *s, AVPacket
>> *pkt);
>>  * Write a packet to an output media file ensuring correct interleaving.
>>  *
>>  * This function will buffer the packets internally as needed to make
>> sure the
>> - * packets in the output file are properly interleaved in the order of
>> + * packets in the output file are properly interleaved, usually
>> ordered by
>>  * increasing dts. Callers doing their own interleaving should call
>>  * av_write_frame() instead of this function.
>>  *
>> @@ -2609,10 +2609,10 @@ int av_write_frame(AVFormatContext *s,
>> AVPacket *pkt);
>>  *            <br>
>>  *            If the packet is reference-counted, this function will take
>>  *            ownership of this reference and unreference it later
>> when it sees
>> - *            fit.
>> - *            The caller must not access the data through this
>> reference after
>> - *            this function returns. If the packet is not
>> reference-counted,
>> - *            libavformat will make a copy.
>> + *            fit. If the packet is not reference-counted,
>> libavformat will
>> + *            make a copy.
>> + *            The returned packet will be blank (as if returned from
>> + *            av_packet_alloc()), even on error.
>>  *            <br>
>>  *            This parameter can be NULL (at any time, not just at the
>> end), to
>>  *            flush the interleaving queues.
>> @@ -2628,10 +2628,9 @@ int av_write_frame(AVFormatContext *s, AVPacket
>> *pkt);
>>  *            The dts for subsequent packets in one stream must be
>> strictly
>>  *            increasing (unless the output format is flagged with the
>>  *            AVFMT_TS_NONSTRICT, then they merely have to be
>> nondecreasing).
>> - *            @ref AVPacket.duration "duration") should also be set
>> if known.
>> + *            @ref AVPacket.duration "duration" should also be set if
>> known.
>>  *
>> - * @return 0 on success, a negative AVERROR on error. Libavformat
>> will always
>> - *         take care of freeing the packet, even if this function fails.
>> + * @return 0 on success, a negative AVERROR on error.
>>  *
>>  * @see av_write_frame(), AVFormatContext.max_interleave_delta
> 
> Otherwise LGTM.
> 
This is unfortunately not LGTM. There is a mismatch between the
bsf API and this API: The former treats empty packets (defined as
packets for which data and side_data_elems vanish) as EOF, the latter
doesn't.* And if somebody sent such a packet that is not blank (as if
freshly allocated/unrefed), then this packet would not be blank on
return and the bitstream filter would reject any future packets. The
first of these problems made me refrain from applying this and the
follow-up patch that removes av_packet_unref() calls. But of course the
second problem is the more serious one.

- Andreas

*: What counts as eof for a bsf changed over time: When the new packet
based bsf API was introduced in 33d18982, NULL packets and packets
without data were eof, but only the former was documented as such.
7d5501be and bfdca87a ended this discrepancy by forbidding packets only
treating NULL as eof and forbidding packets without payload (i.e.
packets needed to contain data or side data). 844a115c and f92e1af8
removed the ban on packets without payload (no data and no side data)
and treated them as eof, but without documenting said fact. 41b05b84
documented the current behaviour.
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index f1d7eac2ee..31dc6c6c16 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,10 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2020-03-31 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
+  av_interleaved_write_frame() now guarantees to always return
+  blank packets, even on failure.
+
 2020-03-29 - xxxxxxxxxx - lavf 58.42.100 - avformat.h
   av_read_frame() now guarantees to handle uninitialized input packets
   and to return refcounted packets on success.
diff --git a/libavformat/avformat.h b/libavformat/avformat.h
index 8f7466931a..9669ded1cd 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -2596,7 +2596,7 @@  int av_write_frame(AVFormatContext *s, AVPacket *pkt);
  * Write a packet to an output media file ensuring correct interleaving.
  *
  * This function will buffer the packets internally as needed to make sure the
- * packets in the output file are properly interleaved in the order of
+ * packets in the output file are properly interleaved, usually ordered by
  * increasing dts. Callers doing their own interleaving should call
  * av_write_frame() instead of this function.
  *
@@ -2609,10 +2609,10 @@  int av_write_frame(AVFormatContext *s, AVPacket *pkt);
  *            <br>
  *            If the packet is reference-counted, this function will take
  *            ownership of this reference and unreference it later when it sees
- *            fit.
- *            The caller must not access the data through this reference after
- *            this function returns. If the packet is not reference-counted,
- *            libavformat will make a copy.
+ *            fit. If the packet is not reference-counted, libavformat will
+ *            make a copy.
+ *            The returned packet will be blank (as if returned from
+ *            av_packet_alloc()), even on error.
  *            <br>
  *            This parameter can be NULL (at any time, not just at the end), to
  *            flush the interleaving queues.
@@ -2628,10 +2628,9 @@  int av_write_frame(AVFormatContext *s, AVPacket *pkt);
  *            The dts for subsequent packets in one stream must be strictly
  *            increasing (unless the output format is flagged with the
  *            AVFMT_TS_NONSTRICT, then they merely have to be nondecreasing).
- *            @ref AVPacket.duration "duration") should also be set if known.
+ *            @ref AVPacket.duration "duration" should also be set if known.
  *
- * @return 0 on success, a negative AVERROR on error. Libavformat will always
- *         take care of freeing the packet, even if this function fails.
+ * @return 0 on success, a negative AVERROR on error.
  *
  * @see av_write_frame(), AVFormatContext.max_interleave_delta
  */