When the Ogg muxer writes a page, it has to do three things: It needs to
write a page header, then it has to actually copy the page data and then
it has to calculate and write a CRC checksum of both header as well as
data at a certain position in the page header.
To do this, the muxer used a dynamic buffer for both writing as well as
calculating the checksum via an AVIOContext's feature to automatically
calculate checksums on the data it writes. This entails an allocation of
an AVIOContext, of the opaque specific to dynamic buffers and of the
buffer itself (which may be reallocated multiple times) as well as
memcopying the data (first into the AVIOContext's small write buffer,
then into the dynamic buffer's big buffer).
This commit changes this: The page header is no longer written into a
dynamic buffer any more; instead the (small) page header is written into
a small buffer on the stack. The CRC is then calculated directly via
av_crc() on both the page header as well as the page data. Then both the
page header and the page data are written.
Finally, ogg_write_page() can now no longer fail, so it has been
modified to return nothing; this also fixed a bug in the only caller of
this function: It didn't check the return value.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
After this claim was made in e34e361 kamedo2 did an in-depth ABX
test comparing these encoders:
https://hydrogenaud.io/index.php?topic=111085.0
Result: FFmpeg AAC wasn't as good as libfdk_aac on average.
I know some things have changed since then such as, "use the fast
coder as the default" (fcb681ac) for example, so maybe the situation
is different now.
However, I am unaware of any recent comparison. So without any
substantiation we shouldn't make such a blantant claim.
Signed-off-by: Lou Logan <lou@lrcd.com>
Signed-off-by: Gyan Doshi <ffmpeg@gyani.pro>
Up until now the documentation of ff_bsf_get_packet_ref() allowed return
values >= 0 in case of success, whereas av_bsf_receive_packet() only
allows 0 on success. Given that for some bitstream filters the return
value of ff_bsf_get_packet_ref() is forwarded to the caller of
av_bsf_receive_packet() without any filtering, there would be a problem
if ff_bsf_get_packet_ref() actually returned values > 0. But it
currently doesn't and there is no reason why it should ever do so.
Therefore this commit aligns the return values of these functions by
restricting ff_bsf_get_packet_ref() to always returns 0 on success.
Reviewed-by: James Almer <jamrial@gmail.com>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
Mainly includes reindentation and returning directly (i.e. without
a goto fail when possible).
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
hls_init() would at first allocate the vtt_basename string, then
allocate the vtt_m3u8_name string followed by several operations that
may fail and then open the subtitles' output context. Yet upon freeing,
these strings were only freed when the subtitles' output context
existed, ensuring that they leak if something goes wrong between their
allocation and the opening of the subtitles' output context. So drop the
check for whether this output context exists.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
This fixes memleaks in instances such as:
a) When an allocation fails at one of the two places in hls_init() where
the error is returned immediately without goto fail first.
b) When an error happens when writing the header.
c) When an allocation fails at one of the three places in
hls_write_trailer() where the error is returned immediately without goto
fail first.
d) When one decides not to write the trailer at all (e.g. because of
errors when writing packets).
Furthermore, it removes code duplication and allows to return
immediately, without goto fail first.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
Several variables which are only used when the HLS_SINGLE_FILE flag is
unset have been set even when this flag is set. This has been changed.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
The Matroska specification allows multiple (level 1) Tags elements per
file, yet our demuxer didn't: While it parsed any amount of Tags
elements it found in front of the Clusters (albeit with warnings because
of duplicate elements), it would treat any Tags element only referenced
via a SeekHead entry as already parsed if any Tags element has already
been parsed; therefore this Tags element would not be parsed at all.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
There can be more than one SeekHead in a Matroska file, but most of the
other level 1 elements can only occur once.* Therefore the Matroska
demuxer only allows one entry per ID in its internal list of level 1
elements known to it; the only exception to this are SeekHeads.
The only exception to this are SeekHeads: When one is encountered
(either directly or in the list of entries read from SeekHeads),
a new entry in the list of known level-1 elements is always added,
even when this entry is actually already known.
This leads to lots of seeks in case of circular SeekHeads: Each time a
SeekHead is parsed, a new entry for a SeekHead will be added to the list
of entries read from SeekHeads. The exception for SeekHeads mentioned
above now implies that this SeekHead will always appear new and unparsed
and parsing will be attempted. This continued until the list of known
level-1 elements is full.
Fixing this is pretty simple: Don't add a new entry for a SeekHead if
its position matches the position of an already known SeekHead.
*: Actually, there can be multiple Tags and several other level 1
elements are "identically recurring" which means they may be resent
multiple times, but each instance must be absolutely identical to the
previous.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
A Seek element in a Matroska SeekHead should contain a SeekID and a
SeekPosition element and upon reading, they should be sanitized:
Given that IDs are restricted to 32 bit, longer SeekIDs should be treated
as invalid. Instead currently the lower 32 bits have been used.
For SeekPosition, no checks were performed for the element to be
present and if present, whether it was excessively large (i.e. the
absolute file position described by it exceeding INT64_MAX). The
SeekPosition element had a default value of -1 which means that a check
seems to have been intended; but it was not implemented. This commit adds
a check for overflow to the calculation of the absolute file position of
the referenced level 1 elements.
Using -1 (i.e. UINT64_MAX) as default value for SeekPosition implies that
a Seek element without SeekPosition will run afoul of this check.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
x264_encoder_encode can return 0 with nnal 0. As a result, encode_nals will
return 0. In this condition, it's better to return 0 immediately to avoid
the following unneeded pict_type and flags setting.
Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
Generic retime functionality is replaced by a few lines of code directly in the
muxers which used it, which seems a lot easier to understand and this way the
retiming is not dependant of the input durations.
Also remove retimeinterleave, since it is not used by anything anymore.
Signed-off-by: Marton Balint <cus@passwd.hu>
And rename it to retimeinterleave, use the pcm_rechunk bitstream filter for
rechunking.
By seperating the two functions we hopefully get cleaner code.
Signed-off-by: Marton Balint <cus@passwd.hu>
Previously only 1:1 bitstream filters were supported, the end of the stream was
not signalled to the bitstream filters and time base changes were ignored.
This change also allows muxers to set up bitstream filters regardless of the
autobsf flag during write_header instead of during check_bitstream and those
bitstream filters will always be executed.
Signed-off-by: Marton Balint <cus@passwd.hu>
avformat_alloc_output_context2() already sets the oformat member, so
that there is no reason to overwrite it again with the value it already
has.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
because the offset should use one byte
Reviewed-by: Zhao Jun <barryjzhao@tencent.com>
Reported-by: Zhao Jun <barryjzhao@tencent.com>
Signed-off-by: Steven Liu <liuqi05@kuaishou.com>
Reviewed-by: Martin Storsjö <martin@martin.st>
Signed-off-by: Linjie Fu <linjie.fu@intel.com>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
Failures of the allocations that happen under the hood when using dynamic
buffers are usually completely unchecked and the Matroska muxer is no
exception to this.
The API has its part in this, because there is no documented way to
actually check for errors: The return value of both avio_get_dyn_buf()
as well as avio_close_dyn_buf() is only documented as "the length of
the byte buffer", so that using this to return errors would be an API
break.
Therefore this commit uses the only reliable way to check for errors
with avio_get_dyn_buf(): The AVIOContext's error flag. (This is one of
the advantages of avio_get_dyn_buf(): By not destroying the AVIOContext
it is possible to inspect this value.) Checking whether the size or the
pointer vanishes is not enough as it does not check for truncated output
(the dynamic buffer API is int based and so has to truncate the buffer
even when enough memory would be available; it's current actual limit is
even way below INT_MAX).
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
If one already has the contents of a master elements in a buffer of
known size, then writing a EBML master element is no different from
writing an EBML binary element. It is overtly complicated to use
start/end_ebml_master() as these functions first write an unkown-length
size field of the appropriate length, then write the buffer's contents,
followed by a seek to the length field to overwrite it with the real
size (obtained via avio_tell() although it was already known in
advance), followed by another seek to the previous position. Just use
put_ebml_binary() instead.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
There is a good upper bound for the maximum length of the Colour master
element; it is therefore unnecessary to use a dynamic buffer for it.
A simple buffer on the stack is enough. This commit implements this.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
The Matroska muxer updates several header elements when the output is
seekable; if unseekable, the buffer containing the contents of the element
is immediately freed after writing. Before this commit, there were three
places doing exactly the same: Checking whether the output is seekable
and calling the function that writes and frees or the function that
just writes the EBML master. This has been unified; adding SeekHead
entries for these elements has been unified, too.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
David Manouchehri
Andreas Rheinhardt
Limin Wang
Lou Logan
Michael Niedermayer
Guo, Yejun
Marton Balint
Steven Liu
Linjie Fu