Packet::write_interleaved should return Result<(), Error> instead of Result<bool, Error>
========================================================================================
https://github.com/zmwangx/rust-ffmpeg/blob/f1ccd4e69641dbd2be98e3ef38e670b592a103c1/src/codec/packet/packet.rs#L222-L236
`av_interleaved_write_frame` never returns 1, so `bool` in the signature doesn't make sense:
```c
/**
* 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
* increasing dts. Callers doing their own interleaving should call
* av_write_frame() instead of this function.
*
* Using this function instead of av_write_frame() can give muxers advance
* knowledge of future packets, improving e.g. the behaviour of the mp4
* muxer for VFR content in fragmenting mode.
*
* @param s media file handle
* @param pkt The packet containing the data to be written.
* <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.
* <br>
* This parameter can be NULL (at any time, not just at the end), to
* flush the interleaving queues.
* <br>
* Packet's @ref AVPacket.stream_index "stream_index" field must be
* set to the index of the corresponding stream in @ref
* AVFormatContext.streams "s->streams".
* <br>
* The timestamps (@ref AVPacket.pts "pts", @ref AVPacket.dts "dts")
* must be set to correct values in the stream's timebase (unless the
* output format is flagged with the AVFMT_NOTIMESTAMPS flag, then
* they can be set to AV_NOPTS_VALUE).
* 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.
*
* @return 0 on success, a negative AVERROR on error. Libavformat will always
* take care of freeing the packet, even if this function fails.
*
* @see av_write_frame(), AVFormatContext.max_interleave_delta
*/
int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt);
```
Need better error mapping for AVERROR(errno)
============================================
From `libavutil/error.h`:
```c
/* error handling */
#if EDOM > 0
#define AVERROR(e) (-(e)) ///< Returns a negative error code from a POSIX error code, to return from library functions.
#define AVUNERROR(e) (-(e)) ///< Returns a POSIX error code from a library function error return value.
#else
/* Some platforms have E* and errno already negated. */
#define AVERROR(e) (e)
#define AVUNERROR(e) (e)
#endif
#define FFERRTAG(a, b, c, d) (-(int)MKTAG(a, b, c, d))
#define AVERROR_BSF_NOT_FOUND FFERRTAG(0xF8,'B','S','F') ///< Bitstream filter not found
#define AVERROR_BUG FFERRTAG( 'B','U','G','!') ///< Internal bug, also see AVERROR_BUG2
#define AVERROR_BUFFER_TOO_SMALL FFERRTAG( 'B','U','F','S') ///< Buffer too small
#define AVERROR_DECODER_NOT_FOUND FFERRTAG(0xF8,'D','E','C') ///< Decoder not found
#define AVERROR_DEMUXER_NOT_FOUND FFERRTAG(0xF8,'D','E','M') ///< Demuxer not found
#define AVERROR_ENCODER_NOT_FOUND FFERRTAG(0xF8,'E','N','C') ///< Encoder not found
#define AVERROR_EOF FFERRTAG( 'E','O','F',' ') ///< End of file
#define AVERROR_EXIT FFERRTAG( 'E','X','I','T') ///< Immediate exit was requested; the called function should not be restarted
#define AVERROR_EXTERNAL FFERRTAG( 'E','X','T',' ') ///< Generic error in an external library
#define AVERROR_FILTER_NOT_FOUND FFERRTAG(0xF8,'F','I','L') ///< Filter not found
#define AVERROR_INVALIDDATA FFERRTAG( 'I','N','D','A') ///< Invalid data found when processing input
#define AVERROR_MUXER_NOT_FOUND FFERRTAG(0xF8,'M','U','X') ///< Muxer not found
#define AVERROR_OPTION_NOT_FOUND FFERRTAG(0xF8,'O','P','T') ///< Option not found
#define AVERROR_PATCHWELCOME FFERRTAG( 'P','A','W','E') ///< Not yet implemented in FFmpeg, patches welcome
#define AVERROR_PROTOCOL_NOT_FOUND FFERRTAG(0xF8,'P','R','O') ///< Protocol not found
#define AVERROR_STREAM_NOT_FOUND FFERRTAG(0xF8,'S','T','R') ///< Stream not found
/**
* This is semantically identical to AVERROR_BUG
* it has been introduced in Libav after our AVERROR_BUG and with a modified value.
*/
#define AVERROR_BUG2 FFERRTAG( 'B','U','G',' ')
#define AVERROR_UNKNOWN FFERRTAG( 'U','N','K','N') ///< Unknown error, typically from an external library
#define AVERROR_EXPERIMENTAL (-0x2bb2afa8) ///< Requested feature is flagged experimental. Set strict_std_compliance if you really want to use it.
#define AVERROR_INPUT_CHANGED (-0x636e6701) ///< Input changed between calls. Reconfiguration is required. (can be OR-ed with AVERROR_OUTPUT_CHANGED)
#define AVERROR_OUTPUT_CHANGED (-0x636e6702) ///< Output changed between calls. Reconfiguration is required. (can be OR-ed with AVERROR_INPUT_CHANGED)
/* HTTP & RTSP errors */
#define AVERROR_HTTP_BAD_REQUEST FFERRTAG(0xF8,'4','0','0')
#define AVERROR_HTTP_UNAUTHORIZED FFERRTAG(0xF8,'4','0','1')
#define AVERROR_HTTP_FORBIDDEN FFERRTAG(0xF8,'4','0','3')
#define AVERROR_HTTP_NOT_FOUND FFERRTAG(0xF8,'4','0','4')
#define AVERROR_HTTP_OTHER_4XX FFERRTAG(0xF8,'4','X','X')
#define AVERROR_HTTP_SERVER_ERROR FFERRTAG(0xF8,'5','X','X')
```
Right now we have mappings for `AVERROR_*` but `AVERROR(errno)` are simply mapped to `Error::Unknown`: https://github.com/zmwangx/rust-ffmpeg/blob/f1ccd4e69641dbd2be98e3ef38e670b592a103c1/src/util/error.rs#L44-L78
This is pretty terrible when you need to distinguish between a known `AVERROR(errno)` and actually unknown errors; for instance, `AVERROR(EAGAIN)` is extensively used in codec APIs.
We can probably create variants corresponding to known POSIX error codes, e.g. `libc::EAGAIN` (need to pay attention to portability here, Windows, macOS, Linux and POSIX only share a subset of error codes — ISO C standard doesn't help here since according to [POSIX errno.h](https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/basedefs/errno.h.html), "The ISO C standard only requires the symbols [EDOM], [EILSEQ], and [ERANGE] to be defined."), then implement `Display` using `libc::strerror` (which unfortunately is frowned upon).
I managed to fix this through meticulously tracing none other than [`ffmpeg.c`](https://github.com/FFmpeg/FFmpeg/blob/n4.3.1/fftools/ffmpeg.c). Note that even the official [`transcoding.c` example](https://github.com/FFmpeg/FFmpeg/blob/n4.3.1/doc/examples/transcoding.c) (though a bit dated) has this exact problem when you replace `filter_spec = "null";` with `filter_spec = "fps=fps=25.0,format=bgr8";`, so it's indeed subtle. (I'll try to mail the ffmpeg-dev mailing list to have the example fixed if I have the time.)
The debug process and fix led me to add some new APIs and realize that several existing APIs are either deprecated or slightly incorrect, so this turned out to be a pretty useful issue; you have my gratitude.
The fixed script (only works against the current master): https://gist.github.com/zmwangx/bacd12df8b8dce4c91db47a34a637004/4ffd2f9a701ceb9e54e45d2f4162e18b2200b979
The script is pretty heavily refactored and reformatted, so it's pretty hard to see the changes, but I've clearly marked where the major behavioral changes occur; reverting those would effectively bring back your original version.
## Explanations
The most useful thing in my debugging process was cranking FFmpeg log level to `trace` (or `debug`). Continuing my example above, on the command line:
```
ffmpeg -loglevel repeat+trace -y -i input.gif -filter:v fps=fps=25.0,format=bgr8 output.gif
```
I had to add a `util::log` API to this crate, but once done I can use the same logging level and compare. The issue becomes rather apparent when we compare the logs:
FFmpeg:
```
...
[Parsed_fps_0 @ 0x7fab28504640] Read frame with in pts 90, out pts 23
[Parsed_fps_0 @ 0x7fab28504640] Writing frame with pts 20 to pts 20
[Parsed_fps_0 @ 0x7fab28504640] Writing frame with pts 20 to pts 21
[Parsed_fps_0 @ 0x7fab28504640] Writing frame with pts 20 to pts 22
[Parsed_fps_0 @ 0x7fab28504640] Duplicated frame with pts 20 2 times
[gif @ 0x7fab28817c00] 23x38 image at pos (38;0) [area:100x50]
[gif @ 0x7fab28817c00] 20x21 image at pos (41;0) [area:100x50]
[gif @ 0x7fab28817c00] 3x10 image at pos (58;8) [area:100x50]
[Parsed_fps_0 @ 0x7fab28504640] EOF is at pts 25
[Parsed_fps_0 @ 0x7fab28504640] Writing frame with pts 23 to pts 23
[Parsed_fps_0 @ 0x7fab28504640] Writing frame with pts 23 to pts 24
[Parsed_fps_0 @ 0x7fab28504640] Duplicated frame with pts 23 1 times
[gif @ 0x7fab28817c00] 58x38 image at pos (21;0) [area:100x50]
[gif @ 0x7fab28817c00] 55x37 image at pos (24;1) [area:100x50]
[out_0_0 @ 0x7fab2842f400] EOF on sink link out_0_0:default.
...
[Parsed_fps_0 @ 0x7fab28504640] 10 frames in, 25 frames out; 0 frames dropped, 15 frames duplicated.
...
```
Sample program, prior to the fixes:
```
...
[Parsed_fps_0 @ 0x7fd04ee04800] Read frame with in pts 90, out pts 23
[Parsed_fps_0 @ 0x7fd04ee04800] Writing frame with pts 20 to pts 20
[gif @ 0x7fd04f01a600] 23x38 image at pos (38;0) [area:100x50]
[Parsed_fps_0 @ 0x7fd04ee04800] Writing frame with pts 20 to pts 21
[gif @ 0x7fd04f01a600] 20x21 image at pos (41;0) [area:100x50]
[Parsed_fps_0 @ 0x7fd04ee04800] Writing frame with pts 20 to pts 22
[gif @ 0x7fd04f01a600] 3x10 image at pos (58;8) [area:100x50]
[Parsed_fps_0 @ 0x7fd04ee04800] Duplicated frame with pts 20 2 times
[Parsed_fps_0 @ 0x7fd04ee04800] EOF is at pts 23
[Parsed_fps_0 @ 0x7fd04ee04800] Dropping frame with pts 23
...
[Parsed_fps_0 @ 0x7fd04ee04800] 10 frames in, 23 frames out; 1 frames dropped, 14 frames duplicated.
...
```
Note the difference of `EOF is at pts 23` vs `EOF is at pts 25`, and the two missing frames. (Previously I observed that the sample program only writes 22 frames, but that's because you forgot to call `write_trailer` on the output context, making the last output frame lost in the buffer.)
As I said I meticulously traced `ffmpeg.c`, which led me to the key realization: **FFmpeg uses `av_buffersrc_close` in [`ifilter_send_eof`](https://github.com/FFmpeg/FFmpeg/blob/6b6b9e593dd4d3aaf75f48d40a13ef03bdef9fdb/fftools/ffmpeg.c#L2183-L2204), instead of sending a `NULL` frame through `av_buffersrc_add_frame` to signal EOF, like we did.** Here's the docs for `av_buffersrc_close`:
```c
/**
* Close the buffer source after EOF.
*
* This is similar to passing NULL to av_buffersrc_add_frame_flags()
* except it takes the timestamp of the EOF, i.e. the timestamp of the end
* of the last frame.
*/
int av_buffersrc_close(AVFilterContext *ctx, int64_t pts, unsigned flags);
```
So, in the former case, the EOF pts is manually specified (100); in the latter case, the pts is somehow inferred, wrongly: FFmpeg thought output frame 23 (92) would be the EOF, maybe because the last input frame fed in had pts 90? I'm not sure why it's inferred wrongly — that would entail digging even deeper into FFmpeg source. Anyway, I think it's reasonable to assume that `ffmpeg.c` decided to manually keep track of pts and use `av_buffersrc_close` for a reason. `av_buffersrc_close` wasn't really exposed in this crate, so I added a `close` method to `filter::Source` (f1ccd4e69641dbd2be98e3ef38e670b592a103c1) parallel to the `flush` method we were using.
That's the crucial change. Other major changes:
- `output_context.write_trailer()`, already mentioned;
- `encoder.flush`, metioned in my previous assessment, necessary in theory but doesn't affect behavior in this case.
I should also point out that `decoder.decode` and `encoder.encode` in fact rely on deprecated APIs (`avcodec_decode_video2` and `avcodec_encode_video2`). They should be replaced with `avcodec_send_packet`/`avcodec_receive_frame`/`avcodec_send_frame`/`avcodec_receive_frame`-based APIs, which don't exist in this crate yet; I should add those shortly, and once added those should be preferred.
---
Finally, since your actual program uses https://github.com/kornelski/rust-ffmpeg.git, you need to get in touch with kornelski in order to add the relevant API exposing `av_buffersrc_close` (or maybe he knows how to work around the `filter::Source::flush` frame-dropping problem).