[FFmpeg-devel] [PATCH] libopenjpeg: do not define OPJ_STATIC for shared builds
Setting OPJ_STATIC when building shared libraries with openjpeg 2 causes the openjpeg symbols to have visibility hidden and the final linker step to fail due to undefined references. Signed-off-by: Andreas Cadhalpun --- libavcodec/libopenjpegdec.c | 3 +++ libavcodec/libopenjpegenc.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/libavcodec/libopenjpegdec.c b/libavcodec/libopenjpegdec.c index 65167e6..1f8dadf 100644 --- a/libavcodec/libopenjpegdec.c +++ b/libavcodec/libopenjpegdec.c @@ -24,7 +24,10 @@ * JPEG 2000 decoder using libopenjpeg */ +#include "config.h" +#if !CONFIG_SHARED #define OPJ_STATIC +#endif #include "libavutil/common.h" #include "libavutil/imgutils.h" diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c index 1443551..023fdf4 100644 --- a/libavcodec/libopenjpegenc.c +++ b/libavcodec/libopenjpegenc.c @@ -24,7 +24,10 @@ * JPEG 2000 encoder using libopenjpeg */ +#include "config.h" +#if !CONFIG_SHARED #define OPJ_STATIC +#endif #include "libavutil/avassert.h" #include "libavutil/common.h" -- 2.9.3 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] libopenjpegenc: recreate image data buffer after encoding frame
openjpeg 2 sets the data pointers of the image components to NULL,
causing segfaults if the image is reused.
Signed-off-by: Andreas Cadhalpun
---
The relevant openjpeg2 code is:
https://sources.debian.net/src/openjpeg2/2.1.2-1/src/lib/openjp2/j2k.c/?hl=10253#L10247
---
libavcodec/libopenjpegenc.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
index 023fdf4..2f0ab20 100644
--- a/libavcodec/libopenjpegenc.c
+++ b/libavcodec/libopenjpegenc.c
@@ -776,6 +776,16 @@ static int libopenjpeg_encode_frame(AVCodecContext *avctx,
AVPacket *pkt,
goto done;
}
+// openjpeg 2 sets the data pointers of the image components to NULL.
+// Thus the image can't be reused.
+opj_image_destroy(ctx->image);
+ctx->image = mj2_create_image(avctx, &ctx->enc_params);
+if (!ctx->image) {
+av_log(avctx, AV_LOG_ERROR, "Error creating the mj2 image\n");
+ret = AVERROR(EINVAL);
+goto done;
+}
+
av_shrink_packet(pkt, writer.pos);
#endif // OPENJPEG_MAJOR_VERSION == 1
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libopenjpeg: do not define OPJ_STATIC for shared builds
On 11.10.2016 19:13, Hendrik Leppkes wrote: > On Tue, Oct 11, 2016 at 5:50 PM, Andreas Cadhalpun > wrote: >> Setting OPJ_STATIC when building shared libraries with openjpeg 2 causes >> the openjpeg symbols to have visibility hidden and the final linker step >> to fail due to undefined references. > > Aren't these type of macros typically related to the way the library > (ie. openjpeg) is being linked into avcodec (or ffmpeg), and not what > kind of library we're building? Indeed, you're right. Building a static ffmpeg fails, as well. However, configure doesn't detect this, because it checks the header and the function separately. Also it adds -DOPJ_STATIC to EXTRALIBS, where it is rather useless, instead of to CFLAGS, where it would affect the code. This explains the explicit '#define OPJ_STATIC', which shouldn't be needed at all. And it just assumes that libopenjpeg is built statically... I'll send a patch fixing configure. Consider this patch dropped. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] configure: fix detection of libopenjpeg
Use check_lib2 to test the header together with the function. This is
necessary, because '-DOPJ_STATIC' changes what the included header does.
Also add '-DOPJ_STATIC' to CFLAGS, so that it isn't necessary to
hardcode this in libavcodec/libopenjpeg{dec,enc}.c.
Finally, check for non-static libopenjpeg, too.
Signed-off-by: Andreas Cadhalpun
---
configure | 9 ++---
libavcodec/libopenjpegdec.c | 2 --
libavcodec/libopenjpegenc.c | 2 --
3 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/configure b/configure
index 8fc71fb..ff743cb 100755
--- a/configure
+++ b/configure
@@ -5710,9 +5710,12 @@ enabled libopencv && { check_header
opencv2/core/core_c.h &&
require opencv opencv2/core/core_c.h
cvCreateImageHeader -lopencv_core -lopencv_imgproc; } ||
require_pkg_config opencv opencv/cxcore.h
cvCreateImageHeader; }
enabled libopenh264 && require_pkg_config openh264 wels/codec_api.h
WelsGetCodecVersion
-enabled libopenjpeg && { check_lib openjpeg-2.1/openjpeg.h opj_version
-lopenjp2 -DOPJ_STATIC ||
- check_lib openjpeg-2.0/openjpeg.h opj_version
-lopenjp2 -DOPJ_STATIC ||
- check_lib openjpeg-1.5/openjpeg.h opj_version
-lopenjpeg -DOPJ_STATIC ||
+enabled libopenjpeg && { { check_lib2 openjpeg-2.1/openjpeg.h
opj_version -lopenjp2 -DOPJ_STATIC && add_cflags -DOPJ_STATIC; } ||
+ check_lib2 openjpeg-2.1/openjpeg.h opj_version
-lopenjp2 ||
+ { check_lib openjpeg-2.0/openjpeg.h opj_version
-lopenjp2 -DOPJ_STATIC && add_cflags -DOPJ_STATIC; } ||
+ check_lib2 openjpeg-2.0/openjpeg.h opj_version
-lopenjp2 ||
+ { check_lib openjpeg-1.5/openjpeg.h opj_version
-lopenjpeg -DOPJ_STATIC && add_cflags -DOPJ_STATIC; } ||
+ check_lib2 openjpeg-1.5/openjpeg.h opj_version
-lopenjpeg ||
check_lib openjpeg.h opj_version -lopenjpeg
-DOPJ_STATIC ||
die "ERROR: libopenjpeg not found"; }
enabled libopenmpt&& require_pkg_config "libopenmpt >= 0.2.6557"
libopenmpt/libopenmpt.h openmpt_module_create
diff --git a/libavcodec/libopenjpegdec.c b/libavcodec/libopenjpegdec.c
index 65167e6..b4ce834 100644
--- a/libavcodec/libopenjpegdec.c
+++ b/libavcodec/libopenjpegdec.c
@@ -24,8 +24,6 @@
* JPEG 2000 decoder using libopenjpeg
*/
-#define OPJ_STATIC
-
#include "libavutil/common.h"
#include "libavutil/imgutils.h"
#include "libavutil/intreadwrite.h"
diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
index 1443551..5042507 100644
--- a/libavcodec/libopenjpegenc.c
+++ b/libavcodec/libopenjpegenc.c
@@ -24,8 +24,6 @@
* JPEG 2000 encoder using libopenjpeg
*/
-#define OPJ_STATIC
-
#include "libavutil/avassert.h"
#include "libavutil/common.h"
#include "libavutil/imgutils.h"
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] configure: fix detection of libopenjpeg
On 12.10.2016 03:35, Michael Bradshaw wrote:
> Oh man, I literally just wrote a patch for this today. You beat me by 6
> hours. Anyway, thanks for the patch!
What a coincidence!
> On Tue, Oct 11, 2016 at 11:37 AM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
>
[...]
>> +enabled libopenjpeg && { { check_lib2 openjpeg-2.1/openjpeg.h
>> opj_version -lopenjp2 -DOPJ_STATIC && add_cflags -DOPJ_STATIC; } ||
>>
>
> Use add_cppflags instead of add_cflags. The macro isn't needed for linking.
Fixed.
>> + check_lib2 openjpeg-2.1/openjpeg.h
>> opj_version -lopenjp2 ||
>> + { check_lib openjpeg-2.0/openjpeg.h
>> opj_version -lopenjp2 -DOPJ_STATIC && add_cflags -DOPJ_STATIC; } ||
>>
>
> You can drop the changes for v2.0 and below. Only v2.1 needs the check. The
> new OPJ_STATIC behavior was introduced in OpenJPEG v2.1.1.
Thanks for sharing that information.
Updated patch attached.
Best regards,
Andreas
>From 5c9e02fa1a0f45d84fc31de4ba53d8362a10b4e5 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Tue, 11 Oct 2016 20:28:35 +0200
Subject: [PATCH] configure: fix detection of libopenjpeg
Use check_lib2 to test the header together with the function. This is
necessary, because '-DOPJ_STATIC' changes what the included header does.
Also add '-DOPJ_STATIC' to CPPFLAGS, so that it isn't necessary to
hardcode this in libavcodec/libopenjpeg{dec,enc}.c.
Finally, check for non-static openjpeg 2.1, too.
Signed-off-by: Andreas Cadhalpun
---
configure | 9 +
libavcodec/libopenjpegdec.c | 2 --
libavcodec/libopenjpegenc.c | 2 --
3 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/configure b/configure
index 8fc71fb..2b71415 100755
--- a/configure
+++ b/configure
@@ -5710,10 +5710,11 @@ enabled libopencv && { check_header opencv2/core/core_c.h &&
require opencv opencv2/core/core_c.h cvCreateImageHeader -lopencv_core -lopencv_imgproc; } ||
require_pkg_config opencv opencv/cxcore.h cvCreateImageHeader; }
enabled libopenh264 && require_pkg_config openh264 wels/codec_api.h WelsGetCodecVersion
-enabled libopenjpeg && { check_lib openjpeg-2.1/openjpeg.h opj_version -lopenjp2 -DOPJ_STATIC ||
- check_lib openjpeg-2.0/openjpeg.h opj_version -lopenjp2 -DOPJ_STATIC ||
- check_lib openjpeg-1.5/openjpeg.h opj_version -lopenjpeg -DOPJ_STATIC ||
- check_lib openjpeg.h opj_version -lopenjpeg -DOPJ_STATIC ||
+enabled libopenjpeg && { { check_lib2 openjpeg-2.1/openjpeg.h opj_version -lopenjp2 -DOPJ_STATIC && add_cppflags -DOPJ_STATIC; } ||
+ check_lib2 openjpeg-2.1/openjpeg.h opj_version -lopenjp2 ||
+ { check_lib openjpeg-2.0/openjpeg.h opj_version -lopenjp2 -DOPJ_STATIC && add_cppflags -DOPJ_STATIC; } ||
+ { check_lib openjpeg-1.5/openjpeg.h opj_version -lopenjpeg -DOPJ_STATIC && add_cppflags -DOPJ_STATIC; } ||
+ { check_lib2 openjpeg.h opj_version -lopenjpeg -DOPJ_STATIC && add_cppflags -DOPJ_STATIC; } ||
die "ERROR: libopenjpeg not found"; }
enabled libopenmpt&& require_pkg_config "libopenmpt >= 0.2.6557" libopenmpt/libopenmpt.h openmpt_module_create
enabled libopus && require_pkg_config opus opus_multistream.h opus_multistream_decoder_create
diff --git a/libavcodec/libopenjpegdec.c b/libavcodec/libopenjpegdec.c
index 65167e6..b4ce834 100644
--- a/libavcodec/libopenjpegdec.c
+++ b/libavcodec/libopenjpegdec.c
@@ -24,8 +24,6 @@
* JPEG 2000 decoder using libopenjpeg
*/
-#define OPJ_STATIC
-
#include "libavutil/common.h"
#include "libavutil/imgutils.h"
#include "libavutil/intreadwrite.h"
diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
index 1443551..5042507 100644
--- a/libavcodec/libopenjpegenc.c
+++ b/libavcodec/libopenjpegenc.c
@@ -24,8 +24,6 @@
* JPEG 2000 encoder using libopenjpeg
*/
-#define OPJ_STATIC
-
#include "libavutil/avassert.h"
#include "libavutil/common.h"
#include "libavutil/imgutils.h"
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libopenjpegenc: recreate image data buffer after encoding frame
On 12.10.2016 03:42, Michael Bradshaw wrote:
> On Tue, Oct 11, 2016 at 9:57 AM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
>
>> openjpeg 2 sets the data pointers of the image components to NULL,
>> causing segfaults if the image is reused.
>>
>
> I've never seen this issue.
That's strange, as it happens practically always here (on Debian testing/sid).
> Is there a particular version of OpenJPEG or
The OpenJPEG version is 2.1.2, i.e. the latest.
> reproduction steps available?
For example:
$ ffmpeg -f lavfi -i testsrc -c:v libopenjpeg -f null /dev/null
> Where are the data pointers being set to NULL here?
>>
>> The relevant openjpeg2 code is:
>> https://sources.debian.net/src/openjpeg2/2.1.2-1/src/lib/openjp2/j2k.c/?hl=10253#L10247
I didn't include this link in the commit message, as it will become invalid,
as soon as the next OpenJPEG version enters Debian.
For future reference, the code is:
/* TODO_MSD: Find a better way */
if (p_image->comps) {
OPJ_UINT32 it_comp;
for (it_comp = 0 ; it_comp < p_image->numcomps; it_comp++) {
if (p_image->comps[it_comp].data) {
p_j2k->m_private_image->comps[it_comp].data
=p_image->comps[it_comp].data;
p_image->comps[it_comp].data = NULL;
}
}
}
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]configure: Enable pie for toolchain=hardened.
On 04.10.2016 12:24, Carl Eugen Hoyos wrote: > Sorry if I miss something but with this patch, the hardening_check > script succeeds here both for x86_32 and x86_64 (static and shared). This script uses a very simplistic approach for testing position independent executables. I think it just does the equivalent of 'readelf -h $PROGRAM | grep Type'. If the Type is EXEC, it's a normal executable, and if it is DYN, it assumes it's compiled as PIE. However, that doesn't guarantee that the executable is actually position independent, i.e. does not contain text relocations. > --- a/configure > +++ b/configure > @@ -3577,6 +3577,8 @@ case "$toolchain" in > add_cppflags -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 > add_cflags -fno-strict-overflow -fstack-protector-all > add_ldflags -Wl,-z,relro -Wl,-z,now > +add_cflags -fPIE I think this should be -fPIC, at least when building shared libraries. That's how I understand the gcc manual [1]: -fpie -fPIE These options are similar to -fpic and -fPIC, but generated position independent code can be only linked into executables. > +add_ldexeflags -fPIE -pie > ;; > ?*) > die "Unknown toolchain $toolchain" > -- 1.7.10.4 In general, enabling PIE for toolchain=hardened is a good idea. But According to [2] PIE doesn't work on hppa and m68k, so it shouldn't get enabled for these architectures. On 05.10.2016 15:14, Carl Eugen Hoyos wrote: > I would have expected that this (pie) patch does not work on x86_32 > but the binary runs fine here: Am I missing something or should I > apply to get this tested? The problem on x86_32 is that libavcodec, libavutil, etc. use text relocations in hand-written assembler code, so these libraries won't be position independent, unless using --disable-asm. Now, when producing shared libraries, the ffmpeg binary is actually position independent, just not libavcodec, libavutil... However, when linking statically, the ffmpeg binary contains the text relocations from the hand-written assembler code and is thus not really position independent. This can be tested e.g. with scanelf from pax-utils [3]. * shared PIE build on x86_32 (no text relocations): $ scanelf -t ./ffmpeg TYPE TEXTREL FILE ET_DYN-./ffmpeg * static PIE build on x86_32 (with text relocations): $ scanelf -t ./ffmpeg TYPE TEXTREL FILE ET_DYN TEXTREL ./ffmpeg The '-T' options shows were exactly the text relocations are. Best regards, Andreas 1: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html 2: https://wiki.debian.org/Hardening#DEB_BUILD_HARDENING_PIE_.28gcc.2Fg.2B-.2B-_-fPIE_-pie.29 3: https://wiki.gentoo.org/wiki/Hardened/PaX_Utilities ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]configure: Use LDEXEFLAGS in check_ld().
On 05.10.2016 15:12, Carl Eugen Hoyos wrote:
> From 5e26339b041f5e35112ed7f479ade6e57c3b5248 Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos
> Date: Wed, 5 Oct 2016 15:10:23 +0200
> Subject: [PATCH] configure: Use LDEXEFLAGS in check_ld().
>
> Avoids detecting libraries that are not compatible with ldexeflags.
> ---
> configure |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index f593191..9297321 100755
> --- a/configure
> +++ b/configure
> @@ -985,7 +985,7 @@ check_ld(){
> check_$type $($cflags_filter $flags) || return
> flags=$($ldflags_filter $flags)
> libs=$($ldflags_filter $libs)
> -check_cmd $ld $LDFLAGS $flags $(ld_o $TMPE) $TMPO $libs $extralibs
> +check_cmd $ld $LDFLAGS $LDEXEFLAGS $flags $(ld_o $TMPE) $TMPO $libs
> $extralibs
> }
>
> print_include(){
> -- 1.7.10.4
LGTM.
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]configure: Enable pie for toolchain=hardened.
On 12.10.2016 23:44, Carl Eugen Hoyos wrote: > 2016-10-12 19:04 GMT+02:00 Andreas Cadhalpun > : >> On 04.10.2016 12:24, Carl Eugen Hoyos wrote: >>> Sorry if I miss something but with this patch, the hardening_check >>> script succeeds here both for x86_32 and x86_64 (static and shared). >> >> This script uses a very simplistic approach for testing position >> independent executables. >> I think it just does the equivalent of 'readelf -h $PROGRAM | grep Type'. >> If the Type is EXEC, it's a normal executable, and if it is DYN, it >> assumes it's compiled as PIE. > >> However, that doesn't guarantee that the executable is actually position >> independent, i.e. does not contain text relocations. > > My understanding of PIE is (very) limited but I suspect text relocations > and PIE do not exclude each other. As I understand it the literal meaning of position independent code is code without text relocations, because those are what makes the code position dependent. So in this literal sense PIE and text relocations exclude each other. (The -fPIC/-fPIE flags tell the compiler to produce code without text relocations.) The additional complication for executables is that ASLR doesn't work for normal executables, because they are always mapped to the same point in address space. The basic idea of PIE is to build the executables actually as shared libraries, so that they can benefit from ASLR just as normal shared libraries. (The '-pie' flag tells the linker to produce such an executable.) However, such a '-pie' executable is not necessarily position independent in the literal sense, i.e. might contain text relocations. In that sense PIE and text relocations don't exclude each other. If you want both NX and ASLR security features for an executable it has to be built with '-pie' and must not contain text relocations. >>> --- a/configure >>> +++ b/configure >>> @@ -3577,6 +3577,8 @@ case "$toolchain" in >>> add_cppflags -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 >>> add_cflags -fno-strict-overflow -fstack-protector-all >>> add_ldflags -Wl,-z,relro -Wl,-z,now >>> +add_cflags -fPIE >> >> I think this should be -fPIC, at least when building shared libraries. > > Afaiu, shared libraries and PIE do exclude each other, pic is already > supported in FFmpeg (--enable-pic) but this patch is only meant for > PIE. > >> That's how I understand the gcc manual [1]: >> -fpie >> -fPIE >> These options are similar to -fpic and -fPIC, but generated position >> independent code can be only linked into executables. > > So shared libraries and PIE exclude each other but PIE is for > static linking what pic is for dynamic linking. > Good to know! I think you misunderstood this. The problem with -fPIE is that it allows the compiler to make optimizations only suitable if the resulting object ends up in an executable. However, it is entirely possible to build the objects with -fPIC and produce an executable with '-pie'. These same objects can also be used to build normal shared libraries, while objects compiled with '-fPIE' shouldn't be used for that. And '-pie' is also useful for dynamic linking. So using -fPIC in CFLAGS and '-fPIC -pie' in LDEXEFLAGS should always work. >>> +add_ldexeflags -fPIE -pie >>> ;; >>> ?*) >>> die "Unknown toolchain $toolchain" >>> -- 1.7.10.4 >> >> In general, enabling PIE for toolchain=hardened is a good idea. > >> But According to [2] PIE doesn't work on hppa and m68k, so it >> shouldn't get enabled for these architectures. > > I was convinced that my ancient Linux system wouldn't know > about ASLR but to my surprise, I was able to reproduce that > my patch actually works (56bit entropy iiuc). > My hppa test-system is currently down (and all reports about > pie not working on hppa seem to be from 2008), The information from the wiki might be outdated... > I will try to test, > in any case, I will commit the patch soon. > > I will not be able to test on m68k. I can't test hppa or m68k directly either. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] configure: fix detection of libopenjpeg
On 13.10.2016 03:11, Michael Bradshaw wrote: > On Wed, Oct 12, 2016 at 8:30 AM, Andreas Cadhalpun < > andreas.cadhal...@googlemail.com> wrote: >> >> Updated patch attached. > > > New patch looks good to me (though I don't have push privileges so can't > commit it). I pushed it myself. Thanks for the review. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libopenjpegenc: recreate image data buffer after encoding frame
On 13.10.2016 03:36, Michael Bradshaw wrote:
> Thanks for that (and the link back to the OpenJPEG source). Well dang. I
> think it would be better to change the patch to completely remove the image
> member from LibOpenJPEGContext, and instead just create a local image (and
> destroy it) for every call to libopenjpeg_encode_frame.
OK. Attached patch does that for openjpeg 2.
I didn't change the behavior for openjpeg 1, as reusing the image works there.
Best regards,
Andreas
>From 350ceab27db0346d89698070e40b1ae19ff1d559 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Thu, 13 Oct 2016 21:16:35 +0200
Subject: [PATCH] libopenjpegenc: stop reusing image data buffer for openjpeg 2
openjpeg 2 sets the data pointers of the image components to NULL,
causing segfaults if the image is reused.
Signed-off-by: Andreas Cadhalpun
---
libavcodec/libopenjpegenc.c | 41 ++---
1 file changed, 30 insertions(+), 11 deletions(-)
diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
index 5042507..857ee1a 100644
--- a/libavcodec/libopenjpegenc.c
+++ b/libavcodec/libopenjpegenc.c
@@ -52,7 +52,9 @@
typedef struct LibOpenJPEGContext {
AVClass *avclass;
+#if OPENJPEG_MAJOR_VERSION == 1
opj_image_t *image;
+#endif // OPENJPEG_MAJOR_VERSION == 1
opj_cparameters_t enc_params;
#if OPENJPEG_MAJOR_VERSION == 1
opj_event_mgr_t event_mgr;
@@ -369,18 +371,22 @@ static av_cold int libopenjpeg_encode_init(AVCodecContext *avctx)
cinema_parameters(&ctx->enc_params);
}
+#if OPENJPEG_MAJOR_VERSION == 1
ctx->image = mj2_create_image(avctx, &ctx->enc_params);
if (!ctx->image) {
av_log(avctx, AV_LOG_ERROR, "Error creating the mj2 image\n");
err = AVERROR(EINVAL);
goto fail;
}
+#endif // OPENJPEG_MAJOR_VERSION == 1
return 0;
fail:
+#if OPENJPEG_MAJOR_VERSION == 1
opj_image_destroy(ctx->image);
ctx->image = NULL;
+#endif // OPENJPEG_MAJOR_VERSION == 1
return err;
}
@@ -591,19 +597,25 @@ static int libopenjpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
const AVFrame *frame, int *got_packet)
{
LibOpenJPEGContext *ctx = avctx->priv_data;
-opj_image_t *image = ctx->image;
+int ret;
+AVFrame *gbrframe;
+int cpyresult = 0;
#if OPENJPEG_MAJOR_VERSION == 1
+opj_image_t *image = ctx->image;
opj_cinfo_t *compress = NULL;
opj_cio_t *stream = NULL;
int len;
#else // OPENJPEG_MAJOR_VERSION == 2
+PacketWriter writer = { 0 };
opj_codec_t *compress = NULL;
opj_stream_t *stream= NULL;
-PacketWriter writer = { 0 };
+opj_image_t *image = mj2_create_image(avctx, &ctx->enc_params);
+if (!image) {
+av_log(avctx, AV_LOG_ERROR, "Error creating the mj2 image\n");
+ret = AVERROR(EINVAL);
+goto done;
+}
#endif // OPENJPEG_MAJOR_VERSION == 1
-int cpyresult = 0;
-int ret;
-AVFrame *gbrframe;
switch (avctx->pix_fmt) {
case AV_PIX_FMT_RGB24:
@@ -626,8 +638,10 @@ static int libopenjpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
case AV_PIX_FMT_GBRP14:
case AV_PIX_FMT_GBRP16:
gbrframe = av_frame_clone(frame);
-if (!gbrframe)
-return AVERROR(ENOMEM);
+if (!gbrframe) {
+ret = AVERROR(ENOMEM);
+goto done;
+}
gbrframe->data[0] = frame->data[2]; // swap to be rgb
gbrframe->data[1] = frame->data[0];
gbrframe->data[2] = frame->data[1];
@@ -684,19 +698,21 @@ static int libopenjpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
av_log(avctx, AV_LOG_ERROR,
"The frame's pixel format '%s' is not supported\n",
av_get_pix_fmt_name(avctx->pix_fmt));
-return AVERROR(EINVAL);
+ret = AVERROR(EINVAL);
+goto done;
break;
}
if (!cpyresult) {
av_log(avctx, AV_LOG_ERROR,
"Could not copy the frame data to the internal image buffer\n");
-return -1;
+ret = -1;
+goto done;
}
#if OPENJPEG_MAJOR_VERSION == 2
if ((ret = ff_alloc_packet2(avctx, pkt, 1024, 0)) < 0) {
-return ret;
+goto done;
}
#endif // OPENJPEG_MAJOR_VERSION == 2
@@ -763,7 +779,7 @@ static int libopenjpeg_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
#error Missing call to opj_stream_set_user_data
#endif
-if (!opj_start_compress(compress, ctx->image, stream) ||
+if (!opj_start_compress(compress, image, stream) ||
!opj_encode(compress, stream) ||
!opj_end_compress(compress, stream)) {
av_log(avctx, AV_LOG_ERROR, "Error during the opj encode\n");
@@ -782,6 +798,7 @@ done:
#if O
[FFmpeg-devel] [PATCH] libopenjpegenc: fix out-of-bounds reads when filling the edges
If x is 0, 'x - 1' is in the previous line, or worse outside the buffer
for the first line.
If y is 0, 'x - image->comps[compno].w' is outside the buffer.
Finally, image->comps[compno].w is unsigned (at least in openjpeg2), so
the calculation could silently wrap around without the explicit cast to
int.
Signed-off-by: Andreas Cadhalpun
---
libavcodec/libopenjpegenc.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
index 857ee1a..83c965d 100644
--- a/libavcodec/libopenjpegenc.c
+++ b/libavcodec/libopenjpegenc.c
@@ -415,13 +415,13 @@ static int libopenjpeg_copy_packed8(AVCodecContext
*avctx, const AVFrame *frame,
frame_index += numcomps;
}
for (; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - 1];
+image_line[x] = x > 0 ? image_line[x - 1] : 0;
}
}
for (; y < image->comps[compno].h; ++y) {
image_line = image->comps[compno].data + y *
image->comps[compno].w;
for (x = 0; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - image->comps[compno].w];
+image_line[x] = y > 0 ? image_line[x -
(int)image->comps[compno].w] : 0;
}
}
}
@@ -455,13 +455,13 @@ static int libopenjpeg_copy_packed12(AVCodecContext
*avctx, const AVFrame *frame
frame_index += numcomps;
}
for (; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - 1];
+image_line[x] = x > 0 ? image_line[x - 1] : 0;
}
}
for (; y < image->comps[compno].h; ++y) {
image_line = image->comps[compno].data + y *
image->comps[compno].w;
for (x = 0; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - image->comps[compno].w];
+image_line[x] = y > 0 ? image_line[x -
(int)image->comps[compno].w] : 0;
}
}
}
@@ -495,13 +495,13 @@ static int libopenjpeg_copy_packed16(AVCodecContext
*avctx, const AVFrame *frame
frame_index += numcomps;
}
for (; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - 1];
+image_line[x] = x > 0 ? image_line[x - 1] : 0;
}
}
for (; y < image->comps[compno].h; ++y) {
image_line = image->comps[compno].data + y *
image->comps[compno].w;
for (x = 0; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - image->comps[compno].w];
+image_line[x] = y > 0 ? image_line[x -
(int)image->comps[compno].w] : 0;
}
}
}
@@ -536,13 +536,13 @@ static int libopenjpeg_copy_unpacked8(AVCodecContext
*avctx, const AVFrame *fram
for (x = 0; x < width; ++x)
image_line[x] = frame->data[compno][frame_index++];
for (; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - 1];
+image_line[x] = x > 0 ? image_line[x - 1] : 0;
}
}
for (; y < image->comps[compno].h; ++y) {
image_line = image->comps[compno].data + y *
image->comps[compno].w;
for (x = 0; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - image->comps[compno].w];
+image_line[x] = y > 0 ? image_line[x -
(int)image->comps[compno].w] : 0;
}
}
}
@@ -579,13 +579,13 @@ static int libopenjpeg_copy_unpacked16(AVCodecContext
*avctx, const AVFrame *fra
for (x = 0; x < width; ++x)
image_line[x] = frame_ptr[frame_index++];
for (; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - 1];
+image_line[x] = x > 0 ? image_line[x - 1] : 0;
}
}
for (; y < image->comps[compno].h; ++y) {
image_line = image->comps[compno].data + y *
image->comps[compno].w;
for (x = 0; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - image->comps[compno].w];
+image_line[x] = y > 0 ? image_line[x -
(int)image->comps[compno].w] : 0;
}
}
}
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] doc: fix spelling errors
Thanks to Mathieu Malaterre for reporting the
Que/Queue typo. (https://bugs.debian.org/839542)
Signed-off-by: Andreas Cadhalpun
---
doc/Doxyfile| 2 +-
doc/encoders.texi | 2 +-
doc/ffprobe.texi| 2 +-
doc/filters.texi| 10 +-
doc/muxers.texi | 6 +++---
ffmpeg.c| 2 +-
ffmpeg_cuvid.c | 4 ++--
libavcodec/aaccoder_twoloop.h | 2 +-
libavcodec/cabac.c | 2 +-
libavcodec/ffjni.c | 2 +-
libavcodec/mediacodec_wrapper.h | 2 +-
libavcodec/psymodel.h | 2 +-
libavcodec/x86/vp9lpf_16bpp.asm | 2 +-
libavfilter/af_hdcd.c | 2 +-
libavfilter/f_ebur128.c | 2 +-
libavformat/internal.h | 2 +-
libavutil/frame.h | 2 +-
libavutil/tree.h| 2 +-
18 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/doc/Doxyfile b/doc/Doxyfile
index fb5cdd3..0891899 100644
--- a/doc/Doxyfile
+++ b/doc/Doxyfile
@@ -1637,7 +1637,7 @@ EXTRA_PACKAGES =
# following commands have a special meaning inside the header: $title,
# $datetime, $date, $doxygenversion, $projectname, $projectnumber,
# $projectbrief, $projectlogo. Doxygen will replace $title with the empy
string,
-# for the replacement values of the other commands the user is refered to
+# for the replacement values of the other commands the user is referred to
# HTML_HEADER.
# This tag requires that the tag GENERATE_LATEX is set to YES.
diff --git a/doc/encoders.texi b/doc/encoders.texi
index 1f4044e..375b1b6 100644
--- a/doc/encoders.texi
+++ b/doc/encoders.texi
@@ -1773,7 +1773,7 @@ Enable CAVLC and disable CABAC. It generates the same
effect as
@end table
@item cmp
-Set full pixel motion estimation comparation algorithm. Possible values:
+Set full pixel motion estimation comparison algorithm. Possible values:
@table @samp
@item chroma
diff --git a/doc/ffprobe.texi b/doc/ffprobe.texi
index 2024eed..1069ae3 100644
--- a/doc/ffprobe.texi
+++ b/doc/ffprobe.texi
@@ -245,7 +245,7 @@ continue reading from that.
Each interval is specified by two optional parts, separated by "%".
The first part specifies the interval start position. It is
-interpreted as an abolute position, or as a relative offset from the
+interpreted as an absolute position, or as a relative offset from the
current position if it is preceded by the "+" character. If this first
part is not specified, no seeking will be performed when reading this
interval.
diff --git a/doc/filters.texi b/doc/filters.texi
index 76265e7..1f0f1f8 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -449,7 +449,7 @@ This filter is bit crusher with enhanced functionality. A
bit crusher
is used to audibly reduce number of bits an audio signal is sampled
with. This doesn't change the bit depth at all, it just produces the
effect. Material reduced in bit depth sounds more harsh and "digital".
-This filter is able to even round to continous values instead of discrete
+This filter is able to even round to continuous values instead of discrete
bit depths.
Additionally it has a D/C offset which results in different crushing of
the lower and the upper half of the signal.
@@ -475,7 +475,7 @@ Set level out.
Set bit reduction.
@item mix
-Set mixing ammount.
+Set mixing amount.
@item mode
Can be linear: @code{lin} or logarithmic: @code{log}.
@@ -1203,7 +1203,7 @@ Set video stream size. Only useful if curves option is
activated.
@item mgain
Set max gain that will be displayed. Only useful if curves option is activated.
-Setting this to reasonable value allows to display gain which is derived from
+Setting this to reasonable value allows one to display gain which is derived
from
neighbour bands which are too close to each other and thus produce higher gain
when both are activated.
@@ -8834,7 +8834,7 @@ value.
@section hysteresis
Grow first stream into second stream by connecting components.
-This allows to build more robust edge masks.
+This allows one to build more robust edge masks.
This filter accepts the following options:
@@ -17575,7 +17575,7 @@ magnitude across time and second represents phase
across time.
The filter will transform from frequency domain as displayed in videos back
to time domain as presented in audio output.
-This filter is primarly created for reversing processed @ref{showspectrum}
+This filter is primarily created for reversing processed @ref{showspectrum}
filter outputs, but can synthesize sound from other spectrograms too.
But in such case results are going to be poor if the phase data is not
available, because in such cases phase data need to be recreated, usually
diff --git a/doc/muxers.texi b/doc/muxers.texi
index dbe53f5..c6b8cc5 100644
--- a/doc/muxers.texi
+++ b/doc/muxers.texi
@@ -1513,14 +1513,14 @@ as a list of @var{key}=@var{value} pairs separated by
'
Re: [FFmpeg-devel] [PATCH] libopenjpegenc: fix out-of-bounds reads when filling the edges
On 14.10.2016 00:00, Hendrik Leppkes wrote: > On Thu, Oct 13, 2016 at 10:25 PM, Andreas Cadhalpun > wrote: >> If x is 0, 'x - 1' is in the previous line, or worse outside the buffer >> for the first line. >> >> If y is 0, 'x - image->comps[compno].w' is outside the buffer. >> > > I'm slightly puzzled, as you say, these are for edge handling, edges > in this case are from the image width to buffer width, and image > height to buffer height, respectively > So for x or y to be zero, we would need an image thats zero width, or > zero height, so the edge starts at zero? > > How does that happen, and wouldn't it be much simpler to catch that > case earlier in the chain and simply error out? A image with either > zero width or zero height surely is not something you can encode > either way. The avctx->width/avctx->height is not zero, but libopenjpeg_copy_unpacked8 does: width = avctx->width / image->comps[compno].dx; height = avctx->height / image->comps[compno].dy; So if e.g. avctx->height is 1 and image->comps[compno].dy is 2, height becomes 0. I'm not sure if that's invalid. I think the other functions are currently not affected by this, but I added the check anyway for robustness. (The cast to int is needed in any case.) Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libopenjpegenc: fix out-of-bounds reads when filling the edges
On 14.10.2016 00:49, Michael Niedermayer wrote:
> On Fri, Oct 14, 2016 at 12:23:02AM +0200, Andreas Cadhalpun wrote:
>> The avctx->width/avctx->height is not zero, but libopenjpeg_copy_unpacked8
>> does:
>
>> width = avctx->width / image->comps[compno].dx;
>> height = avctx->height / image->comps[compno].dy;
>
> this looks wrong to me
> the code in mj2_create_image() looks better:
> cmptparm[i].dx = sub_dx[i];
> cmptparm[i].dy = sub_dy[i];
> cmptparm[i].w = (avctx->width + sub_dx[i] - 1) / sub_dx[i];
> cmptparm[i].h = (avctx->height + sub_dy[i] - 1) / sub_dy[i];
Indeed this looks better, so I updated the patch (attached) to change the
calculation of width/height.
Best regards,
Andreas
>From 1461064c1eaabb71661f9ff68b94f35a1b98e3b5 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Thu, 13 Oct 2016 22:14:46 +0200
Subject: [PATCH] libopenjpegenc: fix out-of-bounds reads when filling the
edges
The calculation of width/height should round up, not round down to
prevent setting width or height to 0.
Also image->comps[compno].w is unsigned (at least in openjpeg2), so the
calculation could silently wrap around without the explicit cast to int.
Signed-off-by: Andreas Cadhalpun
---
libavcodec/libopenjpegenc.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c
index 857ee1a..1b7e168 100644
--- a/libavcodec/libopenjpegenc.c
+++ b/libavcodec/libopenjpegenc.c
@@ -421,7 +421,7 @@ static int libopenjpeg_copy_packed8(AVCodecContext *avctx, const AVFrame *frame,
for (; y < image->comps[compno].h; ++y) {
image_line = image->comps[compno].data + y * image->comps[compno].w;
for (x = 0; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - image->comps[compno].w];
+image_line[x] = image_line[x - (int)image->comps[compno].w];
}
}
}
@@ -461,7 +461,7 @@ static int libopenjpeg_copy_packed12(AVCodecContext *avctx, const AVFrame *frame
for (; y < image->comps[compno].h; ++y) {
image_line = image->comps[compno].data + y * image->comps[compno].w;
for (x = 0; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - image->comps[compno].w];
+image_line[x] = image_line[x - (int)image->comps[compno].w];
}
}
}
@@ -501,7 +501,7 @@ static int libopenjpeg_copy_packed16(AVCodecContext *avctx, const AVFrame *frame
for (; y < image->comps[compno].h; ++y) {
image_line = image->comps[compno].data + y * image->comps[compno].w;
for (x = 0; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - image->comps[compno].w];
+image_line[x] = image_line[x - (int)image->comps[compno].w];
}
}
}
@@ -528,8 +528,8 @@ static int libopenjpeg_copy_unpacked8(AVCodecContext *avctx, const AVFrame *fram
}
for (compno = 0; compno < numcomps; ++compno) {
-width = avctx->width / image->comps[compno].dx;
-height = avctx->height / image->comps[compno].dy;
+width = (avctx->width + image->comps[compno].dx - 1) / image->comps[compno].dx;
+height = (avctx->height + image->comps[compno].dy - 1) / image->comps[compno].dy;
for (y = 0; y < height; ++y) {
image_line = image->comps[compno].data + y * image->comps[compno].w;
frame_index = y * frame->linesize[compno];
@@ -542,7 +542,7 @@ static int libopenjpeg_copy_unpacked8(AVCodecContext *avctx, const AVFrame *fram
for (; y < image->comps[compno].h; ++y) {
image_line = image->comps[compno].data + y * image->comps[compno].w;
for (x = 0; x < image->comps[compno].w; ++x) {
-image_line[x] = image_line[x - image->comps[compno].w];
+image_line[x] = image_line[x - (int)image->comps[compno].w];
}
}
}
@@ -570,8 +570,8 @@ static int libopenjpeg_copy_unpacked16(AVCodecContext *avctx, const AVFrame *fra
}
for (compno = 0; compno < numcomps; ++compno) {
-width = avctx->width / image->comps[compno].dx;
-height= avctx->height / image->comps[compno].dy;
+width = (avctx->width + image->comps[compno].dx - 1) / image->comps[compno].dx;
+height= (avctx->height + image->comps[compno].dy - 1) / image->comps[compno].dy;
frame_ptr = (uint16_t *)frame->data[compno];
for (y = 0; y < height; ++y) {
image_line = image->comps[compno].data + y * image->comps[compno].w;
@
Re: [FFmpeg-devel] [PATCH] libopenjpegenc: fix out-of-bounds reads when filling the edges
On 14.10.2016 06:08, Michael Bradshaw wrote: > On Thu, Oct 13, 2016 at 6:49 PM, Michael Niedermayer > wrote: >> >>> libopenjpegenc.c | 18 +- >>> 1 file changed, 9 insertions(+), 9 deletions(-) >>> 17061aee3e88729993c9581f688cbfda01fccaac 0001-libopenjpegenc-fix-out- >> of-bounds-reads-when-filling-.patch >>> From 1461064c1eaabb71661f9ff68b94f35a1b98e3b5 Mon Sep 17 00:00:00 2001 >>> From: Andreas Cadhalpun >>> Date: Thu, 13 Oct 2016 22:14:46 +0200 >>> Subject: [PATCH] libopenjpegenc: fix out-of-bounds reads when filling the >>> edges >>> >>> The calculation of width/height should round up, not round down to >>> prevent setting width or height to 0. >>> >>> Also image->comps[compno].w is unsigned (at least in openjpeg2), so the >>> calculation could silently wrap around without the explicit cast to int. >> >> LGTM, iam not libopenjpegenc maintainer though > > > Looks good to me too. Please feel free to apply it. Thanks! Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]configure: Enable pie for toolchain=hardened.
On 14.10.2016 15:02, Carl Eugen Hoyos wrote: > 2016-10-04 12:24 GMT+02:00 Carl Eugen Hoyos : > >> Sorry if I miss something but with this patch, the hardening_check >> script succeeds here both for x86_32 and x86_64 (static and shared). > > Tested successfully on x86_64 and x86_32 Linux (pie actually works > on my very old system). On Debian hppa, the patch makes no > difference, I cannot test m68k. I guess gcc nowadays just ignores the flag if it wouldn't work. > Since my gcc manual states that -fPIE is needed for -pie, I decided > to use that. In practice it shouldn't make a difference, because -fPIC is added to CFLAGS after -fPIE anyway, which I think overrides it. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] libopenjpegenc: recreate image data buffer after encoding frame
On 14.10.2016 06:07, Michael Bradshaw wrote: > On Thu, Oct 13, 2016 at 12:21 PM, Andreas Cadhalpun < > andreas.cadhal...@googlemail.com> wrote: >> >> OK. Attached patch does that for openjpeg 2. >> I didn't change the behavior for openjpeg 1, as reusing the image works >> there. > > > Looks good to me. Thanks! Please feel free to apply it. Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH]configure: Enable pie for toolchain=hardened.
On 14.10.2016 18:28, Michael Niedermayer wrote:
> On Thu, Oct 13, 2016 at 12:56:56AM +0200, Andreas Cadhalpun wrote:
>> If you want both NX and ASLR security features for an executable it has
>> to be built with '-pie' and must not contain text relocations.
>
> this should not be true
> the difference between text relocations and lack there off is that
> without text relocations a binary is loaded and written into memory
> with text relocations the binary is loaded the addresses for
> relocations updated and writen into memory.
> There is at a theoretical level no difference in required access rights
> write to memory is neccessary at the load stage, no execute is needed
> here and once done rights can be fliped over into execute without write
> This may very well not work out that way in gnu linux but thats a
> implementation problem then not a fundamental issue in NX+ASLR+TEXRELs
> That is unless iam missing something
>
> also a simple test:
> gcc xtest.c -pie -m32 -o xtest
> int main() {
> void *ref;
> asm (
> "mov $main, %0"
> :"=r"(ref)
> );
> printf("? %p\n", ref);
> //can we read it ?
> printf("R %d\n", *(int*)ref);
> //can we write it ?
> *(int*)ref = 123;
>
> return 0;
> }
>
> Executing this shows that the write is prevented and segfaults, the
> address is different on each run and we have a text relocation in it
> thats on a ancient ubuntu without special security patches that i
> remember
Interesting...
I was just rephrasing what I found on the web [1]:
"For NX to be useful, you need to make sure that all the executable
memory pages are loaded and set in stone right away; this makes text
relocation impossible"
Best regards,
Andreas
1: https://blog.flameeyes.eu/2009/11/the-pie-is-not-exactly-a-lie/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] lavf/mp3enc: write encoder delay/padding upon closing
On 05.10.2016 21:37, Jon Toohill wrote:
> ---
> libavformat/mp3enc.c | 34 +++---
> 1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/libavformat/mp3enc.c b/libavformat/mp3enc.c
> index de63401..ddf4b93 100644
> --- a/libavformat/mp3enc.c
> +++ b/libavformat/mp3enc.c
[...]
> @@ -345,10 +342,24 @@ static int mp3_write_audio_packet(AVFormatContext *s,
> AVPacket *pkt)
> #endif
>
> if (mp3->xing_offset) {
> +uint8_t *side_data = NULL;
> +int side_data_size = 0;
> +
> mp3_xing_add_frame(mp3, pkt);
> mp3->audio_size += pkt->size;
> mp3->audio_crc = av_crc(av_crc_get_table(AV_CRC_16_ANSI_LE),
>mp3->audio_crc, pkt->data, pkt->size);
> +
> +side_data = av_packet_get_side_data(pkt,
> +AV_PKT_DATA_SKIP_SAMPLES,
> +&side_data_size);
> +if (side_data && side_data_size >= 10) {
> +mp3->padding = AV_RL32(side_data + 4) + 528 + 1;
I think this should also use FFMAX(..., 0), as the side_data could
theoretically contain
a negative value.
> +if (!mp3->delay)
> +mp3->delay = FFMAX(AV_RL32(side_data) - 528 - 1, 0);
> +} else {
> +mp3->padding = 0;
> +}
> }
> }
>
> @@ -381,7 +392,7 @@ static void mp3_update_xing(AVFormatContext *s)
> AVReplayGain *rg;
> uint16_t tag_crc;
> uint8_t *toc;
> -int i, rg_size;
> +int i, rg_size, delay;
This variable is not used.
> /* replace "Xing" identification string with "Info" for CBR files. */
> if (!mp3->has_variable_bitrate)
> @@ -422,6 +433,15 @@ static void mp3_update_xing(AVFormatContext *s)
> }
> }
>
> +/* write encoder delay/padding */
> +if (mp3->delay >= 1 << 12) {
> +av_log(s, AV_LOG_WARNING, "Too many samples of initial padding.\n");
I think this should cap mp3->delay to prevent setting unrelated bits in the
header:
mp3->delay = (1 << 12) - 1;
> +}
> +if (mp3->padding >= 1 << 12) {
> +av_log(s, AV_LOG_WARNING, "Too many samples of trailing padding.\n");
Same as above.
> +}
> +AV_WB24(mp3->xing_frame + mp3->xing_offset + 141, (mp3->delay << 12) +
> mp3->padding);
> +
> AV_WB32(mp3->xing_frame + mp3->xing_offset + XING_SIZE - 8,
> mp3->audio_size);
> AV_WB16(mp3->xing_frame + mp3->xing_offset + XING_SIZE - 4,
> mp3->audio_crc);
>
>
Otherwise this patch looks good (and is definitely better than my previous
attempt
of fixing this [1]).
Best regards,
Andreas
1: https://ffmpeg.org/pipermail/ffmpeg-devel/2015-December/185657.html
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] lavc/libmp3lame: send encoder delay/padding in packet side data
On 05.10.2016 21:37, Jon Toohill wrote:
> ---
> libavcodec/libmp3lame.c | 26 +-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c
> index 5642264..b3ba0d8 100644
> --- a/libavcodec/libmp3lame.c
> +++ b/libavcodec/libmp3lame.c
[...]
> @@ -269,6 +270,29 @@ static int mp3lame_encode_frame(AVCodecContext *avctx,
> AVPacket *avpkt,
> ff_af_queue_remove(&s->afq, avctx->frame_size, &avpkt->pts,
> &avpkt->duration);
>
> +discard_padding = avctx->frame_size - avpkt->duration;
> +// Check if subtraction resulted in an overflow
> +if ((discard_padding < avctx->frame_size) != (avpkt->duration > 0)) {
> +av_packet_unref(avpkt);
> +av_free(avpkt);
It would be nice to have an av_log here reporting the problem.
Otherwise patch LGTM.
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] matroskadec: fix NULL pointer dereference
The problem was introduced in commit 1273bc6.
Signed-off-by: Andreas Cadhalpun
---
libavformat/matroskadec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 8847c62..a5d3c0e 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext
*matroska, int64_t field_order)
/* workaround a bug in our Matroska muxer, introduced in version 57.36
alongside
* this function, and fixed in 57.52 */
-if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor, µ) ==
3)
+if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d",
&major, &minor, µ) == 3)
bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100);
switch (field_order) {
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] aiffdec: fix division by zero
Signed-off-by: Andreas Cadhalpun --- libavformat/aiffdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c index cd916f9..de82787 100644 --- a/libavformat/aiffdec.c +++ b/libavformat/aiffdec.c @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s, size = st->codecpar->block_align; break; default: -size = (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align; +size = st->codecpar->block_align ? (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE; } size = FFMIN(max_size, size); res = av_get_packet(s->pb, pkt, size); -- 2.9.3 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] astdec: fix division by zero
Signed-off-by: Andreas Cadhalpun --- libavformat/astdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/astdec.c b/libavformat/astdec.c index f3ca721..7a53d0b 100644 --- a/libavformat/astdec.c +++ b/libavformat/astdec.c @@ -90,7 +90,7 @@ static int ast_read_packet(AVFormatContext *s, AVPacket *pkt) pos = avio_tell(s->pb); type = avio_rl32(s->pb); size = avio_rb32(s->pb); -if (size > INT_MAX / s->streams[0]->codecpar->channels) +if (!s->streams[0]->codecpar->channels || size > INT_MAX / s->streams[0]->codecpar->channels) return AVERROR_INVALIDDATA; size *= s->streams[0]->codecpar->channels; -- 2.9.3 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] westwood_aud: prevent division by zero
Signed-off-by: Andreas Cadhalpun
---
libavformat/westwood_aud.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/libavformat/westwood_aud.c b/libavformat/westwood_aud.c
index 4750167..9c2d35c 100644
--- a/libavformat/westwood_aud.c
+++ b/libavformat/westwood_aud.c
@@ -164,6 +164,12 @@ static int wsaud_read_packet(AVFormatContext *s,
if (ret != chunk_size)
return AVERROR(EIO);
+if (st->codecpar->channels <= 0) {
+av_log(s, AV_LOG_ERROR, "invalid number of channels %d\n",
+ st->codecpar->channels);
+return AVERROR_INVALIDDATA;
+}
+
/* 2 samples/byte, 1 or 2 samples per frame depending on stereo */
pkt->duration = (chunk_size * 2) / st->codecpar->channels;
}
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] matroskadec: fix NULL pointer dereference
On 17.10.2016 15:44, James Almer wrote:
> On 10/17/2016 4:47 AM, Benoit Fouet wrote:
>> On 17/10/2016 06:49, James Almer wrote:
>>> On 10/16/2016 9:30 PM, James Almer wrote:
>>>> On 10/16/2016 5:11 PM, Andreas Cadhalpun wrote:
>>>>> The problem was introduced in commit 1273bc6.
>>>>>
>>>>> Signed-off-by: Andreas Cadhalpun
>>>>> ---
>>>>> libavformat/matroskadec.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>>>> index 8847c62..a5d3c0e 100644
>>>>> --- a/libavformat/matroskadec.c
>>>>> +++ b/libavformat/matroskadec.c
>>>>> @@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext
>>>>> *matroska, int64_t field_order)
>>>>> /* workaround a bug in our Matroska muxer, introduced in version
>>>>> 57.36 alongside
>>>>>* this function, and fixed in 57.52 */
>>>>> -if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", &major, &minor,
>>>>> µ) == 3)
>>>>> +if (matroska->muxingapp && sscanf(matroska->muxingapp,
>>>>> "Lavf%d.%d.%d", &major, &minor, µ) == 3)
>>>> LGTM.
>>>>
>>>> Matroska files are supposed to always have that element, but even ffmpeg
>>>> used
>>>> to mux files without it at some point when bitexact flag was enabled, so i
>>>> guess plenty of files out there are missing it.
>>>>
>>>>> bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >=
>>>>> 100);
>>>>> switch (field_order) {
>>>>>
>>> Just tried a file missing the muxingapp element, meaning matroska->muxingapp
>>> is NULL, and sscanf simply returns -1 and sets errno to EINVAL.
>>>
>>> Where does it crash for you and using what file?
>>
>> FWIW, I've just written a quick test on my machine, and an "sscanf(NULL,
>> "%d", &i)" segfaults.
With glibc it crashes:
$ gdb --batch -ex r -ex bt -ex q --args ./ffmpeg_g -i
./id_b42167853bffe74a3a93ea312837dd8a2a25a951.matroska -f null /dev/null
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
ffmpeg version N-82012-gd3874b7 Copyright (c) 2000-2016 the FFmpeg developers
built with gcc 6.2.0 (Debian 6.2.0-6) 20161010
configuration: --disable-optimizations
libavutil 55. 32.100 / 55. 32.100
libavcodec 57. 61.103 / 57. 61.103
libavformat57. 52.100 / 57. 52.100
libavdevice57. 0.102 / 57. 0.102
libavfilter 6. 64.100 / 6. 64.100
libswscale 4. 1.100 / 4. 1.100
libswresample 2. 2.100 / 2. 2.100
[matroska,webm @ 0x2380fc0] Unknown EBML doctype 'ma@roUka'
Truncating packet of size 134217728 to 1551
Truncating packet of size 1431346 to 1507
Truncating packet of size 4467 to 1401
[matroska,webm @ 0x2380fc0] Duplicate element
[matroska,webm @ 0x2380fc0] Unknown/unsupported AVCodecID ��OPU=.
Ignoring attempt to set invalid timebase 0/1 for st:0
Program received signal SIGSEGV, Segmentation fault.
rawmemchr () at ../sysdeps/x86_64/rawmemchr.S:37
37 ../sysdeps/x86_64/rawmemchr.S: Datei oder Verzeichnis nicht gefunden.
#0 rawmemchr () at ../sysdeps/x86_64/rawmemchr.S:37
#1 0x74b383a2 in _IO_str_init_static_internal
(sf=sf@entry=0x7fffd360, ptr=ptr@entry=0x0, size=size@entry=0,
pstart=pstart@entry=0x0) at strops.c:41
#2 0x74b27567 in __GI___isoc99_vsscanf (string=0x0, format=0x15e597c
"Lavf%d.%d.%d", args=args@entry=0x7fffd488) at isoc99_vsscanf.c:41
#3 0x74b27507 in __isoc99_sscanf (s=, format=) at isoc99_sscanf.c:31
#4 0x006ca2eb in mkv_field_order (matroska=0x2381ba0, field_order=2)
at src/libavformat/matroskadec.c:1762
#5 0x006cc0c2 in matroska_parse_tracks (s=0x2380fc0) at
src/libavformat/matroskadec.c:2293
#6 0x006ccc83 in matroska_read_header (s=0x2380fc0) at
src/libavformat/matroskadec.c:2470
#7 0x007bbd13 in avformat_open_input (ps=0x7fffd948,
filename=0x7fffe3a4
"./id_b42167853bffe74a3a93ea312837dd8a2a25a951.matroska", fmt=0x0,
options=0x2380d68) at src/libavformat/utils.c:587
#8 0x00411aeb in open_input_file (o=0x7fffda50,
filename=0x7fffe3a4
"./id_b42167853bffe74a3a93ea312837dd8a2a25a951.matroska") at
src/ffmpeg_opt.c:997
#9 0x0041a9af in open_files (l=0x2380c98, inout=0x154c797 "input",
open_file=0x411330 ) at src/ffmpeg_opt.c:3135
#10 0x0041ab10 in ffmpeg_parse_options (argc=6, argv=0x7fffe048) at
src/ffmpeg_opt.c:3175
#11 0x0042e78a in main (argc=6, argv=0x7fffe048) at
src/ffmpeg.c:4564
> Guess it's up to the implementation, then. I tried with mingw-w64 only.
> The documentation for sscanf says it sets errno to EINVAL if *format is NULL,
> but doesn't mention *str.
>
> Patch still LGTM. It doesn't hurt either way.
Pushed.
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] aiffdec: fix division by zero
On 17.10.2016 05:43, Michael Niedermayer wrote: > On Sun, Oct 16, 2016 at 10:38:42PM +0200, Andreas Cadhalpun wrote: >> Signed-off-by: Andreas Cadhalpun >> --- >> libavformat/aiffdec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c >> index cd916f9..de82787 100644 >> --- a/libavformat/aiffdec.c >> +++ b/libavformat/aiffdec.c >> @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s, >> size = st->codecpar->block_align; >> break; >> default: >> -size = (MAX_SIZE / st->codecpar->block_align) * >> st->codecpar->block_align; >> +size = st->codecpar->block_align ? (MAX_SIZE / >> st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE; > > how do you reach block_align == 0 ? > aiff_read_header() checks for block_align == 0 I'm not aware of a way to reproduce this with the ffmpeg binary, however an API user (e.g. my fuzz-testing-program) can change codecpar->codec_type and codecpar->codec_id to force decoding a stream with a particular codec. However, avcodec_parameters_from_context sets codecpar->block_align to 0 for AVMEDIA_TYPE_VIDEO thus causing the subsequent crash. It's similar for the other two division by zero patches. So an alternative approach would be to set all parameter fields from the codec in avcodec_parameters_from_context, but I prefer making the code more robust. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] aiffdec: fix division by zero
On 17.10.2016 17:13, Michael Niedermayer wrote: > On Mon, Oct 17, 2016 at 04:17:35PM +0200, Andreas Cadhalpun wrote: >> On 17.10.2016 05:43, Michael Niedermayer wrote: >>> On Sun, Oct 16, 2016 at 10:38:42PM +0200, Andreas Cadhalpun wrote: >>>> Signed-off-by: Andreas Cadhalpun >>>> --- >>>> libavformat/aiffdec.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c >>>> index cd916f9..de82787 100644 >>>> --- a/libavformat/aiffdec.c >>>> +++ b/libavformat/aiffdec.c >>>> @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s, >>>> size = st->codecpar->block_align; >>>> break; >>>> default: >>>> -size = (MAX_SIZE / st->codecpar->block_align) * >>>> st->codecpar->block_align; >>>> +size = st->codecpar->block_align ? (MAX_SIZE / >>>> st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE; >>> >>> how do you reach block_align == 0 ? >>> aiff_read_header() checks for block_align == 0 >> >> I'm not aware of a way to reproduce this with the ffmpeg binary, however >> an API user (e.g. my fuzz-testing-program) can change codecpar->codec_type >> and codecpar->codec_id to force decoding a stream with a particular codec. >> >> However, avcodec_parameters_from_context sets codecpar->block_align to 0 >> for AVMEDIA_TYPE_VIDEO thus causing the subsequent crash. > > hmm, patch is probably ok then Pushed. What about the similar patches for astdec and westwood_aud? Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] aiffdec: fix division by zero
On 17.10.2016 20:11, Michael Niedermayer wrote: > On Mon, Oct 17, 2016 at 06:27:29PM +0200, Andreas Cadhalpun wrote: >> On 17.10.2016 17:13, Michael Niedermayer wrote: >>> On Mon, Oct 17, 2016 at 04:17:35PM +0200, Andreas Cadhalpun wrote: >>>> On 17.10.2016 05:43, Michael Niedermayer wrote: >>>>> On Sun, Oct 16, 2016 at 10:38:42PM +0200, Andreas Cadhalpun wrote: >>>>>> Signed-off-by: Andreas Cadhalpun >>>>>> --- >>>>>> libavformat/aiffdec.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c >>>>>> index cd916f9..de82787 100644 >>>>>> --- a/libavformat/aiffdec.c >>>>>> +++ b/libavformat/aiffdec.c >>>>>> @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s, >>>>>> size = st->codecpar->block_align; >>>>>> break; >>>>>> default: >>>>>> -size = (MAX_SIZE / st->codecpar->block_align) * >>>>>> st->codecpar->block_align; >>>>>> +size = st->codecpar->block_align ? (MAX_SIZE / >>>>>> st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE; >>>>> >>>>> how do you reach block_align == 0 ? >>>>> aiff_read_header() checks for block_align == 0 >>>> >>>> I'm not aware of a way to reproduce this with the ffmpeg binary, however >>>> an API user (e.g. my fuzz-testing-program) can change codecpar->codec_type >>>> and codecpar->codec_id to force decoding a stream with a particular codec. >>>> >>>> However, avcodec_parameters_from_context sets codecpar->block_align to 0 >>>> for AVMEDIA_TYPE_VIDEO thus causing the subsequent crash. >>> >>> hmm, patch is probably ok then >> >> Pushed. >> >> What about the similar patches for astdec and westwood_aud? > > probably ok too Thanks, pushed both. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat: close parser if codec changed
The parser depends on the codec and thus must not be used with a different one.
If it is, the 'avctx->codec_id == s->parser->codec_ids[0] ...' assert in
av_parser_parse2 gets triggered.
Signed-off-by: Andreas Cadhalpun
---
libavformat/utils.c | 12
1 file changed, 12 insertions(+)
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 8a51aea..a581994 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -480,6 +480,12 @@ static int update_stream_avctx(AVFormatContext *s)
if (!st->internal->need_context_update)
continue;
+/* close parser, because it depends on the codec */
+if (st->parser) {
+av_parser_close(st->parser);
+st->parser = NULL;
+}
+
/* update internal codec context, for the parser */
ret = avcodec_parameters_to_context(st->internal->avctx, st->codecpar);
if (ret < 0)
@@ -1515,6 +1521,12 @@ static int read_frame_internal(AVFormatContext *s,
AVPacket *pkt)
st->info->found_decoder = 0;
}
+/* close parser, because it depends on the codec */
+if (st->parser) {
+av_parser_close(st->parser);
+st->parser = NULL;
+}
+
ret = avcodec_parameters_to_context(st->internal->avctx,
st->codecpar);
if (ret < 0)
return ret;
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 0/2] Fix gapless encoding/remuxing for MP3
On 17.10.2016 23:55, Jon Toohill wrote: > Round trip wav->mp3->wav now preserves the correct number of samples. > Remuxing mp3->mp3 with -c:a copy also preserves any existing gapless > metadata in the Info tag. > > The code in libmp3lame.c to set AV_PKT_DATA_SKIP_SAMPLES was mostly > copied from libopusenc.c. > > Jon Toohill (2): > lavc/libmp3lame: send encoder delay/padding in packet side data > lavf/mp3enc: write encoder delay/padding upon closing > > libavcodec/libmp3lame.c | 27 ++- > libavformat/mp3enc.c| 34 -- > 2 files changed, 54 insertions(+), 7 deletions(-) > I've applied both patches. Thanks for fixing this! Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet
Nothing guarantees to set request_probe to -1, so this assert can be triggered, e.g. if st->probe_packets is 0. Signed-off-by: Andreas Cadhalpun --- I think the reason why this assert isn't triggered way more often is that the probing code usually works quite well. --- libavformat/utils.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libavformat/utils.c b/libavformat/utils.c index 8a51aea..a62a073 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -806,7 +806,6 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) if (st->probe_packets) if ((err = probe_codec(s, st, NULL)) < 0) return err; -av_assert0(st->request_probe <= 0); } continue; } -- 2.9.3 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet
On 18.10.2016 22:56, Michael Niedermayer wrote:
> On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
>> Nothing guarantees to set request_probe to -1, so this assert can be
>> triggered, e.g. if st->probe_packets is 0.
>
> probe_codec() called with NULL should cause
> st->probe_packets = 0
> st->request_probe = -1;
Yes, but request_probe can be change to a different value later on,
e.g. in ff_parse_mpeg2_descriptor:
int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
{
[...]
if (s->internal->raw_packet_buffer_remaining_size <= 0)
if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets = 0,
request_probe = -1
return err;
[...]
ret = s->iformat->read_packet(s, pkt);
~~~
ff_parse_mpeg2_descriptor([...])
{
[...]
switch (desc_tag) {
[...]
case 0x05: /* registration descriptor */
[...]
st->request_probe = 50;
[...]
}
~~~
[...]
if (st->probe_packets) // still 0
if ((err = probe_codec(s, st, NULL)) < 0)
return err;
av_assert0(st->request_probe <= 0); // now 50
SIGABRT
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet
On 18.10.2016 23:46, Hendrik Leppkes wrote:
> On Tue, Oct 18, 2016 at 11:26 PM, Andreas Cadhalpun
> wrote:
>> On 18.10.2016 22:56, Michael Niedermayer wrote:
>>> On Tue, Oct 18, 2016 at 10:31:37PM +0200, Andreas Cadhalpun wrote:
>>>> Nothing guarantees to set request_probe to -1, so this assert can be
>>>> triggered, e.g. if st->probe_packets is 0.
>>>
>>> probe_codec() called with NULL should cause
>>> st->probe_packets = 0
>>> st->request_probe = -1;
>>
>> Yes, but request_probe can be change to a different value later on,
>> e.g. in ff_parse_mpeg2_descriptor:
>>
>> int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
>> {
>> [...]
>> if (s->internal->raw_packet_buffer_remaining_size <= 0)
>> if ((err = probe_codec(s, st, NULL)) < 0) // probe_packets =
>> 0, request_probe = -1
>> return err;
>> [...]
>> ret = s->iformat->read_packet(s, pkt);
>> ~~~
>> ff_parse_mpeg2_descriptor([...])
>> {
>> [...]
>> switch (desc_tag) {
>> [...]
>> case 0x05: /* registration descriptor */
>> [...]
>> st->request_probe = 50;
>> [...]
>> }
>> ~~~
>> [...]
>> if (st->probe_packets) // still 0
>> if ((err = probe_codec(s, st, NULL)) < 0)
>> return err;
>> av_assert0(st->request_probe <= 0); // now 50
>> SIGABRT
>>
>
> Can you actually make that happen, or is that just speculation?
Yes, at least in ffmpeg 3.1.4 and master with commit 04fa20d reverted.
(I do fuzz-testing, not speculating.)
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()
On 19.10.2016 04:15, James Almer wrote: > On 10/17/2016 9:57 PM, Michael Niedermayer wrote: >> On Sun, Oct 16, 2016 at 09:34:50PM -0300, James Almer wrote: >>> Fixes valgrind warning about "Conditional jump or move depends on >>> uninitialised value(s)" >>> >>> Signed-off-by: James Almer >>> --- >>> libavformat/mov.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> This should be suppressable by adding it to >> tests/fate-valgrind.supp > > I'll leave that to someone else. No idea how to add stuff to that file. > > Should i drop this patch? Zero initializing a buffer in stack wouldn't > hurt IMO. I prefer fixing such things in the code if it's reasonable possible, which is the case here. In other words, the patch looks good to me. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet
On 19.10.2016 05:29, Michael Niedermayer wrote:
> hmm, i guess the patch is then ok
> alternatively you could use i think:
> @@ -803,7 +803,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
> return ret;
> for (i = 0; i < s->nb_streams; i++) {
> st = s->streams[i];
> -if (st->probe_packets)
> +if (st->probe_packets || st->request_probe > 0)
> if ((err = probe_codec(s, st, NULL)) < 0)
> return err;
> av_assert0(st->request_probe <= 0);
Yes, this works fine and should guarantee that the assert can't be triggered.
Patch doing it that way is attached.
Best regards,
Andreas
>From 7912c6f200a37130844221a73941a7971afa6455 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Wed, 19 Oct 2016 19:23:49 +0200
Subject: [PATCH] avformat: prevent triggering request_probe assert in
ff_read_packet
If probe_codec is called with pkt == NULL, it sets probe_packets to 0
and request_probe to -1.
However, request_probe can change when calling s->iformat->read_packet
and thus a probe_packets value of 0 doesn't guarantee a request_probe
value of -1.
In that case calling probe_codec again is necessary to prevent
triggering the assert.
Signed-off-by: Andreas Cadhalpun
---
libavformat/utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 8a51aea..70dbfa1 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -803,7 +803,7 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt)
return ret;
for (i = 0; i < s->nb_streams; i++) {
st = s->streams[i];
-if (st->probe_packets)
+if (st->probe_packets || st->request_probe > 0)
if ((err = probe_codec(s, st, NULL)) < 0)
return err;
av_assert0(st->request_probe <= 0);
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/8] compat/cuda: add dynamic loader
On 19.10.2016 18:52, Sven C. Dack wrote: > On 19/10/16 17:18, Hendrik Leppkes wrote: >> Thats the general interpretation of the license situation. If you >> include non-free headers, your binary becomes non-free, hence why >> building with cuda currently requires the --enable-nonfree option. > No. This is a generalization and cannot make sense. At best does it > ignore the individual licenses and their particular terms and > discards them for a convenience. > > From what I can tell does only one condition apply here, which is > regarding the use of the header files. There is however no > reproduction, disclosure, distribution or modification of these > headers happening here, which is what is prohibited by Nvidia. I think you are missing the main problem here: FFmpeg is licensed under the LGPL 2.1, which states [1]: " 4. You may copy and distribute the Library [...] in object code or executable form [...] provided that you accompany it with the complete corresponding machine-readable source code" If during compilation the cuda.h header is used, it is part of the complete source code and thus the license requires it to be distributed together with the object code. However, you say that Nvidia prohibits re-distribution of this header and as a result the compiled ffmpeg binaries cannot legally be distributed. This is why it requires --enable-nonfree. Best regards, Andreas 1: https://www.gnu.org/licenses/lgpl-2.1.html ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] aiffdec: fix division by zero
This is similar to commit c143a9c. Signed-off-by: Andreas Cadhalpun --- libavformat/aiffdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c index de82787..d96fc1d 100644 --- a/libavformat/aiffdec.c +++ b/libavformat/aiffdec.c @@ -391,7 +391,7 @@ static int aiff_read_packet(AVFormatContext *s, pkt->flags &= ~AV_PKT_FLAG_CORRUPT; /* Only one stream in an AIFF file */ pkt->stream_index = 0; -pkt->duration = (res / st->codecpar->block_align) * aiff->block_duration; +pkt->duration = (res / (st->codecpar->block_align ? st->codecpar->block_align : 1)) * aiff->block_duration ; return 0; } -- 2.9.3 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/8] compat/cuda: add dynamic loader
On 19.10.2016 20:46, Sven C. Dack wrote: > No. This is exactly what I meant with wearing tin foil hats. Insults won't help you. > Just because a compiler includes information provided by header > files into the compilation process does this not imply a transfer > of ownership or copyright of this information. The header files > do not become a part of the source code. You may find the ownership > and copyright only no longer being easily distinguishable once it > becomes a binary. The seperation is however still present and there > is no magical transfer of ownership happening here. Even if there > was, who is to say ffmpeg isn't becoming part of the header files > and thus is now being owned by Nvidia?!? I wrote nothing about ownership. > For this reason can such an interpretation of the GPL not make > good sense. It's just absurd. Have you actually read the license? It clearly defines what complete source code means: "For a library, complete source code means all the source code for all modules it contains, plus any associated interface definition files, plus the scripts used to control compilation and installation of the library." The FAQ explains this as [1]: "For a typical C program, this translates into all the source code (.c files) plus header files (.h files) plus the scripts used to control compilation and installation." > How is this with compiling under Windows or just using Intel's or > Microsoft's compiler? Does this make ffmpeg non-free or does ffmpeg > there claiming ownership over Intel's or Microsoft's header files? Read the FAQ: "What about the compiler, the toolchain?" Best regards, Andreas 1: http://gpl-violations.org/faq/sourcecode-faq/ ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] dcstr: fix division by zero
Signed-off-by: Andreas Cadhalpun --- libavformat/dcstr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/dcstr.c b/libavformat/dcstr.c index 69fae41..d5d2281 100644 --- a/libavformat/dcstr.c +++ b/libavformat/dcstr.c @@ -47,7 +47,7 @@ static int dcstr_read_header(AVFormatContext *s) avio_skip(s->pb, 4); st->duration = avio_rl32(s->pb); st->codecpar->channels *= avio_rl32(s->pb); -if (!align || align > INT_MAX / st->codecpar->channels) +if (!align || !st->codecpar->channels || align > INT_MAX / st->codecpar->channels) return AVERROR_INVALIDDATA; st->codecpar->block_align = align * st->codecpar->channels; -- 2.9.3 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/8] compat/cuda: add dynamic loader
On 19.10.2016 21:41, Sven C. Dack wrote: > You are misinterpreting it. The FAQ explicitly excludes external > components such as compilers, kernels and libraries. What is or is not a system component is quite debatable. However, it always depends on the specific operating system under consideration. For example, my operating system doesn't (normally) come with the cuda header. However, a binary built with the header available could run on it. This is problematic, because I couldn't rebuild the binary on the system from the distributed source code. > Anyhow, I don't want to challenge the interpretation. If this > is what you want then I'm happy with the options as they are. > I'm not bothered. :) OK. :) Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: remove request_probe assert from ff_read_packet
On 19.10.2016 22:45, Michael Niedermayer wrote: > On Wed, Oct 19, 2016 at 07:27:59PM +0200, Andreas Cadhalpun wrote: >> f1f2b8b10efdab27848eeb2e2bac646eda08a175 >> 0001-avformat-prevent-triggering-request_probe-assert-in-.patch >> From 7912c6f200a37130844221a73941a7971afa6455 Mon Sep 17 00:00:00 2001 >> From: Andreas Cadhalpun >> Date: Wed, 19 Oct 2016 19:23:49 +0200 >> Subject: [PATCH] avformat: prevent triggering request_probe assert in >> ff_read_packet >> >> If probe_codec is called with pkt == NULL, it sets probe_packets to 0 >> and request_probe to -1. >> However, request_probe can change when calling s->iformat->read_packet >> and thus a probe_packets value of 0 doesn't guarantee a request_probe >> value of -1. >> In that case calling probe_codec again is necessary to prevent >> triggering the assert. >> >> Signed-off-by: Andreas Cadhalpun >> --- >> libavformat/utils.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > LGTM Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] rsd: limit number of channels
Negative values don't make sense and too large values can cause
overflows. For AV_CODEC_ID_ADPCM_THP this leads to a too small extradata
buffer being allocated, causing out-of-bounds writes.
Signed-off-by: Andreas Cadhalpun
---
libavformat/rsd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavformat/rsd.c b/libavformat/rsd.c
index ee6fdfb..5a56e72 100644
--- a/libavformat/rsd.c
+++ b/libavformat/rsd.c
@@ -84,8 +84,10 @@ static int rsd_read_header(AVFormatContext *s)
}
par->channels = avio_rl32(pb);
-if (!par->channels)
+if (par->channels <= 0 || par->channels > INT_MAX / 36) {
+av_log(s, AV_LOG_ERROR, "Invalid number of channels: %d\n",
par->channels);
return AVERROR_INVALIDDATA;
+}
avio_skip(pb, 4); // Bit depth
par->sample_rate = avio_rl32(pb);
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] rsd: limit number of channels
On 20.10.2016 02:04, Michael Niedermayer wrote: > On Wed, Oct 19, 2016 at 11:46:43PM +0200, Andreas Cadhalpun wrote: >> Negative values don't make sense and too large values can cause >> overflows. For AV_CODEC_ID_ADPCM_THP this leads to a too small extradata >> buffer being allocated, causing out-of-bounds writes. >> >> Signed-off-by: Andreas Cadhalpun >> --- >> libavformat/rsd.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) > > LGTM Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] aiffdec: fix division by zero
On 20.10.2016 02:56, Michael Niedermayer wrote:
> On Wed, Oct 19, 2016 at 09:18:51PM +0200, Andreas Cadhalpun wrote:
>> This is similar to commit c143a9c.
>>
>> Signed-off-by: Andreas Cadhalpun
>> ---
>> libavformat/aiffdec.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> can aiff work without block_align ?
Well, it can use a fall-back value. That value can be wrong, of course.
> either way, block_duration is from the header reading
> if its still accurate then using it together with 1 instead of the
> matching block align is quite likely not correct
> OTOH if block_duration does not represent the actual content then
> the duration would only be correct by pure chance
>
> Its a bit unfortunate that theres no usecase with an undamaged sample
> which would have clear correct values
>
> one has to work on the assumptation of a use case where the user needs
> to override the codec and then ask "what is correct to do" that makes
> this a bit tricky ...
Alternatively aiff_read_packet could just error out, if you prefer that.
Patch doing that attached.
Best regards,
Andreas
>From d1edb842a886de0bae6e32ac602f2fef6810081a Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Thu, 20 Oct 2016 20:08:15 +0200
Subject: [PATCH] aiff: check block_align in aiff_read_packet
It can be unset in avcodec_parameters_from_context and a value of 0
causes SIGFPE crashes.
Signed-off-by: Andreas Cadhalpun
---
libavformat/aiffdec.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
index de82787..59e969d 100644
--- a/libavformat/aiffdec.c
+++ b/libavformat/aiffdec.c
@@ -371,6 +371,11 @@ static int aiff_read_packet(AVFormatContext *s,
if (max_size <= 0)
return AVERROR_EOF;
+if (!st->codecpar->block_align) {
+av_log(s, AV_LOG_ERROR, "block_align not set\n");
+return AVERROR_INVALIDDATA;
+}
+
/* Now for that packet */
switch (st->codecpar->codec_id) {
case AV_CODEC_ID_ADPCM_IMA_QT:
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] dcstr: fix division by zero
On 20.10.2016 02:59, Michael Niedermayer wrote:
> On Wed, Oct 19, 2016 at 10:41:22PM +0200, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun
>> ---
>> libavformat/dcstr.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/dcstr.c b/libavformat/dcstr.c
>> index 69fae41..d5d2281 100644
>> --- a/libavformat/dcstr.c
>> +++ b/libavformat/dcstr.c
>> @@ -47,7 +47,7 @@ static int dcstr_read_header(AVFormatContext *s)
>> avio_skip(s->pb, 4);
>> st->duration = avio_rl32(s->pb);
>
>> st->codecpar->channels *= avio_rl32(s->pb);
>
> This here can overflow and needs a check
Yes.
>
>> -if (!align || align > INT_MAX / st->codecpar->channels)
>> +if (!align || !st->codecpar->channels || align > INT_MAX /
>> st->codecpar->channels)
>> return AVERROR_INVALIDDATA;
>
> might need a <0 check too should be ok otherwise
OK. New patch attached.
Best regards,
Andreas
>From 656f4ea3f664417197a622dcf80284e890caa849 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Thu, 20 Oct 2016 20:13:54 +0200
Subject: [PATCH] dcstr: fix division by zero
Also check for possible overflows.
Signed-off-by: Andreas Cadhalpun
---
libavformat/dcstr.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/libavformat/dcstr.c b/libavformat/dcstr.c
index 69fae41..6035dd4 100644
--- a/libavformat/dcstr.c
+++ b/libavformat/dcstr.c
@@ -33,6 +33,7 @@ static int dcstr_probe(AVProbeData *p)
static int dcstr_read_header(AVFormatContext *s)
{
unsigned codec, align;
+int mult;
AVStream *st;
st = avformat_new_stream(s, NULL);
@@ -46,7 +47,12 @@ static int dcstr_read_header(AVFormatContext *s)
align = avio_rl32(s->pb);
avio_skip(s->pb, 4);
st->duration = avio_rl32(s->pb);
-st->codecpar->channels *= avio_rl32(s->pb);
+mult = avio_rl32(s->pb);
+if (st->codecpar->channels <= 0 || mult <= 0 || mult > INT_MAX / st->codecpar->channels) {
+av_log(s, AV_LOG_ERROR, "invalid number of channels %d x %d\n", st->codecpar->channels, mult);
+return AVERROR_INVALIDDATA;
+}
+st->codecpar->channels *= mult;
if (!align || align > INT_MAX / st->codecpar->channels)
return AVERROR_INVALIDDATA;
st->codecpar->block_align = align * st->codecpar->channels;
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/2] avformat/mov: zero initialize codec_name in mov_parse_stsd_video()
On 20.10.2016 03:25, Michael Niedermayer wrote: > On Wed, Oct 19, 2016 at 07:30:29PM +0200, Andreas Cadhalpun wrote: >> On 19.10.2016 04:15, James Almer wrote: >>> On 10/17/2016 9:57 PM, Michael Niedermayer wrote: >>>> On Sun, Oct 16, 2016 at 09:34:50PM -0300, James Almer wrote: >>>>> Fixes valgrind warning about "Conditional jump or move depends on >>>>> uninitialised value(s)" >>>>> >>>>> Signed-off-by: James Almer >>>>> --- >>>>> libavformat/mov.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> This should be suppressable by adding it to >>>> tests/fate-valgrind.supp >>> >>> I'll leave that to someone else. No idea how to add stuff to that file. >>> >>> Should i drop this patch? Zero initializing a buffer in stack wouldn't >>> hurt IMO. >> >> I prefer fixing such things in the code if it's reasonable possible, which >> is the case here. In other words, the patch looks good to me. > > I think the bug is that valgrind misinterprets highly optimized libc/gcc > code, i might be wrong though as i didnt disassemble and analyze this, > that was just my feeling ... > But if true a change to ffmpeg can only workaround but not fix this Yes, this seems to be a false positive. But working around it is good, because it increases the signal to noise ratio of valgrind. This is similar to false positive compiler warnings: Not working around them has high chances of wasting the time of the next one to look into it. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] cavsdec: unref frame before referencing again
This fixes asserts (from commit 13aae8) in av_frame_ref and av_frame_move_ref. Signed-off-by: Andreas Cadhalpun --- libavcodec/cavsdec.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavcodec/cavsdec.c b/libavcodec/cavsdec.c index 70ac6f8..fed7043 100644 --- a/libavcodec/cavsdec.c +++ b/libavcodec/cavsdec.c @@ -1217,6 +1217,8 @@ static int cavs_decode_frame(AVCodecContext *avctx, void *data, int *got_frame, h->got_keyframe = 1; } case PIC_PB_START_CODE: +if (*got_frame) +av_frame_unref(data); *got_frame = 0; if (!h->got_keyframe) break; -- 2.9.3 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] mpeg12dec: unref discarded picture from extradata
Otherwise another frame gets referenced into picture, triggering an assert
(from commit 13aae8) in av_frame_ref.
Signed-off-by: Andreas Cadhalpun
---
libavcodec/mpeg12dec.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 44f7b61..ac8160d 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -2808,6 +2808,7 @@ static int mpeg_decode_frame(AVCodecContext *avctx, void
*data,
avctx->extradata, avctx->extradata_size);
if (*got_output) {
av_log(avctx, AV_LOG_ERROR, "picture in extradata\n");
+av_frame_unref(picture);
*got_output = 0;
}
s->extradata_decoded = 1;
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] dcstr: fix division by zero
On 21.10.2016 01:37, Michael Niedermayer wrote: > On Thu, Oct 20, 2016 at 08:19:00PM +0200, Andreas Cadhalpun wrote: >> dcstr.c |8 +++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> 365ebc3a050f6754a981340e0a8df5dbf781 >> 0001-dcstr-fix-division-by-zero.patch >> From 656f4ea3f664417197a622dcf80284e890caa849 Mon Sep 17 00:00:00 2001 >> From: Andreas Cadhalpun >> Date: Thu, 20 Oct 2016 20:13:54 +0200 >> Subject: [PATCH] dcstr: fix division by zero >> >> Also check for possible overflows. >> >> Signed-off-by: Andreas Cadhalpun >> --- >> libavformat/dcstr.c | 8 +++- >> 1 file changed, 7 insertions(+), 1 deletion(-) > > LGTM Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] cavsdec: unref frame before referencing again
On 21.10.2016 02:31, Michael Niedermayer wrote: > On Thu, Oct 20, 2016 at 10:16:17PM +0200, Andreas Cadhalpun wrote: >> This fixes asserts (from commit 13aae8) in av_frame_ref and >> av_frame_move_ref. >> >> Signed-off-by: Andreas Cadhalpun >> --- >> libavcodec/cavsdec.c | 2 ++ >> 1 file changed, 2 insertions(+) > > should be ok Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mpeg12dec: unref discarded picture from extradata
On 21.10.2016 02:36, Michael Niedermayer wrote: > On Thu, Oct 20, 2016 at 10:57:15PM +0200, Andreas Cadhalpun wrote: >> Otherwise another frame gets referenced into picture, triggering an assert >> (from commit 13aae8) in av_frame_ref. >> >> Signed-off-by: Andreas Cadhalpun >> --- >> libavcodec/mpeg12dec.c | 1 + >> 1 file changed, 1 insertion(+) > > should be ok Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] aiffdec: fix division by zero
On 21.10.2016 01:32, Michael Niedermayer wrote: > On Thu, Oct 20, 2016 at 08:11:19PM +0200, Andreas Cadhalpun wrote: >> aiffdec.c |5 + >> 1 file changed, 5 insertions(+) >> 2fb78e5573b52b635b5077a265a54542e054cf02 >> 0001-aiff-check-block_align-in-aiff_read_packet.patch >> From d1edb842a886de0bae6e32ac602f2fef6810081a Mon Sep 17 00:00:00 2001 >> From: Andreas Cadhalpun >> Date: Thu, 20 Oct 2016 20:08:15 +0200 >> Subject: [PATCH] aiff: check block_align in aiff_read_packet >> >> It can be unset in avcodec_parameters_from_context and a value of 0 >> causes SIGFPE crashes. > > LGTM Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] doc: fix spelling errors
On 13.10.2016 23:25, Andreas Cadhalpun wrote: > Thanks to Mathieu Malaterre for reporting the > Que/Queue typo. (https://bugs.debian.org/839542) > > Signed-off-by: Andreas Cadhalpun > --- > doc/Doxyfile| 2 +- > doc/encoders.texi | 2 +- > doc/ffprobe.texi| 2 +- > doc/filters.texi| 10 +- > doc/muxers.texi | 6 +++--- > ffmpeg.c| 2 +- > ffmpeg_cuvid.c | 4 ++-- > libavcodec/aaccoder_twoloop.h | 2 +- > libavcodec/cabac.c | 2 +- > libavcodec/ffjni.c | 2 +- > libavcodec/mediacodec_wrapper.h | 2 +- > libavcodec/psymodel.h | 2 +- > libavcodec/x86/vp9lpf_16bpp.asm | 2 +- > libavfilter/af_hdcd.c | 2 +- > libavfilter/f_ebur128.c | 2 +- > libavformat/internal.h | 2 +- > libavutil/frame.h | 2 +- > libavutil/tree.h| 2 +- > 18 files changed, 25 insertions(+), 25 deletions(-) Ping. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: close parser if codec changed
On 17.10.2016 20:49, Andreas Cadhalpun wrote: > The parser depends on the codec and thus must not be used with a different > one. > If it is, the 'avctx->codec_id == s->parser->codec_ids[0] ...' assert in > av_parser_parse2 gets triggered. > > Signed-off-by: Andreas Cadhalpun > --- > libavformat/utils.c | 12 > 1 file changed, 12 insertions(+) Ping. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] doc: fix spelling errors
On 21.10.2016 22:12, Lou Logan wrote:
> On Thu, Oct 13, 2016, at 01:25 PM, Andreas Cadhalpun wrote:
> [...]
>> @item mgain
>> Set max gain that will be displayed. Only useful if curves option is
>> activated.
>> -Setting this to reasonable value allows to display gain which is derived
>> from
>> +Setting this to reasonable value allows one to display gain which is
>> derived from
>
> To *a* reasonable...
Sure.
> I've never been a fan of the "one" pronoun, but I'm fine with it being
> used in the docs and it's better than it not being there.
I've replaced 'allows one to' with 'makes it possible'.
Hopefully you like that better. ;)
Updated patch is attached.
Best regards,
Andreas
>From 794261f73f72bb1c85b6fde8217d01b34642abf1 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Thu, 13 Oct 2016 23:08:01 +0200
Subject: [PATCH] doc: fix spelling errors
Thanks to Mathieu Malaterre for reporting the
Que/Queue typo. (https://bugs.debian.org/839542)
Signed-off-by: Andreas Cadhalpun
---
doc/Doxyfile| 2 +-
doc/encoders.texi | 2 +-
doc/ffprobe.texi| 2 +-
doc/filters.texi| 10 +-
doc/muxers.texi | 6 +++---
ffmpeg.c| 2 +-
ffmpeg_cuvid.c | 4 ++--
libavcodec/aaccoder_twoloop.h | 2 +-
libavcodec/cabac.c | 2 +-
libavcodec/ffjni.c | 2 +-
libavcodec/mediacodec_wrapper.h | 2 +-
libavcodec/psymodel.h | 2 +-
libavcodec/x86/vp9lpf_16bpp.asm | 2 +-
libavfilter/af_hdcd.c | 2 +-
libavfilter/f_ebur128.c | 2 +-
libavformat/internal.h | 2 +-
libavutil/frame.h | 2 +-
libavutil/tree.h| 2 +-
18 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/doc/Doxyfile b/doc/Doxyfile
index fb5cdd3..0891899 100644
--- a/doc/Doxyfile
+++ b/doc/Doxyfile
@@ -1637,7 +1637,7 @@ EXTRA_PACKAGES =
# following commands have a special meaning inside the header: $title,
# $datetime, $date, $doxygenversion, $projectname, $projectnumber,
# $projectbrief, $projectlogo. Doxygen will replace $title with the empy string,
-# for the replacement values of the other commands the user is refered to
+# for the replacement values of the other commands the user is referred to
# HTML_HEADER.
# This tag requires that the tag GENERATE_LATEX is set to YES.
diff --git a/doc/encoders.texi b/doc/encoders.texi
index 09e1b9e..5a60e7e 100644
--- a/doc/encoders.texi
+++ b/doc/encoders.texi
@@ -1773,7 +1773,7 @@ Enable CAVLC and disable CABAC. It generates the same effect as
@end table
@item cmp
-Set full pixel motion estimation comparation algorithm. Possible values:
+Set full pixel motion estimation comparison algorithm. Possible values:
@table @samp
@item chroma
diff --git a/doc/ffprobe.texi b/doc/ffprobe.texi
index 2024eed..1069ae3 100644
--- a/doc/ffprobe.texi
+++ b/doc/ffprobe.texi
@@ -245,7 +245,7 @@ continue reading from that.
Each interval is specified by two optional parts, separated by "%".
The first part specifies the interval start position. It is
-interpreted as an abolute position, or as a relative offset from the
+interpreted as an absolute position, or as a relative offset from the
current position if it is preceded by the "+" character. If this first
part is not specified, no seeking will be performed when reading this
interval.
diff --git a/doc/filters.texi b/doc/filters.texi
index b470f40..c37fa29 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -449,7 +449,7 @@ This filter is bit crusher with enhanced functionality. A bit crusher
is used to audibly reduce number of bits an audio signal is sampled
with. This doesn't change the bit depth at all, it just produces the
effect. Material reduced in bit depth sounds more harsh and "digital".
-This filter is able to even round to continous values instead of discrete
+This filter is able to even round to continuous values instead of discrete
bit depths.
Additionally it has a D/C offset which results in different crushing of
the lower and the upper half of the signal.
@@ -475,7 +475,7 @@ Set level out.
Set bit reduction.
@item mix
-Set mixing ammount.
+Set mixing amount.
@item mode
Can be linear: @code{lin} or logarithmic: @code{log}.
@@ -1203,7 +1203,7 @@ Set video stream size. Only useful if curves option is activated.
@item mgain
Set max gain that will be displayed. Only useful if curves option is activated.
-Setting this to reasonable value allows to display gain which is derived from
+Setting this to a reasonable value makes it possible to display gain which is derived from
neighbour bands which are too close to each other and thus produce higher gain
when both are activated.
@@ -8858,7 +8858,7 @@ value.
@section hysteresis
Grow
Re: [FFmpeg-devel] [PATCH] doc: fix spelling errors
On 21.10.2016 23:47, Lou Logan wrote: > On Fri, Oct 21, 2016, at 01:21 PM, Andreas Cadhalpun wrote: >> >> Updated patch is attached. > > LGTM, thanks. Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: close parser if codec changed
On 22.10.2016 00:18, Michael Niedermayer wrote:
> On Mon, Oct 17, 2016 at 08:49:23PM +0200, Andreas Cadhalpun wrote:
>> The parser depends on the codec and thus must not be used with a different
>> one.
>> If it is, the 'avctx->codec_id == s->parser->codec_ids[0] ...' assert in
>> av_parser_parse2 gets triggered.
>>
>> Signed-off-by: Andreas Cadhalpun
>> ---
>> libavformat/utils.c | 12
>> 1 file changed, 12 insertions(+)
>
> This changes the audio output from
> http://samples.ffmpeg.org/MPEG2/vid_0x80.ts
>
> is that intended ?
Nice catch: it shouldn't change, of course.
While processing that sample only the codec_tag gets changed, which is no
reason to
close the parser. That is only necessary, when the codec_id changes.
Fixed patch is attached.
Best regards,
Andreas
>From 9de87a4fb2c6c6311a11a2da5de8554a71adfa66 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Mon, 17 Oct 2016 20:26:51 +0200
Subject: [PATCH] avformat: close parser if codec changed
The parser depends on the codec and thus must not be used with a different one.
If it is, the 'avctx->codec_id == s->parser->codec_ids[0] ...' assert in
av_parser_parse2 gets triggered.
Signed-off-by: Andreas Cadhalpun
---
libavformat/utils.c | 12
1 file changed, 12 insertions(+)
diff --git a/libavformat/utils.c b/libavformat/utils.c
index 70dbfa1..a8a78ed 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -480,6 +480,12 @@ static int update_stream_avctx(AVFormatContext *s)
if (!st->internal->need_context_update)
continue;
+/* close parser, because it depends on the codec */
+if (st->parser && st->internal->avctx->codec_id != st->codecpar->codec_id) {
+av_parser_close(st->parser);
+st->parser = NULL;
+}
+
/* update internal codec context, for the parser */
ret = avcodec_parameters_to_context(st->internal->avctx, st->codecpar);
if (ret < 0)
@@ -1515,6 +1521,12 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
st->info->found_decoder = 0;
}
+/* close parser, because it depends on the codec */
+if (st->parser && st->internal->avctx->codec_id != st->codecpar->codec_id) {
+av_parser_close(st->parser);
+st->parser = NULL;
+}
+
ret = avcodec_parameters_to_context(st->internal->avctx, st->codecpar);
if (ret < 0)
return ret;
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] mpegts: handle AVMEDIA_TYPE_UNKNOWN correctly
It is negative, so can't be used for left shifting.
This fixes ubsan runtime error: shift exponent -1 is negative
Signed-off-by: Andreas Cadhalpun
---
libavformat/mpegts.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
index 97a2225..cc2addc 100644
--- a/libavformat/mpegts.c
+++ b/libavformat/mpegts.c
@@ -2353,7 +2353,8 @@ static int handle_packet(MpegTSContext *ts, const uint8_t
*packet)
int types = 0;
for (i = 0; i < ts->stream->nb_streams; i++) {
AVStream *st = ts->stream->streams[i];
-types |= 1<codecpar->codec_type;
+if (st->codecpar->codec_type >= 0)
+types |= 1<codecpar->codec_type;
}
if ((types & (1< 10) {
av_log(ts->stream, AV_LOG_DEBUG, "All programs have pmt,
headers found\n");
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] mpegts: handle AVMEDIA_TYPE_UNKNOWN correctly
On 22.10.2016 11:41, Michael Niedermayer wrote: > On Sat, Oct 22, 2016 at 01:22:21AM +0200, Andreas Cadhalpun wrote: >> It is negative, so can't be used for left shifting. >> >> This fixes ubsan runtime error: shift exponent -1 is negative >> >> Signed-off-by: Andreas Cadhalpun >> --- >> libavformat/mpegts.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > should be ok Pushed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] faq: use relative links to own documentation
This way locally installed documentation refers to itself instead of the
website.
Bud-Id: https://bugs.debian.org/841501
Signed-off-by: Andreas Cadhalpun
---
doc/faq.texi | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/doc/faq.texi b/doc/faq.texi
index ef111c7..ff35c89 100644
--- a/doc/faq.texi
+++ b/doc/faq.texi
@@ -311,18 +311,18 @@ invoking ffmpeg with several @option{-i} options.
For audio, to put all channels together in a single stream (example: two
mono streams into one stereo stream): this is sometimes called to
@emph{merge} them, and can be done using the
-@url{https://ffmpeg.org/ffmpeg-filters.html#amerge, @code{amerge}} filter.
+@url{ffmpeg-filters.html#amerge, @code{amerge}} filter.
@item
For audio, to play one on top of the other: this is called to @emph{mix}
them, and can be done by first merging them into a single stream and then
-using the @url{https://ffmpeg.org/ffmpeg-filters.html#pan, @code{pan}} filter
to mix
+using the @url{ffmpeg-filters.html#pan, @code{pan}} filter to mix
the channels at will.
@item
For video, to display both together, side by side or one on top of a part of
the other; it can be done using the
-@url{https://ffmpeg.org/ffmpeg-filters.html#overlay, @code{overlay}} video
filter.
+@url{ffmpeg-filters.html#overlay, @code{overlay}} video filter.
@end itemize
@@ -333,19 +333,19 @@ There are several solutions, depending on the exact
circumstances.
@subsection Concatenating using the concat @emph{filter}
-FFmpeg has a @url{https://ffmpeg.org/ffmpeg-filters.html#concat,
+FFmpeg has a @url{ffmpeg-filters.html#concat,
@code{concat}} filter designed specifically for that, with examples in the
documentation. This operation is recommended if you need to re-encode.
@subsection Concatenating using the concat @emph{demuxer}
-FFmpeg has a @url{https://www.ffmpeg.org/ffmpeg-formats.html#concat,
+FFmpeg has a @url{ffmpeg-formats.html#concat,
@code{concat}} demuxer which you can use when you want to avoid a re-encode and
your format doesn't support file level concatenation.
@subsection Concatenating using the concat @emph{protocol} (file level)
-FFmpeg has a @url{https://ffmpeg.org/ffmpeg-protocols.html#concat,
+FFmpeg has a @url{ffmpeg-protocols.html#concat,
@code{concat}} protocol designed specifically for that, with examples in the
documentation.
@@ -485,7 +485,7 @@ scaling adjusts the SAR to keep the DAR constant.
If you want to stretch, or “unstretch”, the image, you need to override the
information with the
-@url{https://ffmpeg.org/ffmpeg-filters.html#setdar_002c-setsar, @code{setdar
or setsar filters}}.
+@url{ffmpeg-filters.html#setdar_002c-setsar, @code{setdar or setsar filters}}.
Do not forget to examine carefully the original video to check whether the
stretching comes from the image or from the aspect ratio information.
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] faq: use relative links to own documentation
On 23.10.2016 06:24, James Almer wrote:
> On 10/22/2016 3:17 PM, Andreas Cadhalpun wrote:
>> This way locally installed documentation refers to itself instead of the
>> website.
>>
>> Bud-Id: https://bugs.debian.org/841501
>> Signed-off-by: Andreas Cadhalpun
>> ---
>> doc/faq.texi | 14 +++---
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/doc/faq.texi b/doc/faq.texi
>> index ef111c7..ff35c89 100644
>> --- a/doc/faq.texi
>> +++ b/doc/faq.texi
>> @@ -311,18 +311,18 @@ invoking ffmpeg with several @option{-i} options.
>> For audio, to put all channels together in a single stream (example: two
>> mono streams into one stereo stream): this is sometimes called to
>> @emph{merge} them, and can be done using the
>> -@url{https://ffmpeg.org/ffmpeg-filters.html#amerge, @code{amerge}} filter.
>> +@url{ffmpeg-filters.html#amerge, @code{amerge}} filter.
>>
>> @item
>> For audio, to play one on top of the other: this is called to @emph{mix}
>> them, and can be done by first merging them into a single stream and then
>> -using the @url{https://ffmpeg.org/ffmpeg-filters.html#pan, @code{pan}}
>> filter to mix
>> +using the @url{ffmpeg-filters.html#pan, @code{pan}} filter to mix
>> the channels at will.
>>
>> @item
>> For video, to display both together, side by side or one on top of a part of
>> the other; it can be done using the
>> -@url{https://ffmpeg.org/ffmpeg-filters.html#overlay, @code{overlay}} video
>> filter.
>> +@url{ffmpeg-filters.html#overlay, @code{overlay}} video filter.
>>
>> @end itemize
>>
>> @@ -333,19 +333,19 @@ There are several solutions, depending on the exact
>> circumstances.
>>
>> @subsection Concatenating using the concat @emph{filter}
>>
>> -FFmpeg has a @url{https://ffmpeg.org/ffmpeg-filters.html#concat,
>> +FFmpeg has a @url{ffmpeg-filters.html#concat,
>> @code{concat}} filter designed specifically for that, with examples in the
>> documentation. This operation is recommended if you need to re-encode.
>>
>> @subsection Concatenating using the concat @emph{demuxer}
>>
>> -FFmpeg has a @url{https://www.ffmpeg.org/ffmpeg-formats.html#concat,
>> +FFmpeg has a @url{ffmpeg-formats.html#concat,
>> @code{concat}} demuxer which you can use when you want to avoid a re-encode
>> and
>> your format doesn't support file level concatenation.
>>
>> @subsection Concatenating using the concat @emph{protocol} (file level)
>>
>> -FFmpeg has a @url{https://ffmpeg.org/ffmpeg-protocols.html#concat,
>> +FFmpeg has a @url{ffmpeg-protocols.html#concat,
>> @code{concat}} protocol designed specifically for that, with examples in the
>> documentation.
>>
>> @@ -485,7 +485,7 @@ scaling adjusts the SAR to keep the DAR constant.
>>
>> If you want to stretch, or “unstretch”, the image, you need to override the
>> information with the
>> -@url{https://ffmpeg.org/ffmpeg-filters.html#setdar_002c-setsar,
>> @code{setdar or setsar filters}}.
>> +@url{ffmpeg-filters.html#setdar_002c-setsar, @code{setdar or setsar
>> filters}}.
>>
>> Do not forget to examine carefully the original video to check whether the
>> stretching comes from the image or from the aspect ratio information.
>
> Should be ok.
Pushed.
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop
Hi, On 23.10.2016 04:10, Ronald S. Bultje wrote: > This is hideous, you're sprinkling emms_c in various places to make some > stupid test pass. The test is morbidly stupid Can you elaborate why you think the test is bad? > and there is no general > consensus on patterns to be followed as for where to place emms_c. Someone > who doesn't know any better will litter each new decoder with 10-20 calls > to emms_c just because he found that other decoders do it in undocumented, > unexplained and unclear locations also. > > If you want this to be a "thing", you need to design and document carefully > where emms_c is necessary. Then come up with some system that makes this > work by itself. I've said from the beginning that I highly dislike > littering the code with emms_c in individual decoders, and that's exactly > what you're doing here. This is insane. I also don't like adding emms_c in various places, but I don't see a realistic alternative other than not fixing the problem. And I think that would be worse than this patch set. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/x86/emms: Document the emms_c() vs alloc/free relation.
On 23.10.2016 05:37, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer
> ---
> libavutil/x86/emms.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/libavutil/x86/emms.h b/libavutil/x86/emms.h
> index 6fda6e2..42c18e2 100644
> --- a/libavutil/x86/emms.h
> +++ b/libavutil/x86/emms.h
> @@ -31,6 +31,8 @@ void avpriv_emms_yasm(void);
> * Empty mmx state.
> * this must be called between any dsp function and float/double code.
> * for example sin(); dsp->idct_put(); emms_c(); cos()
> + * Note, *alloc() and *free() also use float code in some libc
> implementations
> + * thus this also applies to them or any function using them.
> */
> static av_always_inline void emms_c(void)
> {
>
Seems fine.
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 01/12] adxdec: validate sample_rate
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/adxdec.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/libavformat/adxdec.c b/libavformat/adxdec.c
index cf44531..0315ecb 100644
--- a/libavformat/adxdec.c
+++ b/libavformat/adxdec.c
@@ -109,6 +109,11 @@ static int adx_read_header(AVFormatContext *s)
return AVERROR_INVALIDDATA;
}
+if (par->sample_rate <= 0) {
+av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n", par->sample_rate);
+return AVERROR_INVALIDDATA;
+}
+
par->codec_type = AVMEDIA_TYPE_AUDIO;
par->codec_id= s->iformat->raw_codec_id;
par->bit_rate= par->sample_rate * par->channels * BLOCK_SIZE * 8LL /
BLOCK_SAMPLES;
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 02/12] bfi: validate sample_rate
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/bfi.c | 4
1 file changed, 4 insertions(+)
diff --git a/libavformat/bfi.c b/libavformat/bfi.c
index 568363d..ef4c17d 100644
--- a/libavformat/bfi.c
+++ b/libavformat/bfi.c
@@ -88,6 +88,10 @@ static int bfi_read_header(AVFormatContext * s)
vstream->codecpar->extradata_size);
astream->codecpar->sample_rate = avio_rl32(pb);
+if (astream->codecpar->sample_rate <= 0) {
+av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n",
astream->codecpar->sample_rate);
+return AVERROR_INVALIDDATA;
+}
/* Set up the video codec... */
avpriv_set_pts_info(vstream, 32, 1, fps);
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 03/12] ffmdec: validate sample_rate
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/ffmdec.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/libavformat/ffmdec.c b/libavformat/ffmdec.c
index 16ba8ec..6b09be2 100644
--- a/libavformat/ffmdec.c
+++ b/libavformat/ffmdec.c
@@ -432,6 +432,11 @@ static int ffm2_read_header(AVFormatContext *s)
goto fail;
}
codec->sample_rate = avio_rb32(pb);
+if (codec->sample_rate <= 0) {
+av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n",
codec->sample_rate);
+ret = AVERROR_INVALIDDATA;
+goto fail;
+}
codec->channels = avio_rl16(pb);
codec->frame_size = avio_rl16(pb);
break;
@@ -628,6 +633,10 @@ static int ffm_read_header(AVFormatContext *s)
break;
case AVMEDIA_TYPE_AUDIO:
codec->sample_rate = avio_rb32(pb);
+if (codec->sample_rate <= 0) {
+av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n",
codec->sample_rate);
+goto fail;
+}
codec->channels = avio_rl16(pb);
codec->frame_size = avio_rl16(pb);
break;
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 05/12] mov: validate time_scale
A negative timescale doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/mov.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 0a3fdd1..6f313a5 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1223,6 +1223,10 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
mov_metadata_creation_time(&st->metadata, creation_time);
sc->time_scale = avio_rb32(pb);
+if (sc->time_scale <= 0) {
+av_log(c->fc, AV_LOG_ERROR, "Invalid mdhd time scale %d\n",
sc->time_scale);
+return AVERROR_INVALIDDATA;
+}
st->duration = (version == 1) ? avio_rb64(pb) : avio_rb32(pb); /* duration
*/
lang = avio_rb16(pb); /* language */
@@ -1248,7 +1252,10 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
}
mov_metadata_creation_time(&c->fc->metadata, creation_time);
c->time_scale = avio_rb32(pb); /* time scale */
-
+if (c->time_scale <= 0) {
+av_log(c->fc, AV_LOG_ERROR, "Invalid mvhd time scale %d\n",
c->time_scale);
+return AVERROR_INVALIDDATA;
+}
av_log(c->fc, AV_LOG_TRACE, "time scale = %i\n", c->time_scale);
c->duration = (version == 1) ? avio_rb64(pb) : avio_rb32(pb); /* duration
*/
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 04/12] mov: validate sample_rate
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
fate-aac-al07_96 fails if sample_rate == 0 is rejected in
ff_mov_read_stsd_entries.
Signed-off-by: Andreas Cadhalpun
---
libavformat/mov.c | 8
1 file changed, 8 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index dada1e0..0a3fdd1 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -888,6 +888,10 @@ static int mov_read_ddts(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
st = c->fc->streams[c->fc->nb_streams-1];
st->codecpar->sample_rate = get_bits_long(&gb, 32);
+if (st->codecpar->sample_rate <= 0) {
+av_log(c->fc, AV_LOG_ERROR, "Invalid sample rate %d\n",
st->codecpar->sample_rate);
+return AVERROR_INVALIDDATA;
+}
skip_bits_long(&gb, 32); /* max bitrate */
st->codecpar->bit_rate = get_bits_long(&gb, 32);
st->codecpar->bits_per_coded_sample = get_bits(&gb, 8);
@@ -2273,6 +2277,10 @@ int ff_mov_read_stsd_entries(MOVContext *c, AVIOContext
*pb, int entries)
} else if (st->codecpar->codec_type==AVMEDIA_TYPE_AUDIO) {
st->codecpar->codec_id = id;
mov_parse_stsd_audio(c, pb, st, sc);
+if (st->codecpar->sample_rate < 0) {
+av_log(c->fc, AV_LOG_ERROR, "Invalid sample rate %d\n",
st->codecpar->sample_rate);
+return AVERROR_INVALIDDATA;
+}
} else if (st->codecpar->codec_type==AVMEDIA_TYPE_SUBTITLE){
st->codecpar->codec_id = id;
mov_parse_stsd_subtitle(c, pb, st, sc,
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 07/12] mpeg4audio: validate sample_rate
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
Also check for errors from avpriv_mpeg4audio_get_config in
ff_mp4_read_dec_config_descr.
Signed-off-by: Andreas Cadhalpun
---
libavcodec/mpeg4audio.c | 5 +
libavformat/isom.c | 6 --
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/libavcodec/mpeg4audio.c b/libavcodec/mpeg4audio.c
index 188d843..01c374f 100644
--- a/libavcodec/mpeg4audio.c
+++ b/libavcodec/mpeg4audio.c
@@ -42,6 +42,11 @@ static int parse_config_ALS(GetBitContext *gb,
MPEG4AudioConfig *c)
// which are buggy in old ALS conformance files
c->sample_rate = get_bits_long(gb, 32);
+if (c->sample_rate <= 0) {
+av_log(NULL, AV_LOG_ERROR, "Invalid sample rate %d\n", c->sample_rate);
+return AVERROR_INVALIDDATA;
+}
+
// skip number of samples
skip_bits_long(gb, 32);
diff --git a/libavformat/isom.c b/libavformat/isom.c
index cb457dd..88f8605 100644
--- a/libavformat/isom.c
+++ b/libavformat/isom.c
@@ -508,8 +508,10 @@ int ff_mp4_read_dec_config_descr(AVFormatContext *fc,
AVStream *st, AVIOContext
return ret;
if (st->codecpar->codec_id == AV_CODEC_ID_AAC) {
MPEG4AudioConfig cfg = {0};
-avpriv_mpeg4audio_get_config(&cfg, st->codecpar->extradata,
- st->codecpar->extradata_size * 8, 1);
+ret = avpriv_mpeg4audio_get_config(&cfg, st->codecpar->extradata,
+ st->codecpar->extradata_size *
8, 1);
+if (ret < 0)
+return ret;
st->codecpar->channels = cfg.channels;
if (cfg.object_type == 29 && cfg.sampling_index < 3) // old mp3on4
st->codecpar->sample_rate =
avpriv_mpa_freq_tab[cfg.sampling_index];
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 06/12] mov: validate sidx timescale
A negative timescale doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/mov.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 6f313a5..413675f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4233,6 +4233,11 @@ static int mov_read_sidx(MOVContext *c, AVIOContext *pb,
MOVAtom atom)
timescale = av_make_q(1, avio_rb32(pb));
+if (timescale.den <= 0) {
+av_log(c->fc, AV_LOG_ERROR, "Invalid sidx timescale 1/%d\n",
timescale.den);
+return AVERROR_INVALIDDATA;
+}
+
if (version == 0) {
pts = avio_rb32(pb);
offset += avio_rb32(pb);
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 08/12] mvdec: validate sample_rate
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/mvdec.c | 4
1 file changed, 4 insertions(+)
diff --git a/libavformat/mvdec.c b/libavformat/mvdec.c
index 80ef4b1..f0a29eb 100644
--- a/libavformat/mvdec.c
+++ b/libavformat/mvdec.c
@@ -317,6 +317,10 @@ static int mv_read_header(AVFormatContext *avctx)
ast->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
ast->nb_frames = vst->nb_frames;
ast->codecpar->sample_rate = avio_rb32(pb);
+if (ast->codecpar->sample_rate <= 0) {
+av_log(avctx, AV_LOG_ERROR, "Invalid sample rate %d\n",
ast->codecpar->sample_rate);
+return AVERROR_INVALIDDATA;
+}
avpriv_set_pts_info(ast, 33, 1, ast->codecpar->sample_rate);
if (set_channels(avctx, ast, avio_rb32(pb)) < 0)
return AVERROR_INVALIDDATA;
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 09/12] nuv: validate sample_rate
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/nuv.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/libavformat/nuv.c b/libavformat/nuv.c
index d920250..9bdea4a 100644
--- a/libavformat/nuv.c
+++ b/libavformat/nuv.c
@@ -66,7 +66,7 @@ static int nuv_probe(AVProbeData *p)
* @param myth set if this is a MythTVVideo format file
* @return 0 or AVERROR code
*/
-static int get_codec_data(AVIOContext *pb, AVStream *vst,
+static int get_codec_data(AVFormatContext *s, AVIOContext *pb, AVStream *vst,
AVStream *ast, int myth)
{
nuv_frametype frametype;
@@ -114,6 +114,10 @@ static int get_codec_data(AVIOContext *pb, AVStream *vst,
ast->codecpar->codec_tag = avio_rl32(pb);
ast->codecpar->sample_rate = avio_rl32(pb);
+if (ast->codecpar->sample_rate <= 0) {
+av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n",
ast->codecpar->sample_rate);
+return AVERROR_INVALIDDATA;
+}
ast->codecpar->bits_per_coded_sample = avio_rl32(pb);
ast->codecpar->channels = avio_rl32(pb);
ast->codecpar->channel_layout= 0;
@@ -232,7 +236,7 @@ static int nuv_header(AVFormatContext *s)
} else
ctx->a_id = -1;
-if ((ret = get_codec_data(pb, vst, ast, is_mythtv)) < 0)
+if ((ret = get_codec_data(s, pb, vst, ast, is_mythtv)) < 0)
return ret;
ctx->rtjpg_video = vst && vst->codecpar->codec_id == AV_CODEC_ID_NUV;
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 10/12] oggparsespeex: validate sample_rate
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/oggparsespeex.c | 4
1 file changed, 4 insertions(+)
diff --git a/libavformat/oggparsespeex.c b/libavformat/oggparsespeex.c
index 434b0fd..2b49150 100644
--- a/libavformat/oggparsespeex.c
+++ b/libavformat/oggparsespeex.c
@@ -68,6 +68,10 @@ static int speex_header(AVFormatContext *s, int idx) {
}
st->codecpar->sample_rate = AV_RL32(p + 36);
+if (st->codecpar->sample_rate <= 0) {
+av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n",
st->codecpar->sample_rate);
+return AVERROR_INVALIDDATA;
+}
st->codecpar->channels = AV_RL32(p + 48);
if (st->codecpar->channels < 1 || st->codecpar->channels > 2) {
av_log(s, AV_LOG_ERROR, "invalid channel count. Speex must be mono
or stereo.\n");
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 11/12] voc_packet: validate sample_rate
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/voc_packet.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/libavformat/voc_packet.c b/libavformat/voc_packet.c
index 5833a79..0d56436 100644
--- a/libavformat/voc_packet.c
+++ b/libavformat/voc_packet.c
@@ -106,6 +106,11 @@ ff_voc_get_packet(AVFormatContext *s, AVPacket *pkt,
AVStream *st, int max_size)
}
}
+if (par->sample_rate <= 0) {
+av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n", par->sample_rate);
+return AVERROR_INVALIDDATA;
+}
+
if (tmp_codec >= 0) {
tmp_codec = ff_codec_get_id(ff_voc_codec_tags, tmp_codec);
if (par->codec_id == AV_CODEC_ID_NONE)
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 12/12] xmv: validate sample_rate
AVCodecParameters.sample_rate is a signed integer, so
XMVAudioPacket.sample_rate should be, too.
A negative sample rate doesn't make sense and triggers assertions in
av_rescale_rnd.
Signed-off-by: Andreas Cadhalpun
---
libavformat/xmv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavformat/xmv.c b/libavformat/xmv.c
index b85d0cc..b974e5a 100644
--- a/libavformat/xmv.c
+++ b/libavformat/xmv.c
@@ -77,7 +77,7 @@ typedef struct XMVAudioPacket {
/* Stream format properties. */
uint16_t compression; ///< The type of compression.
uint16_t channels;///< Number of channels.
-uint32_t sample_rate; ///< Sampling rate.
+int32_t sample_rate; ///< Sampling rate.
uint16_t bits_per_sample; ///< Bits per compressed sample.
uint32_t bit_rate;///< Bits of compressed data per second.
uint16_t flags; ///< Flags
@@ -210,7 +210,7 @@ static int xmv_read_header(AVFormatContext *s)
av_log(s, AV_LOG_WARNING, "Unsupported 5.1 ADPCM audio stream "
"(0x%04X)\n", packet->flags);
-if (!packet->channels || !packet->sample_rate ||
+if (!packet->channels || packet->sample_rate <= 0 ||
packet->channels >= UINT16_MAX / XMV_BLOCK_ALIGN_SIZE) {
av_log(s, AV_LOG_ERROR, "Invalid parameters for audio track
%"PRIu16".\n",
audio_track);
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 02/12] bfi: validate sample_rate
On 24.10.2016 09:55, wm4 wrote:
> On Sun, 23 Oct 2016 18:27:02 +0200
> Andreas Cadhalpun wrote:
>
>> A negative sample rate doesn't make sense and triggers assertions in
>> av_rescale_rnd.
>>
>> Signed-off-by: Andreas Cadhalpun
>> ---
>> libavformat/bfi.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/libavformat/bfi.c b/libavformat/bfi.c
>> index 568363d..ef4c17d 100644
>> --- a/libavformat/bfi.c
>> +++ b/libavformat/bfi.c
>> @@ -88,6 +88,10 @@ static int bfi_read_header(AVFormatContext * s)
>> vstream->codecpar->extradata_size);
>>
>> astream->codecpar->sample_rate = avio_rl32(pb);
>> +if (astream->codecpar->sample_rate <= 0) {
>> +av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n",
>> astream->codecpar->sample_rate);
>> +return AVERROR_INVALIDDATA;
>> +}
>>
>> /* Set up the video codec... */
>> avpriv_set_pts_info(vstream, 32, 1, fps);
>
> Would it make sense to validate codecpars in the generic code (utils.c)?
I think that's a good idea. Still, checking the value where it is actually
set is good in any case.
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop
On 23.10.2016 20:18, Carl Eugen Hoyos wrote: > 2016-10-23 12:14 GMT+02:00 Andreas Cadhalpun > : > >> I also don't like adding emms_c in various places, but I don't see a >> realistic alternative > > (+1!) > >> other than not fixing the problem. And I think that would be worse >> than this patch set. > > I don't (strongly) disagree but could you elaborate on why this would > be such a bad alternative? For one thing FFmpeg shouldn't rely on undefined behavior (a theoretical issue) and for another thing it should work with musl libc (a practical problem). I think not fixing the latter is bad for our users. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop
On 24.10.2016 16:14, Ronald S. Bultje wrote: > On Mon, Oct 24, 2016 at 8:47 AM, wm4 wrote: >> On Mon, 24 Oct 2016 07:54:47 -0400 >> "Ronald S. Bultje" wrote: >>> On Mon, Oct 24, 2016 at 3:36 AM, wm4 wrote: I was under the impression that it is UB to have the FPU in MMX state at any time while in C, not just while e.g. calling the stdlib. Maybe I got that wrong (how would MMX intrinsics even work?) - can anyone shed light on the exact requirements? (Possibly again, sorry.) >>> >>> I'm under the impression that it's part of the calling convention. That >> is, >>> any code anywhere (including mmx intrinsics, indeed) can - when called - >>> expect the state to be cleared by the caller, just like you'd expect >>> eax/edx to be caller-save (whereas esi/edi are callee-save). >>> >>> However, if you never call external code (including intrinsics), you can >>> ignore this, just as you can ignore / create your own calling >>> convention (remember fastcall etc.?). However, when calling any external >>> code, this could (in theory) crash; it's just that right now it only >>> crashes with musl when calling malloc/free. So basically, ffmpeg has its >>> own calling convention, and manually calling emms_c() fixes "ffmpeg" >>> calling convention to be compatible with "standard" calling >> convention...? >> >> It can't really be a calling convention unless the compiler is aware of >> it? It is defined as part of the System V Application Binary Interface [1]: "The CPU shall be in x87 mode upon entry to a function. Therefore, every function that uses the MMX registers is required to issue an emms or femms instruction after using MMX registers, before returning or calling another function." >> We're doing things behind the compiler's back here. The safest >> assumption would be that leaving the FPU state invalid while in C is >> not allowed, period. That's what the standard says. >> The next safest assumption is that it's fine as long as we explicitly >> don't use floating point or call external functions that aren't >> MMX-aware. This would mean calling any libc functions or user callbacks >> (including indirectly through e.g. av_log) is not fine. This is probably OK in practice and likely has a significant performance benefit. Best regards, Andreas 1: https://software.intel.com/sites/default/files/article/402129/mpx-linux64-abi.pdf ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 09/13] avcodec/svq1dec: clear MMX state after MB decode loop
On 24.10.2016 21:34, wm4 wrote: > On Mon, 24 Oct 2016 21:19:46 +0200 > Andreas Cadhalpun wrote: >> On 24.10.2016 16:14, Ronald S. Bultje wrote: >>> On Mon, Oct 24, 2016 at 8:47 AM, wm4 wrote: >>>> The next safest assumption is that it's fine as long as we explicitly >>>> don't use floating point or call external functions that aren't >>>> MMX-aware. This would mean calling any libc functions or user callbacks >>>> (including indirectly through e.g. av_log) is not fine. >> >> This is probably OK in practice and likely has a significant performance >> benefit. > > Seems like it. > > The compiler still could generate code using the FPU state at any > point, though, unless maybe there is an inline asm block. No function > can put the FPU into MMX mode, because any MMX using function must have > called emms before returning. Consequently, only compiler-known > intrinsics or opaque inline asm block could clobber the FPU state. The > latter because the compiler doesn't inspect asm blocks. I agree. However, I think that this is currently only a theoretical issue. At least I'm not aware of this causing problems with a real compiler. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec: validate codec parameters in avcodec_parameters_to_context
This should reduce the impact of a demuxer (or API user) setting bogus
codec parameters.
Suggested-by: wm4
Signed-off-by: Andreas Cadhalpun
---
libavcodec/utils.c | 82 +-
1 file changed, 81 insertions(+), 1 deletion(-)
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 87de15f..9977ffd 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -4227,8 +4227,20 @@ int avcodec_parameters_to_context(AVCodecContext *codec,
codec->codec_id = par->codec_id;
codec->codec_tag = par->codec_tag;
+if (par->bit_rate < 0) {
+av_log(codec, AV_LOG_ERROR, "Invalid bit rate %"PRId64"\n",
par->bit_rate);
+return AVERROR(EINVAL);
+}
codec->bit_rate = par->bit_rate;
+if (par->bits_per_coded_sample < 0) {
+av_log(codec, AV_LOG_ERROR, "Invalid bits per coded sample %d\n",
par->bits_per_coded_sample);
+return AVERROR(EINVAL);
+}
codec->bits_per_coded_sample = par->bits_per_coded_sample;
+if (par->bits_per_raw_sample < 0) {
+av_log(codec, AV_LOG_ERROR, "Invalid bits per raw sample %d\n",
par->bits_per_raw_sample);
+return AVERROR(EINVAL);
+}
codec->bits_per_raw_sample = par->bits_per_raw_sample;
codec->profile = par->profile;
codec->level = par->level;
@@ -4236,42 +4248,110 @@ int avcodec_parameters_to_context(AVCodecContext
*codec,
switch (par->codec_type) {
case AVMEDIA_TYPE_VIDEO:
codec->pix_fmt= par->format;
+if ( (par->width || par->height) && av_image_check_size(par->width,
par->height, 0, codec) < 0)
+return AVERROR(EINVAL);
codec->width = par->width;
codec->height = par->height;
codec->field_order= par->field_order;
+if (par->color_range < 0 || par->color_range > AVCOL_RANGE_NB) {
+av_log(codec, AV_LOG_ERROR, "Invalid color range %d\n",
par->color_range);
+return AVERROR(EINVAL);
+}
codec->color_range= par->color_range;
+if (par->color_primaries < 0 || par->color_primaries > AVCOL_PRI_NB) {
+av_log(codec, AV_LOG_ERROR, "Invalid color primaries %d\n",
par->color_primaries);
+return AVERROR(EINVAL);
+}
codec->color_primaries= par->color_primaries;
+if (par->color_trc < 0 || par->color_trc > AVCOL_TRC_NB) {
+av_log(codec, AV_LOG_ERROR, "Invalid color transfer
characteristics %d\n", par->color_trc);
+return AVERROR(EINVAL);
+}
codec->color_trc = par->color_trc;
+if (par->color_space < 0 || par->color_space > AVCOL_SPC_NB) {
+av_log(codec, AV_LOG_ERROR, "Invalid color space %d\n",
par->color_space);
+return AVERROR(EINVAL);
+}
codec->colorspace = par->color_space;
+if (par->chroma_location < 0 || par->chroma_location >
AVCHROMA_LOC_NB) {
+av_log(codec, AV_LOG_ERROR, "Invalid chroma location %d\n",
par->chroma_location);
+return AVERROR(EINVAL);
+}
codec->chroma_sample_location = par->chroma_location;
+if (par->sample_aspect_ratio.num < 0 || par->sample_aspect_ratio.den <
0) {
+av_log(codec, AV_LOG_ERROR, "Invalid sample aspect ratio %d/%d\n",
+ par->sample_aspect_ratio.num, par->sample_aspect_ratio.den);
+return AVERROR(EINVAL);
+}
codec->sample_aspect_ratio= par->sample_aspect_ratio;
+if (par->video_delay < 0) {
+av_log(codec, AV_LOG_ERROR, "Invalid video delay %d\n",
par->video_delay);
+return AVERROR(EINVAL);
+}
codec->has_b_frames = par->video_delay;
break;
case AVMEDIA_TYPE_AUDIO:
+if (par->format < -1 || par->format > AV_SAMPLE_FMT_NB) {
+av_log(codec, AV_LOG_ERROR, "Invalid sample format %d\n",
par->format);
+return AVERROR(EINVAL);
+}
codec->sample_fmt = par->format;
codec->channel_layout = par->channel_layout;
+if (par->channels < 0) {
+av_log(codec, AV_LOG_ERROR, "Invalid number of channels %d\n",
par->channels);
+return AVERROR(EINVAL);
+}
codec->channels = par->channels;
+if (par->sample_rate < 0) {
+av_log(codec, AV_LOG_ERROR, "Invalid sample rate
[FFmpeg-devel] [PATCH] configure: make sure LTO does not optimize out the test functions
Bud-Id: https://bugs.gentoo.org/show_bug.cgi?id=598054
Signed-off-by: Andreas Cadhalpun
---
configure | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index 481f692..14a20ed 100755
--- a/configure
+++ b/configure
@@ -1147,8 +1147,14 @@ check_func_headers(){
for hdr in $headers; do
print_include $hdr
done
+# LTO could optimize out the test functions without this
+echo "#if defined(__GNUC__) && __GNUC__ >= 4"
+echo " #define USED __attribute__((used))"
+echo "#else"
+echo " #define USED"
+echo "#endif"
for func in $funcs; do
-echo "long check_$func(void) { return (long) $func; }"
+echo "USED long check_$func(void) { return (long) $func; }"
done
echo "int main(void) { return 0; }"
} | check_ld "cc" "$@" && enable $funcs && enable_safe $headers
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/12] adxdec: validate sample_rate
On 25.10.2016 12:58, Paul B Mahol wrote: > patch(es)have good intent, but better fix is doing/checking it in single > place. I don't agree. In general, validity checks should be where the values are actually read. This eliminates the risk that bogus values could cause problems between being set and being checked. Also, having only a check in a central place is bad for debugging, because it is not immediately clear where the bogus value came from, when the check is triggered. (I know this from personal experience debugging all the cases triggering the assert in av_rescale_rnd.) The problem with that approach is that such checks can easily be forgotten, which is why I think a check in a central place would make sense in addition to checking the individual cases. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] configure: make sure LTO does not optimize out the test functions
On 25.10.2016 23:34, Carl Eugen Hoyos wrote:
> 2016-10-25 19:19 GMT+02:00 Andreas Cadhalpun
> :
>
>> +# LTO could optimize out the test functions without this
>> +echo "#if defined(__GNUC__) && __GNUC__ >= 4"
>> +echo " #define USED __attribute__((used))"
>> +echo "#else"
>> +echo " #define USED"
>> +echo "#endif"
>
> Why is the ugly #if - #else - #endif necessary?
I'm under the impression that __attribute__((used)) is not available for all
compilers,
see e.g. libavutil/attributes.h.
But thinking more about it, a better way is to actually use the functions so
that
LTO can't optimize them out. This avoids the ugly preprocessor checks.
> Please mention ticket #5909 if it is related.
Sure, new patch attached.
By the way, this is not a regression in 3.1.5, but a fix included in that
release allowed them to drop their distro-specific patch, exposing the problem
with LTO.
Best regards,
Andreas
>From bb289a0b2b0948afa99227bcff188301c1143624 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Tue, 25 Oct 2016 19:09:46 +0200
Subject: [PATCH] configure: make sure LTO does not optimize out the test
functions
Fixes trac ticket #5909
Bud-Id: https://bugs.gentoo.org/show_bug.cgi?id=598054
Signed-off-by: Andreas Cadhalpun
---
configure | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index 481f692..54faef1 100755
--- a/configure
+++ b/configure
@@ -1150,7 +1150,12 @@ check_func_headers(){
for func in $funcs; do
echo "long check_$func(void) { return (long) $func; }"
done
-echo "int main(void) { return 0; }"
+echo "int main(void) { int ret = 0;"
+# LTO could optimize out the test functions without this
+for func in $funcs; do
+echo " ret |= ((intptr_t)check_$func) & 0x;"
+done
+echo "return ret; }"
} | check_ld "cc" "$@" && enable $funcs && enable_safe $headers
}
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] configure: make sure LTO does not optimize out the test functions
On 26.10.2016 01:26, Michael Niedermayer wrote:
> On Wed, Oct 26, 2016 at 01:16:13AM +0200, Andreas Cadhalpun wrote:
>> configure |7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>> 742684cf379693d08075d43fdfb75ed5e2e936c6
>> 0001-configure-make-sure-LTO-does-not-optimize-out-the-te.patch
>> From bb289a0b2b0948afa99227bcff188301c1143624 Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun
>> Date: Tue, 25 Oct 2016 19:09:46 +0200
>> Subject: [PATCH] configure: make sure LTO does not optimize out the test
>> functions
>>
>> Fixes trac ticket #5909
>>
>> Bud-Id: https://bugs.gentoo.org/show_bug.cgi?id=598054
>> Signed-off-by: Andreas Cadhalpun
>> ---
>> configure | 7 ++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index 481f692..54faef1 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1150,7 +1150,12 @@ check_func_headers(){
>> for func in $funcs; do
>> echo "long check_$func(void) { return (long) $func; }"
>> done
>> -echo "int main(void) { return 0; }"
>> +echo "int main(void) { int ret = 0;"
>> +# LTO could optimize out the test functions without this
>> +for func in $funcs; do
>> +echo " ret |= ((intptr_t)check_$func) & 0x;"
>> +done
>> +echo "return ret; }"
>
> breaks configure
>
> i get this:
>
> ERROR: LoadLibrary/dlopen not found for avisynth
I forgot to include stdint.h. Fixed patch attached.
Best regards,
Andreas
>From 3f062e381b69c8e9b59f6caa700c23deaec53b45 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Tue, 25 Oct 2016 19:09:46 +0200
Subject: [PATCH] configure: make sure LTO does not optimize out the test
functions
Fixes trac ticket #5909
Bud-Id: https://bugs.gentoo.org/show_bug.cgi?id=598054
Signed-off-by: Andreas Cadhalpun
---
configure | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/configure b/configure
index 481f692..5ee78eb 100755
--- a/configure
+++ b/configure
@@ -1147,10 +1147,16 @@ check_func_headers(){
for hdr in $headers; do
print_include $hdr
done
+echo "#include "
for func in $funcs; do
echo "long check_$func(void) { return (long) $func; }"
done
-echo "int main(void) { return 0; }"
+echo "int main(void) { int ret = 0;"
+# LTO could optimize out the test functions without this
+for func in $funcs; do
+echo " ret |= ((intptr_t)check_$func) & 0x;"
+done
+echo "return ret; }"
} | check_ld "cc" "$@" && enable $funcs && enable_safe $headers
}
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec: validate codec parameters in avcodec_parameters_to_context
On 25.10.2016 13:43, Michael Niedermayer wrote: > On Tue, Oct 25, 2016 at 01:50:47AM +0200, Andreas Cadhalpun wrote: >> This should reduce the impact of a demuxer (or API user) setting bogus >> codec parameters. >> >> Suggested-by: wm4 >> Signed-off-by: Andreas Cadhalpun >> --- >> libavcodec/utils.c | 82 >> +- >> 1 file changed, 81 insertions(+), 1 deletion(-) > > This adds some warnings: > > libavcodec/utils.c: In function ‘avcodec_parameters_to_context’: > libavcodec/utils.c:4256:9: warning: comparison of unsigned expression < 0 is > always false [-Wtype-limits] > libavcodec/utils.c:4261:9: warning: comparison of unsigned expression < 0 is > always false [-Wtype-limits] > libavcodec/utils.c:4266:9: warning: comparison of unsigned expression < 0 is > always false [-Wtype-limits] > libavcodec/utils.c:4271:9: warning: comparison of unsigned expression < 0 is > always false [-Wtype-limits] > libavcodec/utils.c:4276:9: warning: comparison of unsigned expression < 0 is > always false [-Wtype-limits] > > i guess thats enum (un)signednes ... I'll just cast the enums to unsigned and check for > *_NB. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec: validate codec parameters in avcodec_parameters_to_context
On 25.10.2016 14:56, Hendrik Leppkes wrote: > On Tue, Oct 25, 2016 at 2:39 PM, wm4 wrote: >> On Tue, 25 Oct 2016 09:47:29 +0200 >> Hendrik Leppkes wrote: >> >>> On Tue, Oct 25, 2016 at 1:50 AM, Andreas Cadhalpun >>> wrote: >>>> This should reduce the impact of a demuxer (or API user) setting bogus >>>> codec parameters. >>>> >>>> >>> >>> This seems rather noisy I've added a macro to make it less noisy. >>> and doesn't really solve anything, does it? As you analyze below, it was fixing only a part of the problem. >>> Decoders still need to validate values instead of blindly trusting >>> them, and this just hides some problems in these decoders, Yes, the problem hiding is bad, which is why I added av_assert2's so that developers can easily check if the validation fails. >>> instead of >>> fixing them there. API users of avcodec would not fill >>> AVCodecParameters, they would fill a codec context directly. >> >> You could also argue that the demuxer shouldn't return invalid >> parameters either. > > It should not, but this patch does not address this. Indeed, the demuxers should be fixed in addition to this patch. > There is various combinations of component usage that are possible, > and in fact are used in the real world: > > avformat -> avcodec > other demuxer -> avcodec > avformat -> other decoder > > This patch only addresses the first case, and only if you actually use > this function (which I for example do not, since I have an abstraction > layer in between, so I never have AVCodecParameters and AVCodecContext > in the same function). > So in short, it just doesn't fix much, and you can still get invalid > output from avformat, and potentially still undefined behavior in > avcodec if its fed those values through other means. For the third case, the demuxers have to be fixed. Having the asserts in the central code makes it much easier to find these problems. >> How about this: always convert the params to a temporary codecpar, and >> provide a function to determine the validity of a codecpar. This way >> the check could be done in multiple places without duplicating the code >> needed for it. > > That still sounds odd, although slightly better. At the very least it > should be a dedicated function that checks the values in a key place, > say you want to check params that are fed to a decoder, then call this > check in avcodec_open, because thats something everyone has to call to > use avcodec. I tried to implement the suggestions of both of you, see attached patch. Note that the added asserts are triggered by *many* of my fuzzed samples. I'm happy to write patches for these cases, if we achieve agreement that the central check alone is insufficient. Another noteworthy point is that this patch makes it easy to trigger the av_assert0 in iff_read_packet. I think that assert should be replaced with an error return. Best regards, Andreas >From f371be7a027da3958e221b4dc88ad558c1489107 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun Date: Tue, 25 Oct 2016 01:45:27 +0200 Subject: [PATCH] avcodec: validate codec parameters This should reduce the impact of a broken demuxer (or API user) setting bogus codec parameters. The av_assert2 calls should ease detecting broken demuxers. Suggested-by: wm4 Signed-off-by: Andreas Cadhalpun --- libavcodec/utils.c | 82 -- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 87de15f..9773b0b 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1243,6 +1243,7 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code int ret = 0; AVDictionary *tmp = NULL; const AVPixFmtDescriptor *pixdesc; +AVCodecParameters *par = NULL; if (avcodec_is_open(avctx)) return 0; @@ -1259,8 +1260,14 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code if (!codec) codec = avctx->codec; -if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE) -return AVERROR(EINVAL); +par = avcodec_parameters_alloc(); +if (!par) +return AVERROR(ENOMEM); +/* check if the codec parameters in the context are valid */ +ret = avcodec_parameters_from_context(par, avctx); +avcodec_parameters_free(&par); +if (ret < 0) +return ret; if (options) av_dict_copy(&tmp, *options, 0); @@ -4124,6 +4131,66 @@ static void codec_parameters_reset(AVCodecParameters *par) par->level = FF_LEVEL_UNKNOWN; } +#define CHECK_PARAMETER
Re: [FFmpeg-devel] [PATCH] configure: make sure LTO does not optimize out the test functions
On 26.10.2016 10:52, Carl Eugen Hoyos wrote: > 2016-10-26 1:16 GMT+02:00 Andreas Cadhalpun > : >> I'm under the impression that __attribute__((used)) is not available >> for all compilers, > Yes, but __attribute__((foo_bar)) does not break compilation here. Have you tested with MSVC? > (But it is still a regression: 3.1 should not have seen the patch, > it doesn't support new incompatible external libraries, and the > issue was neither a regression nor security-related.) This was not about a new external library, the support was added in 2015 (commit 99eabcd), and I think fixing build failures is quite sensible for release branches. On 26.10.2016 10:55, Carl Eugen Hoyos wrote: > 2016-10-26 1:35 GMT+02:00 Andreas Cadhalpun > : > >> I forgot to include stdint.h. Fixed patch attached. > > Why don't you cast to (int)? Because gcc doesn't like it: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 01/12] adxdec: validate sample_rate
On 26.10.2016 20:15, Paul B Mahol wrote: > On 10/25/16, Michael Niedermayer wrote: >> On Tue, Oct 25, 2016 at 07:45:25PM +0200, Andreas Cadhalpun wrote: >>> On 25.10.2016 12:58, Paul B Mahol wrote: >>>> patch(es)have good intent, but better fix is doing/checking it in single >>>> place. >>> >>> I don't agree. >>> In general, validity checks should be where the values are actually read. >>> This eliminates the risk that bogus values could cause problems between >>> being set >>> and being checked. >>> Also, having only a check in a central place is bad for debugging, because >>> it is >>> not immediately clear where the bogus value came from, when the check is >>> triggered. >>> (I know this from personal experience debugging all the cases triggering >>> the >>> assert in av_rescale_rnd.) >>> >>> The problem with that approach is that such checks can easily be >>> forgotten, which >>> is why I think a check in a central place would make sense in addition to >>> checking >>> the individual cases. >> >> some formats may also lack a sample rate like mpeg ps/ts >> the fact is known insude the demuxer, generic code after it doesnt >> know. Doing the only check after the parser is a bit late OTOH >> >> [...] >> >> -- >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >> >> What does censorship reveal? It reveals fear. -- Julian Assange >> > > I'm not (yet) aware of better "fix" so do as you like. Have you seen the patch for checking this in a central place [1]? It would catch all the negative sample rates, but not the negative timescales. (I still think it should be checked both centrally and locally.) Best regards, Andreas 1: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-October/201769.html ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat: close parser if codec changed
On 22.10.2016 01:16, Andreas Cadhalpun wrote: > From 9de87a4fb2c6c6311a11a2da5de8554a71adfa66 Mon Sep 17 00:00:00 2001 > From: Andreas Cadhalpun > Date: Mon, 17 Oct 2016 20:26:51 +0200 > Subject: [PATCH] avformat: close parser if codec changed > > The parser depends on the codec and thus must not be used with a different > one. > If it is, the 'avctx->codec_id == s->parser->codec_ids[0] ...' assert in > av_parser_parse2 gets triggered. > > Signed-off-by: Andreas Cadhalpun > --- > libavformat/utils.c | 12 > 1 file changed, 12 insertions(+) Ping. (I'd like to have this in 3.2.) Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] configure: make sure LTO does not optimize out the test functions
On 26.10.2016 22:57, Carl Eugen Hoyos wrote: > 2016-10-26 21:36 GMT+02:00 Andreas Cadhalpun > : >> On 26.10.2016 10:52, Carl Eugen Hoyos wrote: >>> 2016-10-26 1:16 GMT+02:00 Andreas Cadhalpun >>> : >>>> I'm under the impression that __attribute__((used)) is not available >>>> for all compilers, >> >>> Yes, but __attribute__((foo_bar)) does not break compilation here. >> >> Have you tested with MSVC? > > No. Then it's safer to assume it doesn't work. >> On 26.10.2016 10:55, Carl Eugen Hoyos wrote: >>> 2016-10-26 1:35 GMT+02:00 Andreas Cadhalpun >>> : >>> >>>> I forgot to include stdint.h. Fixed patch attached. >>> >>> Why don't you cast to (int)? >> >> Because gcc doesn't like it: >> warning: cast from pointer to integer of different size >> [-Wpointer-to-int-cast] > > I don't see the problem with this warning but I guess you could use > long, no? As Hendrik said, intptr_t is the correct type here, so I see no reason not to use. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Remove the ffserver program and the ffm muxer/demuxer
On 27.10.2016 00:16, Rostislav Pehlivanov wrote:
> On 26 October 2016 at 22:48, James Almer wrote:
>
>> On 10/26/2016 6:19 PM, Rostislav Pehlivanov wrote:
>>> Also removes url_feof from libavformat.v which should have been
>>> removed long ago and changed the multipart jpeg boundary tag to
>>> ffmpeg rather than ffserver (it's arbitrary).
>>
>> [...]
>>
>>> diff --git a/libavformat/libavformat.v b/libavformat/libavformat.v
>>> index c961cd8..47d5ddc 100644
>>> --- a/libavformat/libavformat.v
>>> +++ b/libavformat/libavformat.v
>>> @@ -1,19 +1,6 @@
>>> LIBAVFORMAT_MAJOR {
>>> global:
>>> av*;
>>> -#FIXME those are for ffserver
>>> -ff_inet_aton;
>>> -ff_socket_nonblock;
>>> -ff_rtsp_parse_line;
>>> -ff_rtp_get_local_rtp_port;
>>> -ff_rtp_get_local_rtcp_port;
>>> -ffio_open_dyn_packet_buf;
>>> -ffio_set_buf_size;
>>> -ffurl_close;
>>> -ffurl_open;
>>> -ffurl_write;
>>> -#those are deprecated, remove on next bump
>>> -url_feof;
>>> local:
>>> *;
>>> };
>>
>> No, this can't be done until the next major bump. url_feof is even guarded
>> by an scheduled FF_API define.
>>
>> The rest should be ok, but anything that implies a library ABI break can't
>> be done just yet. Wait for other comments and/or testing. This removes a
>> lot of things after all.
>>
> Fixed version attached without removing url_feof (apparently that's meant
> to go in the next bump, wasn't forgotten)
No, none of the symbols can be removed without a major bump. The simple reason
is that a ffserver built from the 3.1 branch still has to work with the
libavformat from the 3.2 branch.
Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] FFmpeg 3.2
Hi, On 26.10.2016 21:29, Michael Niedermayer wrote: > On Tue, Sep 27, 2016 at 03:30:03PM +0200, Michael Niedermayer wrote: >> Its long since FFmpeg 3.1, so its time to make 3.2 >> ill branch release/3.2 off master and make 3.2 in maybe about a week or >> 2 unless something delays it > > release/3.2 branched > > ill make the 3.2 release tomorrow from release/3.2 > so if anyone wants to backport something, you have till then The following regressions from 3.1 should be fixed before releasing 3.2: (They were detected by the Debian autopkgtest.) * apng creation is broken: $ ./ffmpeg -f lavfi -i testsrc=s=32x32:d=0.1 -c:v apng -f apng apng.apng -y &> /dev/null $ ./ffprobe ./apng.apng ffprobe version n3.2-dev-1377-gebf8ec5 Copyright (c) 2007-2016 the FFmpeg developers built with gcc 6.2.0 (Debian 6.2.0-6) 20161010 configuration: --disable-optimizations libavutil 55. 34.100 / 55. 34.100 libavcodec 57. 64.100 / 57. 64.100 libavformat57. 56.100 / 57. 56.100 libavdevice57. 1.100 / 57. 1.100 libavfilter 6. 65.100 / 6. 65.100 libswscale 4. 2.100 / 4. 2.100 libswresample 2. 3.100 / 2. 3.100 [png @ 0x29b02c0] IDAT without IHDR [png_pipe @ 0x29ae7e0] decoding for stream 0 failed [png_pipe @ 0x29ae7e0] Could not find codec parameters for stream 0 (Video: png, none(pc)): unspecified size Consider increasing the value for the 'analyzeduration' and 'probesize' options Input #0, png_pipe, from './apng.apng': Duration: N/A, bitrate: N/A Stream #0:0: Video: png, none(pc), 25 tbr, 25 tbn, 25 tbc * Probing of hls containing dts/eac3 doesn't work anymore: $ ./ffmpeg -f lavfi -i sine=d=0.1 -strict -2 -c:a dca -f hls dca.hls -y &> /dev/null $ ./ffprobe dca.hls ffprobe version n3.2-dev-1377-gebf8ec5 Copyright (c) 2007-2016 the FFmpeg developers built with gcc 6.2.0 (Debian 6.2.0-6) 20161010 configuration: --disable-optimizations libavutil 55. 34.100 / 55. 34.100 libavcodec 57. 64.100 / 57. 64.100 libavformat57. 56.100 / 57. 56.100 libavdevice57. 1.100 / 57. 1.100 libavfilter 6. 65.100 / 6. 65.100 libswscale 4. 2.100 / 4. 2.100 libswresample 2. 3.100 / 2. 3.100 [hls,applehttp @ 0x3dfd7e0] Could not find codec parameters for stream 0 (Unknown: none ([130][0][0][0] / 0x0082)): unknown codec Consider increasing the value for the 'analyzeduration' and 'probesize' options Input #0, hls,applehttp, from 'dca.hls': Duration: 00:00:00.09, start: 1.40, bitrate: 9 kb/s Program 0 Metadata: variant_bitrate : 0 Stream #0:0: Unknown: none ([130][0][0][0] / 0x0082) Metadata: variant_bitrate : 0 Unsupported codec with id 0 for input stream 0 $ ./ffmpeg -f lavfi -i sine=d=0.1 -strict -2 -c:a eac3 -f hls eac3.hls -y &> /dev/null $ ./ffprobe eac3.hls ffprobe version n3.2-dev-1377-gebf8ec5 Copyright (c) 2007-2016 the FFmpeg developers built with gcc 6.2.0 (Debian 6.2.0-6) 20161010 configuration: --disable-optimizations libavutil 55. 34.100 / 55. 34.100 libavcodec 57. 64.100 / 57. 64.100 libavformat57. 56.100 / 57. 56.100 libavdevice57. 1.100 / 57. 1.100 libavfilter 6. 65.100 / 6. 65.100 libswscale 4. 2.100 / 4. 2.100 libswresample 2. 3.100 / 2. 3.100 Format mpegts detected only with low score of 2, misdetection possible! [hls,applehttp @ 0x2d297e0] Could not find codec parameters for stream 0 (Unknown: none ([135][0][0][0] / 0x0087)): unknown codec Consider increasing the value for the 'analyzeduration' and 'probesize' options Input #0, hls,applehttp, from 'eac3.hls': Duration: 00:00:00.07, start: 1.40, bitrate: 13 kb/s Program 0 Metadata: variant_bitrate : 0 Stream #0:0: Unknown: none ([135][0][0][0] / 0x0087) Metadata: variant_bitrate : 0 Unsupported codec with id 0 for input stream 0 * Probing of hls containing mp2 detects mp3: $ ./ffmpeg -f lavfi -i sine=d=0.1 -strict -2 -c:a mp2 -f hls mp2.hls -y &> /dev/null $ ./ffprobe mp2.hls ffprobe version n3.2-dev-1377-gebf8ec5 Copyright (c) 2007-2016 the FFmpeg developers built with gcc 6.2.0 (Debian 6.2.0-6) 20161010 configuration: --disable-optimizations libavutil 55. 34.100 / 55. 34.100 libavcodec 57. 64.100 / 57. 64.100 libavformat57. 56.100 / 57. 56.100 libavdevice57. 1.100 / 57. 1.100 libavfilter 6. 65.100 / 6. 65.100 libswscale 4. 2.100 / 4. 2.100 libswresample 2. 3.100 / 2. 3.100 Input #0, hls,applehttp, from 'mp2.hls': Duration: 00:00:00.08, start: 1.40, bitrate: 11 kb/s Program 0 Metadata: variant_bitrate : 0 Stream #0:0: Audio: mp3 ([3][0][0][0] / 0x0003), 44100 Hz, mono, s16p, 384 kb/s Metadata: variant_bitrate : 0 Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] FFmpeg 3.2
On 27.10.2016 18:44, James Almer wrote: > Could you do a bisect and trac ticket for these? HLS got a lot of commits in > the past > few months. > apng didn't, so that one is weird. I've found the commits causing the problems: * commit 5ef19590802f000299e418143fc2301e3f43affe causes the apng issue * commit 04964ac311abe670fb3b60290a330f2067544b13 causes the hls issues I'll skip trac and send patches partly reverting those commits. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] FFmpeg 3.2
On 27.10.2016 20:07, Michael Niedermayer wrote: > i didnt see this before making the release Bad timing, can happen. (I guess I should have checked this earlier, or rather FATE should have...) > but 3.2.1 is planed in a week or 2 Let's get it fixed in that release. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] ffmpeg: copy the extradata from encoder to muxer
This fixes creating apng files.
This partly reverts commit 5ef19590802f000299e418143fc2301e3f43affe.
Signed-off-by: Andreas Cadhalpun
---
ffmpeg.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/ffmpeg.c b/ffmpeg.c
index 3b91710..9971148 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -646,6 +646,7 @@ static void write_packet(OutputFile *of, AVPacket *pkt,
OutputStream *ost)
{
AVFormatContext *s = of->ctx;
AVStream *st = ost->st;
+AVCodecContext *avctx = ost->encoding_needed ? ost->enc_ctx :
ost->st->codec;
int ret;
if (!of->header_written) {
@@ -709,6 +710,16 @@ static void write_packet(OutputFile *of, AVPacket *pkt,
OutputStream *ost)
}
}
+if (!st->codecpar->extradata_size && avctx->extradata_size) {
+st->codecpar->extradata = av_mallocz(avctx->extradata_size +
AV_INPUT_BUFFER_PADDING_SIZE);
+if (!st->codecpar->extradata) {
+av_log(NULL, AV_LOG_ERROR, "Could not allocate extradata buffer to
copy parser data.\n");
+exit_program(1);
+}
+st->codecpar->extradata_size = avctx->extradata_size;
+memcpy(st->codecpar->extradata, avctx->extradata,
avctx->extradata_size);
+}
+
if (!(s->oformat->flags & AVFMT_NOTIMESTAMPS)) {
if (pkt->dts != AV_NOPTS_VALUE &&
pkt->pts != AV_NOPTS_VALUE &&
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] hls: always call avformat_find_stream_info for subdemuxers
This fixes probing dts/eac3/mp2 in hls.
This partly reverts commit 04964ac311abe670fb3b60290a330f2067544b13.
Signed-off-by: Andreas Cadhalpun
---
libavformat/hls.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/libavformat/hls.c b/libavformat/hls.c
index 3c09dd8..2364144 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -1750,17 +1750,9 @@ static int hls_read_header(AVFormatContext *s)
if (pls->is_id3_timestamped == -1)
av_log(s, AV_LOG_WARNING, "No expected HTTP requests have been
made\n");
-/*
- * For ID3 timestamped raw audio streams we need to detect the packet
- * durations to calculate timestamps in
fill_timing_for_id3_timestamped_stream(),
- * but for other streams we can rely on our user calling
avformat_find_stream_info()
- * on us if they want to.
- */
-if (pls->is_id3_timestamped) {
-ret = avformat_find_stream_info(pls->ctx, NULL);
-if (ret < 0)
-goto fail;
-}
+ret = avformat_find_stream_info(pls->ctx, NULL);
+if (ret < 0)
+goto fail;
pls->has_noheader_flag = !!(pls->ctx->ctx_flags & AVFMTCTX_NOHEADER);
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] hls: always call avformat_find_stream_info for subdemuxers
On 27.10.2016 21:30, Hendrik Leppkes wrote: > On Thu, Oct 27, 2016 at 9:20 PM, Andreas Cadhalpun > wrote: >> This fixes probing dts/eac3/mp2 in hls. >> >> This partly reverts commit 04964ac311abe670fb3b60290a330f2067544b13. >> > > That commit does a lot of magic flag handling to avoid calling that > function all the time (in fact that seems to be one of the main > purposes of that commit), I don't think a partial revert is a good > idea. I just wanted to keep the change minimal. Would you prefer a full revert of that commit? > At the very least I would like to hear from the author of that > particular commit. CC'ed the author. Better ideas/patches are always welcome. > PS: > AFAIK those codecs are not supported in HLS by the spec, either way. Maybe, but ffprobe handled them fine in the past, so if that wasn't intentionally broken, it should be fixed. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] ffmpeg: copy the extradata from encoder to muxer
On 27.10.2016 21:37, James Almer wrote:
> If apng encoder needs to add new extradata in a packet, it should do it
> with av_packet_new_side_data() instead.
Thanks for the hint. Attached is a patch fixing it that way.
Best regards,
Andreas
>From 4325ccdaf3bb7c74312cbaeb49d76fe535f18956 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun
Date: Thu, 27 Oct 2016 22:34:48 +0200
Subject: [PATCH] apng: use side data to pass extradata to muxer
This fixes creating apng files, which is broken since commit
5ef19590802f000299e418143fc2301e3f43affe.
Signed-off-by: Andreas Cadhalpun
---
libavcodec/pngenc.c | 4
libavformat/apngenc.c | 27 ---
2 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
index 00c830e..1a308f2 100644
--- a/libavcodec/pngenc.c
+++ b/libavcodec/pngenc.c
@@ -917,6 +917,10 @@ static int encode_apng(AVCodecContext *avctx, AVPacket *pkt,
if (s->last_frame) {
uint8_t* last_fctl_chunk_start = pkt->data;
uint8_t buf[26];
+uint8_t *side_data = av_packet_new_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA, avctx->extradata_size);
+if (!side_data)
+return AVERROR(ENOMEM);
+memcpy(side_data, avctx->extradata, avctx->extradata_size);
AV_WB32(buf + 0, s->last_frame_fctl.sequence_number);
AV_WB32(buf + 4, s->last_frame_fctl.width);
diff --git a/libavformat/apngenc.c b/libavformat/apngenc.c
index e25df2e..f702667 100644
--- a/libavformat/apngenc.c
+++ b/libavformat/apngenc.c
@@ -101,15 +101,29 @@ static int apng_write_header(AVFormatContext *format_context)
return 0;
}
-static void flush_packet(AVFormatContext *format_context, AVPacket *packet)
+static int flush_packet(AVFormatContext *format_context, AVPacket *packet)
{
APNGMuxContext *apng = format_context->priv_data;
AVIOContext *io_context = format_context->pb;
AVStream *codec_stream = format_context->streams[0];
AVCodecParameters *codec_par = codec_stream->codecpar;
+uint8_t *side_data = NULL;
+int side_data_size = 0;
av_assert0(apng->prev_packet);
+if (packet)
+side_data = av_packet_get_side_data(packet, AV_PKT_DATA_NEW_EXTRADATA, &side_data_size);
+
+if (side_data_size) {
+av_freep(&codec_par->extradata);
+codec_par->extradata = av_mallocz(side_data_size + AV_INPUT_BUFFER_PADDING_SIZE);
+if (!codec_par->extradata)
+return AVERROR(ENOMEM);
+codec_par->extradata_size = side_data_size;
+memcpy(codec_par->extradata, side_data, codec_par->extradata_size);
+}
+
if (apng->frame_number == 0 && !packet) {
uint8_t *existing_acTL_chunk;
uint8_t *existing_fcTL_chunk;
@@ -195,11 +209,13 @@ static void flush_packet(AVFormatContext *format_context, AVPacket *packet)
av_packet_unref(apng->prev_packet);
if (packet)
av_copy_packet(apng->prev_packet, packet);
+return 0;
}
static int apng_write_packet(AVFormatContext *format_context, AVPacket *packet)
{
APNGMuxContext *apng = format_context->priv_data;
+int ret;
if (!apng->prev_packet) {
apng->prev_packet = av_malloc(sizeof(*apng->prev_packet));
@@ -208,7 +224,9 @@ static int apng_write_packet(AVFormatContext *format_context, AVPacket *packet)
av_copy_packet(apng->prev_packet, packet);
} else {
-flush_packet(format_context, packet);
+ret = flush_packet(format_context, packet);
+if (ret < 0)
+return ret;
}
return 0;
@@ -219,10 +237,13 @@ static int apng_write_trailer(AVFormatContext *format_context)
APNGMuxContext *apng = format_context->priv_data;
AVIOContext *io_context = format_context->pb;
uint8_t buf[8];
+int ret;
if (apng->prev_packet) {
-flush_packet(format_context, NULL);
+ret = flush_packet(format_context, NULL);
av_freep(&apng->prev_packet);
+if (ret < 0)
+return ret;
}
apng_write_chunk(io_context, MKBETAG('I', 'E', 'N', 'D'), NULL, 0);
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec: validate codec parameters in avcodec_parameters_to_context
On 27.10.2016 22:14, Michael Niedermayer wrote: > On Wed, Oct 26, 2016 at 01:59:59AM +0200, Andreas Cadhalpun wrote: >> Note that the added asserts are triggered by *many* of my fuzzed samples. >> I'm happy to write patches for these cases, if we achieve agreement >> that the central check alone is insufficient. Have you seen this comment? >> utils.c | 82 >> ++-- >> 1 file changed, 80 insertions(+), 2 deletions(-) >> 40a8bafecb6d289a4220a27ac411fbcac3204952 >> 0001-avcodec-validate-codec-parameters.patch >> From f371be7a027da3958e221b4dc88ad558c1489107 Mon Sep 17 00:00:00 2001 >> From: Andreas Cadhalpun >> Date: Tue, 25 Oct 2016 01:45:27 +0200 >> Subject: [PATCH] avcodec: validate codec parameters >> >> This should reduce the impact of a broken demuxer (or API user) setting bogus >> codec parameters. >> >> The av_assert2 calls should ease detecting broken demuxers. > > have you tried a fuzzer ? > these assertions fail on fuzzed files > > Assertion 0 failed at libavcodec/utils.c:4157 > Aborted > Assertion !((unsigned)par->color_primaries > AVCOL_PRI_NB) failed at > libavcodec/utils.c:4161 As noted above, I'm well aware of that. This just shows how many demuxers currently set bogus values... Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
