[FFmpeg-devel] [PATCH 4/6] avformat/matroska: use av_stereo3d_alloc2()

[James Almer]

Signed-off-by: James Almer 
---
 libavformat/matroska.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libavformat/matroska.c b/libavformat/matroska.c
index f3e1be7..259e50f 100644
--- a/libavformat/matroska.c
+++ b/libavformat/matroska.c
@@ -151,9 +151,10 @@ const char * const 
ff_matroska_video_stereo_plane[MATROSKA_VIDEO_STEREO_PLANE_CO
 int ff_mkv_stereo3d_conv(AVStream *st, MatroskaVideoStereoModeType stereo_mode)
 {
 AVStereo3D *stereo;
+size_t stereo3d_size;
 int ret;
 
-stereo = av_stereo3d_alloc();
+stereo = av_stereo3d_alloc2(&stereo3d_size);
 if (!stereo)
 return AVERROR(ENOMEM);
 
@@ -195,7 +196,7 @@ int ff_mkv_stereo3d_conv(AVStream *st, 
MatroskaVideoStereoModeType stereo_mode)
 }
 
 ret = av_stream_add_side_data(st, AV_PKT_DATA_STEREO3D, (uint8_t *)stereo,
-  sizeof(*stereo));
+  stereo3d_size);
 if (ret < 0) {
 av_freep(&stereo);
 return ret;
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 3/6] avutil/stereo3d: add av_stereo3d_alloc2()

[James Almer]

Also deprecate av_stereo3d_alloc().

This new alloc function optionally returns the size of the AVStereo3D struct.

Signed-off-by: James Almer 
---
 libavutil/stereo3d.c | 12 
 libavutil/stereo3d.h | 14 ++
 2 files changed, 26 insertions(+)

diff --git a/libavutil/stereo3d.c b/libavutil/stereo3d.c
index a538364..bdea287 100644
--- a/libavutil/stereo3d.c
+++ b/libavutil/stereo3d.c
@@ -30,6 +30,18 @@ AVStereo3D *av_stereo3d_alloc(void)
 return av_mallocz(sizeof(AVStereo3D));
 }
 
+AVStereo3D *av_stereo3d_alloc2(size_t *size)
+{
+AVStereo3D *stereo3d = av_mallocz(sizeof(AVStereo3D));
+if (!stereo3d)
+return NULL;
+
+if (size)
+*size = sizeof(*stereo3d);
+
+return stereo3d;
+}
+
 AVStereo3D *av_stereo3d_create_side_data(AVFrame *frame)
 {
 AVFrameSideData *side_data = av_frame_new_side_data(frame,
diff --git a/libavutil/stereo3d.h b/libavutil/stereo3d.h
index 19c5416..d82a4b8 100644
--- a/libavutil/stereo3d.h
+++ b/libavutil/stereo3d.h
@@ -21,8 +21,10 @@
 #ifndef AVUTIL_STEREO3D_H
 #define AVUTIL_STEREO3D_H
 
+#include 
 #include 
 
+#include "attributes.h"
 #include "frame.h"
 
 /**
@@ -137,10 +139,22 @@ typedef struct AVStereo3D {
  * The resulting struct can be freed using av_freep().
  *
  * @return An AVStereo3D filled with default values or NULL on failure.
+ *
+ * @deprecated Use av_stereo3d_alloc2().
  */
+attribute_deprecated
 AVStereo3D *av_stereo3d_alloc(void);
 
 /**
+ * Allocate an AVStereo3D structure and set its fields to default values.
+ * The resulting struct can be freed using av_freep().
+ *
+ * @param size pointer for AVStereo3D structure size to store (optional)
+ * @return An AVStereo3D filled with default values or NULL on failure.
+ */
+AVStereo3D *av_stereo3d_alloc2(size_t *size);
+
+/**
  * Allocate a complete AVFrameSideData and add it to the frame.
  *
  * @param frame The frame which side data is added to.
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 5/6] avformat/mov: use av_stereo3d_alloc2()

[James Almer]

Signed-off-by: James Almer 
---
 libavformat/isom.h | 1 +
 libavformat/mov.c  | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 0fd9eb0..70c2af1 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -181,6 +181,7 @@ typedef struct MOVStreamContext {
 
 int32_t *display_matrix;
 AVStereo3D *stereo3d;
+size_t stereo3d_size;
 AVSphericalMapping *spherical;
 size_t spherical_size;
 
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 0b1c182..3f8fac7 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4535,7 +4535,7 @@ static int mov_read_st3d(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 return 0;
 }
 
-sc->stereo3d = av_stereo3d_alloc();
+sc->stereo3d = av_stereo3d_alloc2(&sc->stereo3d_size);
 if (!sc->stereo3d)
 return AVERROR(ENOMEM);
 
@@ -4670,7 +4670,7 @@ static int mov_parse_uuid_spherical(MOVStreamContext *sc, 
AVIOContext *pb, size_
 else
 mode = AV_STEREO3D_2D;
 
-sc->stereo3d = av_stereo3d_alloc();
+sc->stereo3d = av_stereo3d_alloc2(&sc->stereo3d_size);
 if (!sc->stereo3d)
 goto out;
 
@@ -5933,7 +5933,7 @@ static int mov_read_header(AVFormatContext *s)
 if (sc->stereo3d) {
 err = av_stream_add_side_data(st, AV_PKT_DATA_STEREO3D,
   (uint8_t *)sc->stereo3d,
-  sizeof(*sc->stereo3d));
+  sc->stereo3d_size);
 if (err < 0)
 return err;
 
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 6/6] avcodec/mjpegdec: use av_stereo3d_alloc2()

[James Almer]

Signed-off-by: James Almer 
---
 libavcodec/mjpegdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 742e07c..b0a6e00 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1783,7 +1783,7 @@ static int mjpeg_decode_app(MJpegDecodeContext *s)
 type   = get_bits(&s->gb, 8);
 len -= 4;
 
-s->stereo3d = av_stereo3d_alloc();
+s->stereo3d = av_stereo3d_alloc2(NULL);
 if (!s->stereo3d) {
 goto out;
 }
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/6] avformat/matroskadec: use mastering display metadata's own alloc function

[James Almer]

Signed-off-by: James Almer 
---
 libavformat/matroskadec.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 58731aa..493d63f 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1867,14 +1867,12 @@ static int mkv_parse_video_color(AVStream *st, const 
MatroskaTrack *track) {
 // Use similar rationals as other standards.
 const int chroma_den = 5;
 const int luma_den = 1;
-AVMasteringDisplayMetadata *metadata =
-(AVMasteringDisplayMetadata*) av_stream_new_side_data(
-st, AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
-sizeof(AVMasteringDisplayMetadata));
+size_t mastering_size;
+int ret;
+AVMasteringDisplayMetadata *metadata = 
av_mastering_display_metadata_alloc2(&mastering_size);
 if (!metadata) {
 return AVERROR(ENOMEM);
 }
-memset(metadata, 0, sizeof(AVMasteringDisplayMetadata));
 if (has_mastering_primaries) {
 metadata->display_primaries[0][0] = av_make_q(
 round(mastering_meta->r_x * chroma_den), chroma_den);
@@ -1901,6 +1899,13 @@ static int mkv_parse_video_color(AVStream *st, const 
MatroskaTrack *track) {
 round(mastering_meta->min_luminance * luma_den), luma_den);
 metadata->has_luminance = 1;
 }
+
+ret = av_stream_add_side_data(st, 
AV_PKT_DATA_MASTERING_DISPLAY_METADATA,
+  (uint8_t *)metadata, mastering_size);
+if (ret < 0) {
+av_freep(&metadata);
+return ret;
+}
 }
 return 0;
 }
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/6] avutil/mastering_display_metadata: add av_mastering_display_metadata_alloc2()

[James Almer]

Also deprecate av_mastering_display_metadata_alloc().

This new alloc function optionally returns the size of the 
AVMasteringDisplayMetadata
struct.

Signed-off-by: James Almer 
---
 libavutil/mastering_display_metadata.c | 12 
 libavutil/mastering_display_metadata.h | 17 +
 2 files changed, 29 insertions(+)

diff --git a/libavutil/mastering_display_metadata.c 
b/libavutil/mastering_display_metadata.c
index e1683e5..872f14b 100644
--- a/libavutil/mastering_display_metadata.c
+++ b/libavutil/mastering_display_metadata.c
@@ -29,6 +29,18 @@ AVMasteringDisplayMetadata 
*av_mastering_display_metadata_alloc(void)
 return av_mallocz(sizeof(AVMasteringDisplayMetadata));
 }
 
+AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc2(size_t *size)
+{
+AVMasteringDisplayMetadata *mastering = 
av_mallocz(sizeof(AVMasteringDisplayMetadata));
+if (!mastering)
+return NULL;
+
+if (size)
+*size = sizeof(*mastering);
+
+return mastering;
+}
+
 AVMasteringDisplayMetadata 
*av_mastering_display_metadata_create_side_data(AVFrame *frame)
 {
 AVFrameSideData *side_data = av_frame_new_side_data(frame,
diff --git a/libavutil/mastering_display_metadata.h 
b/libavutil/mastering_display_metadata.h
index 936533f..32357b3 100644
--- a/libavutil/mastering_display_metadata.h
+++ b/libavutil/mastering_display_metadata.h
@@ -21,6 +21,10 @@
 #ifndef AVUTIL_MASTERING_DISPLAY_METADATA_H
 #define AVUTIL_MASTERING_DISPLAY_METADATA_H
 
+#include 
+#include 
+
+#include "attributes.h"
 #include "frame.h"
 #include "rational.h"
 
@@ -74,10 +78,23 @@ typedef struct AVMasteringDisplayMetadata {
  *
  * @return An AVMasteringDisplayMetadata filled with default values or NULL
  * on failure.
+ *
+ * @deprecated Use av_mastering_display_metadata_alloc2().
  */
+attribute_deprecated
 AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void);
 
 /**
+ * Allocate an AVMasteringDisplayMetadata structure and set its fields to
+ * default values. The resulting struct can be freed using av_freep().
+ *
+ * @param size pointer for AVMasteringDisplayMetadata structure size to store 
(optional)
+ * @return An AVMasteringDisplayMetadata filled with default values or NULL
+ * on failure.
+ */
+AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc2(size_t *size);
+
+/**
  * Allocate a complete AVMasteringDisplayMetadata and add it to the frame.
  *
  * @param frame The frame which side data is added to.
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Ronald S. Bultje]

Hi,

On Sat, Dec 10, 2016 at 7:17 PM, Carl Eugen Hoyos 
wrote:

> 2016-12-10 23:59 GMT+01:00 James Almer :
>
> >>> Again, I'm against this. This is not news entry worthy.
> >>
> >> Then please revert the configure change:
> >> This is (by far) the biggest change in configure behaviour since
> forever,
> >> this has to be communicated and this was the condition for the patch.
> >
> > I don't think reverting an (allegedly wanted and beneficial) change just
>
> What does "wanted" mean in this context?
>
> The change was reviewed and it was ok with a Changelog and a news
> entry. The patch was applied, so please add the news entry.
>
> > because you wont accept it unless it has its own news entry makes sense
> > or is even fair to the person that wrote it and submitted it.
> >
> >>
> >>> A line can be added to the next release news entry announcement, or
> >>> to the RELEASE_NOTES files, but not alone.
> >>
> >> If you care, add it to the release notes, but this cannot replace the
> >> news entry.
> >
> > The RELEASE_NOTES mention is the only thing that's a must for this,
> > aside from the Changelog entry.
>
> I disagree but please feel free to add this to release notes if you want.
>
> The news entry is needed for this change.


... according to you...

Right?

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec: Require avoptions for the user to set max_pixels.

[James Almer]

On 12/10/2016 9:23 PM, Michael Niedermayer wrote:
> On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:
>> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
>>> When we will backport this, it will be inevitably in a different location
>>> in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
>>> use the same soname though and must have a binary compatible interface.
>>> It thus can only saftely be accessed through AVOptions
>>>
>>> It may be enough to require this only in the releases but that could be
>>> rather confusing.
>>
>> Wouldn't it be better to initially add such new fields inside
>> AVCodecContext->internal and accessible by AVOptions only so they
>> may be trivial to backport, then once a major bump occurs they can
>> be moved to the actual public struct with enabled direct access?
>>
>> Not sure how feasible is to have options_table.h options change
>> values from that internal struct, but it would solve the above
>> concerns.
> 
> AVCodecInternal does not start with a AVClass / AVOption table
> (thats solvable though would need to be done to past releases)
> 
> AVCodecContext->internal is not a child class of AVCodecContext
> (that is solvable too, if we want to take the risk to do that in
>  point releases)
> 
> AVCodecContext->internal is allocated in avcodec_open2() so nothing
> can be set in it by the user before
> (i dont see a solution to this that we would want to backport)
> 
> If you suggest to make these changes now to for future use.
> that will make backports across this change very annoying, as theres
> a point before AVCodecInternal cannot be used.

If this is impossible now for the above reasons then it's something
that could be considered for the next bump, or the one after that
if it requires careful planning and design.
I was simply suggesting something that could prevent backporting
fields on public structs that would not have a fixed offset.

The plan was to undo all the current ones in the next bump now that
we dropped ABI compatibility with Libav, so it would be nice to find
a way to avoid something like that happening again.

> And also theres more work for us to maintain seperate implementations
> for the options, all code accessing them has to deal with them being
> in different places, making all related backports harder.
> 
> To me that looks like a disadvantage from every side
> 
> I think the real solution is to start liking AVOptions, they make
> our work easier.

AVOptions are fine. Private-but-not-really and no-direct-access fields
in public structs are what's kinda ugly an unpopular.

No objections or other comments for this patch from me.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/matroskadec: allocate Colour related fields only if the file contains the relevant master

[James Almer]

On 12/8/2016 6:31 PM, James Almer wrote:
> Ping for patchset.
> 

Pushed.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec: Require avoptions for the user to set max_pixels.

[Michael Niedermayer]

On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:
> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
> > When we will backport this, it will be inevitably in a different location
> > in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> > use the same soname though and must have a binary compatible interface.
> > It thus can only saftely be accessed through AVOptions
> > 
> > It may be enough to require this only in the releases but that could be
> > rather confusing.
> 
> Wouldn't it be better to initially add such new fields inside
> AVCodecContext->internal and accessible by AVOptions only so they
> may be trivial to backport, then once a major bump occurs they can
> be moved to the actual public struct with enabled direct access?
> 
> Not sure how feasible is to have options_table.h options change
> values from that internal struct, but it would solve the above
> concerns.
[...]

If people want we can write something like:

/**
 * The number of pixels per image to maximally accept.
 *
 * - decoding: set by user through AVOptions (NO direct access before 
57.67.100)
 * - encoding: set by user through AVOptions (NO direct access before 
57.67.100)
 */
int64_t max_pixels;

This would allow direct access in the future and require AVOptions in
releases but its quite complex


[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec: Require avoptions for the user to set max_pixels.

[Michael Niedermayer]

On Sat, Dec 10, 2016 at 08:31:57PM -0300, James Almer wrote:
> On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
> > When we will backport this, it will be inevitably in a different location
> > in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> > use the same soname though and must have a binary compatible interface.
> > It thus can only saftely be accessed through AVOptions
> > 
> > It may be enough to require this only in the releases but that could be
> > rather confusing.
> 
> Wouldn't it be better to initially add such new fields inside
> AVCodecContext->internal and accessible by AVOptions only so they
> may be trivial to backport, then once a major bump occurs they can
> be moved to the actual public struct with enabled direct access?
> 
> Not sure how feasible is to have options_table.h options change
> values from that internal struct, but it would solve the above
> concerns.

AVCodecInternal does not start with a AVClass / AVOption table
(thats solvable though would need to be done to past releases)

AVCodecContext->internal is not a child class of AVCodecContext
(that is solvable too, if we want to take the risk to do that in
 point releases)

AVCodecContext->internal is allocated in avcodec_open2() so nothing
can be set in it by the user before
(i dont see a solution to this that we would want to backport)

If you suggest to make these changes now to for future use.
that will make backports across this change very annoying, as theres
a point before AVCodecInternal cannot be used.
And also theres more work for us to maintain seperate implementations
for the options, all code accessing them has to deal with them being
in different places, making all related backports harder.

To me that looks like a disadvantage from every side

I think the real solution is to start liking AVOptions, they make
our work easier.

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Carl Eugen Hoyos]

2016-12-10 23:59 GMT+01:00 James Almer :

>>> Again, I'm against this. This is not news entry worthy.
>>
>> Then please revert the configure change:
>> This is (by far) the biggest change in configure behaviour since forever,
>> this has to be communicated and this was the condition for the patch.
>
> I don't think reverting an (allegedly wanted and beneficial) change just

What does "wanted" mean in this context?

The change was reviewed and it was ok with a Changelog and a news
entry. The patch was applied, so please add the news entry.

> because you wont accept it unless it has its own news entry makes sense
> or is even fair to the person that wrote it and submitted it.
>
>>
>>> A line can be added to the next release news entry announcement, or
>>> to the RELEASE_NOTES files, but not alone.
>>
>> If you care, add it to the release notes, but this cannot replace the
>> news entry.
>
> The RELEASE_NOTES mention is the only thing that's a must for this,
> aside from the Changelog entry.

I disagree but please feel free to add this to release notes if you want.

The news entry is needed for this change.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Carl Eugen Hoyos]

2016-12-10 23:37 GMT+01:00 Hendrik Leppkes :
>>> simple behavior changes in git master should not
>>> be posted as their own news entries.
>>
>> Of course!
>> But this is not a "simple" change but the biggest change in
>> configure behaviour in the last decade.
>>
>
> Not really, it just makes all options consistent. We had various
> --enable-* options that failed when not found and various that didn't
> fail. If anything its a bugfix.

This is about auto-detected options, not optional libraries
that you are talking about.

Please understand that such information to our users
is important and needed.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec: Require avoptions for the user to set max_pixels.

[James Almer]

On 12/10/2016 7:01 PM, Michael Niedermayer wrote:
> When we will backport this, it will be inevitably in a different location
> in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
> use the same soname though and must have a binary compatible interface.
> It thus can only saftely be accessed through AVOptions
> 
> It may be enough to require this only in the releases but that could be
> rather confusing.

Wouldn't it be better to initially add such new fields inside
AVCodecContext->internal and accessible by AVOptions only so they
may be trivial to backport, then once a major bump occurs they can
be moved to the actual public struct with enabled direct access?

Not sure how feasible is to have options_table.h options change
values from that internal struct, but it would solve the above
concerns.

> 
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/avcodec.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 02234aee67..8123092ac0 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3573,8 +3573,8 @@ typedef struct AVCodecContext {
>  /**
>   * The number of pixels per image to maximally accept.
>   *
> - * - decoding: set by user
> - * - encoding: set by user
> + * - decoding: set by user through AVOptions (NO direct access)
> + * - encoding: set by user through AVOptions (NO direct access)
>   */
>  int64_t max_pixels;
>  
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffplay: fix sws_scale possible out of bounds array access

[Michael Niedermayer]

On Sat, Dec 10, 2016 at 11:39:25PM +0100, Marton Balint wrote:
> 
> On Sat, 10 Dec 2016, Michael Niedermayer wrote:
> 
> >On Sat, Dec 10, 2016 at 01:06:59PM +0100, Marton Balint wrote:
> >>As I used simple RGBA formats for subtitles and for the video texture if
> >>avfilter is disabled I kind of assumed that sws_scale won't access data
> >>pointers and strides above index 0, but apparently that is not the case.
> >>
> >>Fixes Coverity CID 1396737, 1396738, 1396739, 1396740.
> >>
> >>Signed-off-by: Marton Balint 
> >>---
> >> ffplay.c | 16 
> >> 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> >LGTM
> >
> >thx
> >
> >also please backport to the releases
> >
> 
> Pushed to master and 3.2. 3.1 and before is using the SDL1 version
> which is not affected.

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[James Almer]

On 12/10/2016 7:27 PM, Carl Eugen Hoyos wrote:
> 2016-12-10 23:24 GMT+01:00 James Almer :
>> On 12/10/2016 7:22 PM, Carl Eugen Hoyos wrote:
>>> 2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun 
>>> :
 Suggested-by: Carl Eugen Hoyos 
 Signed-off-by: Andreas Cadhalpun 
 ---
  src/index | 7 +++
  1 file changed, 7 insertions(+)

 diff --git a/src/index b/src/index
 index c203676..8f533f5 100644
 --- a/src/index
 +++ b/src/index
 @@ -37,6 +37,13 @@
  News


 +  December 10th, 2016, stricter configure 
 behavior.
 +  
 +The configure script now fails if autodetect-libraries are requested 
 but not found.
 +  
 +  
 +If for example SDL headers are not available, '--enable-sdl2' will 
 cause a configure failure.
>>>
>>> Please push, esp. since the relevant change is already in git.
>>>
>>> Thank you, Carl Eugen
>>
>> Again, I'm against this. This is not news entry worthy.
> 
> Then please revert the configure change:
> This is (by far) the biggest change in configure behaviour since forever,
> this has to be communicated and this was the condition for the patch.

I don't think reverting an (allegedly wanted and beneficial) change just
because you wont accept it unless it has its own news entry makes sense
or is even fair to the person that wrote it and submitted it.

> 
>> A line can be added to the next release news entry announcement, or
>> to the RELEASE_NOTES files, but not alone.
> 
> If you care, add it to the release notes, but this cannot replace the
> news entry.

The RELEASE_NOTES mention is the only thing that's a must for this,
aside from the Changelog entry.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Marton Balint]

On Sat, 10 Dec 2016, James Almer wrote:


On 12/10/2016 7:22 PM, Carl Eugen Hoyos wrote:

2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun :

Suggested-by: Carl Eugen Hoyos 
Signed-off-by: Andreas Cadhalpun 
---
 src/index | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/index b/src/index
index c203676..8f533f5 100644
--- a/src/index
+++ b/src/index
@@ -37,6 +37,13 @@
 News
   

+  December 10th, 2016, stricter configure 
behavior.
+  
+The configure script now fails if autodetect-libraries are requested but 
not found.
+  
+  
+If for example SDL headers are not available, '--enable-sdl2' will cause a 
configure failure.


Please push, esp. since the relevant change is already in git.

Thank you, Carl Eugen


Again, I'm against this. This is not news entry worthy.

A line can be added to the next release news entry announcement, or
to the RELEASE_NOTES files, but not alone.



Please, be more accepting about patches which has no technical downside,
if Carl find this impotant, it is no use to start a lengthy thread
discussing it, which in the worst case will cause another vote, and some 
people will either feel bad or trolled after it.


Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[James Almer]

On 12/10/2016 7:34 PM, Carl Eugen Hoyos wrote:
> 2016-12-10 23:32 GMT+01:00 Hendrik Leppkes :
>> On Sat, Dec 10, 2016 at 11:24 PM, James Almer  wrote:
>>> On 12/10/2016 7:22 PM, Carl Eugen Hoyos wrote:
 2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun 
 :
> Suggested-by: Carl Eugen Hoyos 
> Signed-off-by: Andreas Cadhalpun 
> ---
>  src/index | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/src/index b/src/index
> index c203676..8f533f5 100644
> --- a/src/index
> +++ b/src/index
> @@ -37,6 +37,13 @@
>  News
>
>
> +  December 10th, 2016, stricter configure 
> behavior.
> +  
> +The configure script now fails if autodetect-libraries are requested 
> but not found.
> +  
> +  
> +If for example SDL headers are not available, '--enable-sdl2' will 
> cause a configure failure.

 Please push, esp. since the relevant change is already in git.

 Thank you, Carl Eugen
>>>
>>> Again, I'm against this. This is not news entry worthy.
>>>
>>> A line can be added to the next release news entry announcement, or
>>> to the RELEASE_NOTES files, but not alone.
>>>
>>
>> I agree with James,
> 
>> simple behavior changes in git master should not
>> be posted as their own news entries.
> 
> Of course!
> But this is not a "simple" change but the biggest change in
> configure behaviour in the last decade.
> 
> Carl Eugen

It is still a behavior change and specific to one part of the package
that is the configure script.
We don't announce when we increase the minimum version required for
some external library, or when we "upgrade" a library to auto-detect,
and that's something that has as many chances to break existing scripts 
from distros running specific configure line option combinations as
this behavior change recently introduced.
We don't even add a line in Changelog for those in general.

If this is really that important then it can have a line in the news
entry for the upcoming release, and of course should be mentioned in
the RELEASE_NOTES file (Assuming someone actually writes one this time
instead of reusing the boilerplate one from the past several versions).

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Carl Eugen Hoyos]

2016-12-10 23:24 GMT+01:00 James Almer :
> On 12/10/2016 7:22 PM, Carl Eugen Hoyos wrote:
>> 2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun 
>> :
>>> Suggested-by: Carl Eugen Hoyos 
>>> Signed-off-by: Andreas Cadhalpun 
>>> ---
>>>  src/index | 7 +++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/src/index b/src/index
>>> index c203676..8f533f5 100644
>>> --- a/src/index
>>> +++ b/src/index
>>> @@ -37,6 +37,13 @@
>>>  News
>>>
>>>
>>> +  December 10th, 2016, stricter configure 
>>> behavior.
>>> +  
>>> +The configure script now fails if autodetect-libraries are requested 
>>> but not found.
>>> +  
>>> +  
>>> +If for example SDL headers are not available, '--enable-sdl2' will 
>>> cause a configure failure.
>>
>> Please push, esp. since the relevant change is already in git.
>>
>> Thank you, Carl Eugen
>
> Again, I'm against this. This is not news entry worthy.

Then please revert the configure change:
This is (by far) the biggest change in configure behaviour since forever,
this has to be communicated and this was the condition for the patch.

> A line can be added to the next release news entry announcement, or
> to the RELEASE_NOTES files, but not alone.

If you care, add it to the release notes, but this cannot replace the
news entry.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffplay: fix sws_scale possible out of bounds array access

[Marton Balint]

On Sat, 10 Dec 2016, Michael Niedermayer wrote:


On Sat, Dec 10, 2016 at 01:06:59PM +0100, Marton Balint wrote:

As I used simple RGBA formats for subtitles and for the video texture if
avfilter is disabled I kind of assumed that sws_scale won't access data
pointers and strides above index 0, but apparently that is not the case.

Fixes Coverity CID 1396737, 1396738, 1396739, 1396740.

Signed-off-by: Marton Balint 
---
 ffplay.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)


LGTM

thx

also please backport to the releases



Pushed to master and 3.2. 3.1 and before is using the SDL1 version which 
is not affected.


Thanks,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Hendrik Leppkes]

On Sat, Dec 10, 2016 at 11:34 PM, Carl Eugen Hoyos  wrote:
> 2016-12-10 23:32 GMT+01:00 Hendrik Leppkes :
>> On Sat, Dec 10, 2016 at 11:24 PM, James Almer  wrote:
>>> On 12/10/2016 7:22 PM, Carl Eugen Hoyos wrote:
 2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun 
 :
> Suggested-by: Carl Eugen Hoyos 
> Signed-off-by: Andreas Cadhalpun 
> ---
>  src/index | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/src/index b/src/index
> index c203676..8f533f5 100644
> --- a/src/index
> +++ b/src/index
> @@ -37,6 +37,13 @@
>  News
>
>
> +  December 10th, 2016, stricter configure 
> behavior.
> +  
> +The configure script now fails if autodetect-libraries are requested 
> but not found.
> +  
> +  
> +If for example SDL headers are not available, '--enable-sdl2' will 
> cause a configure failure.

 Please push, esp. since the relevant change is already in git.

 Thank you, Carl Eugen
>>>
>>> Again, I'm against this. This is not news entry worthy.
>>>
>>> A line can be added to the next release news entry announcement, or
>>> to the RELEASE_NOTES files, but not alone.
>>>
>>
>> I agree with James,
>
>> simple behavior changes in git master should not
>> be posted as their own news entries.
>
> Of course!
> But this is not a "simple" change but the biggest change in
> configure behaviour in the last decade.
>

Not really, it just makes all options consistent. We had various
--enable-* options that failed when not found and various that didn't
fail. If anything its a bugfix.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Ronald S. Bultje]

Hi,

On Sat, Dec 10, 2016 at 5:24 PM, Carl Eugen Hoyos 
wrote:

> 2016-12-10 14:07 GMT+01:00 Ronald S. Bultje :
> > Hi,
> >
> > On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos 
> > wrote:
> >
> >> 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje :
> >>
> >> > On IRC, we discussed at what values OOM start occurring, which
> >> > seems to be around 30k-60k
> >>
> >> This is not true, why do you think so?
> >
> > http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/
> 2016-December/003980.html
>
> http://www.openwall.com/lists/oss-security/2016/12/08/1
> Iiuc (which isn't sure at all) this is about 26000 streams.


And how does that fundamentally change the discussion? The question was
about orders of magnitude (think powers of 10), so you just changes the
answer from log10(30k)=4.48 to log10(26k)=4.41. In both cases, a sensible
limit is something like exp10(3) or exp10(4), but exp10(2) is not necessary
IMHO,

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Carl Eugen Hoyos]

2016-12-10 23:32 GMT+01:00 Hendrik Leppkes :
> On Sat, Dec 10, 2016 at 11:24 PM, James Almer  wrote:
>> On 12/10/2016 7:22 PM, Carl Eugen Hoyos wrote:
>>> 2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun 
>>> :
 Suggested-by: Carl Eugen Hoyos 
 Signed-off-by: Andreas Cadhalpun 
 ---
  src/index | 7 +++
  1 file changed, 7 insertions(+)

 diff --git a/src/index b/src/index
 index c203676..8f533f5 100644
 --- a/src/index
 +++ b/src/index
 @@ -37,6 +37,13 @@
  News


 +  December 10th, 2016, stricter configure 
 behavior.
 +  
 +The configure script now fails if autodetect-libraries are requested 
 but not found.
 +  
 +  
 +If for example SDL headers are not available, '--enable-sdl2' will 
 cause a configure failure.
>>>
>>> Please push, esp. since the relevant change is already in git.
>>>
>>> Thank you, Carl Eugen
>>
>> Again, I'm against this. This is not news entry worthy.
>>
>> A line can be added to the next release news entry announcement, or
>> to the RELEASE_NOTES files, but not alone.
>>
>
> I agree with James,

> simple behavior changes in git master should not
> be posted as their own news entries.

Of course!
But this is not a "simple" change but the biggest change in
configure behaviour in the last decade.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Hendrik Leppkes]

On Sat, Dec 10, 2016 at 11:24 PM, James Almer  wrote:
> On 12/10/2016 7:22 PM, Carl Eugen Hoyos wrote:
>> 2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun 
>> :
>>> Suggested-by: Carl Eugen Hoyos 
>>> Signed-off-by: Andreas Cadhalpun 
>>> ---
>>>  src/index | 7 +++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/src/index b/src/index
>>> index c203676..8f533f5 100644
>>> --- a/src/index
>>> +++ b/src/index
>>> @@ -37,6 +37,13 @@
>>>  News
>>>
>>>
>>> +  December 10th, 2016, stricter configure 
>>> behavior.
>>> +  
>>> +The configure script now fails if autodetect-libraries are requested 
>>> but not found.
>>> +  
>>> +  
>>> +If for example SDL headers are not available, '--enable-sdl2' will 
>>> cause a configure failure.
>>
>> Please push, esp. since the relevant change is already in git.
>>
>> Thank you, Carl Eugen
>
> Again, I'm against this. This is not news entry worthy.
>
> A line can be added to the next release news entry announcement, or
> to the RELEASE_NOTES files, but not alone.
>

I agree with James, simple behavior changes in git master should not
be posted as their own news entries. We have changelogs for this and
release summary announcements.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Carl Eugen Hoyos]

2016-12-10 14:07 GMT+01:00 Ronald S. Bultje :
> Hi,
>
> On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos 
> wrote:
>
>> 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje :
>>
>> > On IRC, we discussed at what values OOM start occurring, which
>> > seems to be around 30k-60k
>>
>> This is not true, why do you think so?
>
> http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/2016-December/003980.html

http://www.openwall.com/lists/oss-security/2016/12/08/1
Iiuc (which isn't sure at all) this is about 26000 streams.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[James Almer]

On 12/10/2016 7:22 PM, Carl Eugen Hoyos wrote:
> 2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun 
> :
>> Suggested-by: Carl Eugen Hoyos 
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  src/index | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/src/index b/src/index
>> index c203676..8f533f5 100644
>> --- a/src/index
>> +++ b/src/index
>> @@ -37,6 +37,13 @@
>>  News
>>
>>
>> +  December 10th, 2016, stricter configure 
>> behavior.
>> +  
>> +The configure script now fails if autodetect-libraries are requested 
>> but not found.
>> +  
>> +  
>> +If for example SDL headers are not available, '--enable-sdl2' will 
>> cause a configure failure.
> 
> Please push, esp. since the relevant change is already in git.
> 
> Thank you, Carl Eugen

Again, I'm against this. This is not news entry worthy.

A line can be added to the next release news entry announcement, or
to the RELEASE_NOTES files, but not alone.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Carl Eugen Hoyos]

2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun :
> Suggested-by: Carl Eugen Hoyos 
> Signed-off-by: Andreas Cadhalpun 
> ---
>  src/index | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/src/index b/src/index
> index c203676..8f533f5 100644
> --- a/src/index
> +++ b/src/index
> @@ -37,6 +37,13 @@
>  News
>
>
> +  December 10th, 2016, stricter configure 
> behavior.
> +  
> +The configure script now fails if autodetect-libraries are requested but 
> not found.
> +  
> +  
> +If for example SDL headers are not available, '--enable-sdl2' will cause 
> a configure failure.

Please push, esp. since the relevant change is already in git.

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avcodec: Require avoptions for the user to set max_pixels.

[Michael Niedermayer]

When we will backport this, it will be inevitably in a different location
in AVCodecContext in each release and master. 3.0, 3.1, 3.2 and master
use the same soname though and must have a binary compatible interface.
It thus can only saftely be accessed through AVOptions

It may be enough to require this only in the releases but that could be
rather confusing.

Signed-off-by: Michael Niedermayer 
---
 libavcodec/avcodec.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 02234aee67..8123092ac0 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3573,8 +3573,8 @@ typedef struct AVCodecContext {
 /**
  * The number of pixels per image to maximally accept.
  *
- * - decoding: set by user
- * - encoding: set by user
+ * - decoding: set by user through AVOptions (NO direct access)
+ * - encoding: set by user through AVOptions (NO direct access)
  */
 int64_t max_pixels;
 
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avutil/tests: run the cpu_init.c test conditionally on HAVE_THREADS

[James Almer]

On 12/6/2016 4:00 PM, Wan-Teh Chang wrote:
> Also declare the main() function with void arguments because argc and
> argv are unused.
> 
> These changes are suggested by Diego Biurrun and James Almer.
> 
> Signed-off-by: Wan-Teh Chang 
> ---
>  libavutil/Makefile | 2 +-
>  libavutil/tests/cpu_init.c | 9 +
>  tests/fate/libavutil.mak   | 2 +-
>  3 files changed, 3 insertions(+), 10 deletions(-)

Split and pushed, thanks.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 08:47:00PM +0100, Paul B Mahol wrote:
> On 12/9/16, Michael Niedermayer  wrote:
> > Adds av_image_check_size2() with max_pixels and pix_fmt parameters.
> > pix_fmt is unused and is added to avoid a 2nd API change later
> >
> > The old function uses AVOptions to obtain the max_pixels value to simplify
> > the transition.
> >
> > TODO: split into 2 patches (one per lib), docs & bump
> >
> > This allows preventing some OOM and "slow decoding" cases by limiting the
> > maximum resolution
> > this may be useful to avoid fuzzers getting stuck in boring cases
> >
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavcodec/avcodec.h |  8 
> >  libavcodec/options_table.h   |  1 +
> >  libavutil/imgutils.c | 31 ++-
> >  libavutil/imgutils.h | 14 ++
> >  tests/ref/fate/api-mjpeg-codec-param |  2 ++
> >  tests/ref/fate/api-png-codec-param   |  2 ++
> >  6 files changed, 53 insertions(+), 5 deletions(-)
> >
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 7ac2adaf66..81052b10ef 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
> >   */
> >  int trailing_padding;
> >
> > +/**
> > + * The number of pixels per image to maximally accept.
> > + *
> > + * - decoding: set by user
> > + * - encoding: unused
> > + */
> > +int max_pixels;
> 
> int64_t please

changed

[...]

thx
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Michael Niedermayer]

On Sat, Dec 10, 2016 at 01:33:59PM +0100, Andreas Cadhalpun wrote:
> On 09.12.2016 20:31, Michael Niedermayer wrote:
> > Adds av_image_check_size2() with max_pixels and pix_fmt parameters.
> > pix_fmt is unused and is added to avoid a 2nd API change later
> > 
> > The old function uses AVOptions to obtain the max_pixels value to simplify
> > the transition.
> > 
> > TODO: split into 2 patches (one per lib), docs & bump
> > 
> > This allows preventing some OOM and "slow decoding" cases by limiting the 
> > maximum resolution
> > this may be useful to avoid fuzzers getting stuck in boring cases
> 
> Setting this option to a reasonably low value like 100 fixes a
> significant amount of my slow samples. So having it is definitely useful.
> 
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavcodec/avcodec.h |  8 
> >  libavcodec/options_table.h   |  1 +
> >  libavutil/imgutils.c | 31 ++-
> >  libavutil/imgutils.h | 14 ++
> >  tests/ref/fate/api-mjpeg-codec-param |  2 ++
> >  tests/ref/fate/api-png-codec-param   |  2 ++
> >  6 files changed, 53 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 7ac2adaf66..81052b10ef 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
> >   */
> >  int trailing_padding;
> >  
> > +/**
> > + * The number of pixels per image to maximally accept.
> > + *
> > + * - decoding: set by user
> > + * - encoding: unused
> 
> It should work for encoding as well, doesn't it?

enabled it for encoding too

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Michael Niedermayer]

On Sat, Dec 10, 2016 at 12:20:01PM +0100, wm4 wrote:
> On Fri,  9 Dec 2016 20:31:40 +0100
> Michael Niedermayer  wrote:
> 
> > Adds av_image_check_size2() with max_pixels and pix_fmt parameters.
> > pix_fmt is unused and is added to avoid a 2nd API change later
> > 
> > The old function uses AVOptions to obtain the max_pixels value to simplify
> > the transition.
> > 
> > TODO: split into 2 patches (one per lib), docs & bump
> > 
> > This allows preventing some OOM and "slow decoding" cases by limiting the 
> > maximum resolution
> > this may be useful to avoid fuzzers getting stuck in boring cases
> > 
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavcodec/avcodec.h |  8 
> >  libavcodec/options_table.h   |  1 +
> >  libavutil/imgutils.c | 31 ++-
> >  libavutil/imgutils.h | 14 ++
> >  tests/ref/fate/api-mjpeg-codec-param |  2 ++
> >  tests/ref/fate/api-png-codec-param   |  2 ++
> >  6 files changed, 53 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 7ac2adaf66..81052b10ef 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
> >   */
> >  int trailing_padding;
> >  
> > +/**
> > + * The number of pixels per image to maximally accept.
> > + *
> > + * - decoding: set by user
> > + * - encoding: unused
> > + */
> > +int max_pixels;
> > +
> >  } AVCodecContext;
> >  
> >  AVRational av_codec_get_pkt_timebase (const AVCodecContext *avctx);
> > diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> > index ee79859953..2f5eb252fe 100644
> > --- a/libavcodec/options_table.h
> > +++ b/libavcodec/options_table.h
> > @@ -570,6 +570,7 @@ static const AVOption avcodec_options[] = {
> >  {"codec_whitelist", "List of decoders that are allowed to be used", 
> > OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
> > CHAR_MAX, A|V|S|D },
> >  {"pixel_format", "set pixel format", OFFSET(pix_fmt), 
> > AV_OPT_TYPE_PIXEL_FMT, {.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
> >  {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, 
> > {.str=NULL}, 0, INT_MAX, 0 },
> > +{"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), 
> > AV_OPT_TYPE_INT, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D },
> >  {NULL},
> >  };
> >  
> > diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> > index 37808e53d0..96f2bbdc4f 100644
> > --- a/libavutil/imgutils.c
> > +++ b/libavutil/imgutils.c
> > @@ -30,6 +30,7 @@
> >  #include "mathematics.h"
> >  #include "pixdesc.h"
> >  #include "rational.h"
> > +#include "opt.h"
> >  
> >  void av_image_fill_max_pixsteps(int max_pixsteps[4], int 
> > max_pixstep_comps[4],
> >  const AVPixFmtDescriptor *pixdesc)
> > @@ -248,7 +249,7 @@ static const AVClass imgutils_class = {
> >  .parent_log_context_offset = offsetof(ImgUtils, log_ctx),
> >  };
> >  
> > -int av_image_check_size(unsigned int w, unsigned int h, int log_offset, 
> > void *log_ctx)
> > +int av_image_check_size2(unsigned int w, unsigned int h, int64_t 
> > max_pixels, enum AVPixelFormat pix_fmt, int log_offset, void *log_ctx)
> >  {
> >  ImgUtils imgutils = {
> >  .class  = &imgutils_class,
> > @@ -256,11 +257,31 @@ int av_image_check_size(unsigned int w, unsigned int 
> > h, int log_offset, void *lo
> >  .log_ctx= log_ctx,
> >  };
> >  
> > -if ((int)w>0 && (int)h>0 && (w+128)*(uint64_t)(h+128) < INT_MAX/8)
> > -return 0;
> > +if ((int)w<=0 || (int)h<=0 || (w+128)*(uint64_t)(h+128) >= INT_MAX/8) {
> > +av_log(&imgutils, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", 
> > w, h);
> > +return AVERROR(EINVAL);
> > +}
> >  
> > -av_log(&imgutils, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, 
> > h);
> > -return AVERROR(EINVAL);
> > +if (max_pixels < INT64_MAX) {
> > +if (w*(int64_t)h > max_pixels) {
> > +av_log(&imgutils, AV_LOG_ERROR,
> > +"Picture size %ux%u exceeds specified max pixel count 
> > %"PRId64"\n",
> > +w, h, max_pixels);
> > +return AVERROR(EINVAL);
> > +}
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +int av_image_check_size(unsigned int w, unsigned int h, int log_offset, 
> > void *log_ctx)
> > +{
> > +int64_t max = INT64_MAX;
> > +
> > +if (log_ctx)
> > +av_opt_get_int(log_ctx, "max_pixels",  AV_OPT_SEARCH_CHILDREN, 
> > &max);
> > +
> > +return av_image_check_size2(w, h, max, AV_PIX_FMT_NONE, log_offset, 
> > log_ctx);
> >  }
> 
> Still against implicitly using an AVOption. Explicitly passing limits is
> better. Also, while your suggestion is convenient, it doesn't guarantee
> that ever caller will pass the correct context to it (i.e. an
> AVCodecContext that ha

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Ronald S. Bultje]

Hi,

On Sat, Dec 10, 2016 at 1:53 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 10.12.2016 19:49, James Almer wrote:
> > On 12/10/2016 3:47 PM, Andreas Cadhalpun wrote:
> >> Suggested-by: Carl Eugen Hoyos 
> >> Signed-off-by: Andreas Cadhalpun 
> >> ---
> >>  src/index | 7 +++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/src/index b/src/index
> >> index c203676..8f533f5 100644
> >> --- a/src/index
> >> +++ b/src/index
> >> @@ -37,6 +37,13 @@
> >>  News
> >>
> >>
> >> +  December 10th, 2016, stricter configure
> behavior.
> >> +  
> >> +The configure script now fails if autodetect-libraries are
> requested but not found.
> >> +  
> >> +  
> >> +If for example SDL headers are not available, '--enable-sdl2' will
> cause a configure failure.
> >> +  
> >>October 30th, 2016, Results: Summer Of
> Code 2016.
> >>
> >>  This has been a long time coming but we wanted to give a proper
> closure to our participation in this run of the program and it takes time.
> Sometimes it's just to get the final report for each project trimmed down,
> others, is finalizing whatever was still in progress when the program
> finished: final patches need to be merged, TODO lists stabilized, future
> plans agreed; you name it.
> >>
> >
> > This is a Changelog kind of line, not a news entry.
> >
> > It could maybe be added to the news entry announcing the next release,
> > but it's not worth alone on its own.
>
> I don't mind either way, so discuss this with Carl Eugen.


What makes Carl Eugen so special when it comes to posting news entries?

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffplay: fix sws_scale possible out of bounds array access

[Michael Niedermayer]

On Sat, Dec 10, 2016 at 01:06:59PM +0100, Marton Balint wrote:
> As I used simple RGBA formats for subtitles and for the video texture if
> avfilter is disabled I kind of assumed that sws_scale won't access data
> pointers and strides above index 0, but apparently that is not the case.
> 
> Fixes Coverity CID 1396737, 1396738, 1396739, 1396740.
> 
> Signed-off-by: Marton Balint 
> ---
>  ffplay.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)

LGTM

thx

also please backport to the releases

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Michael Niedermayer]

On Fri, Dec 09, 2016 at 06:56:53AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Thu, Dec 8, 2016 at 7:03 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
> 
> > On 08.12.2016 22:59, Carl Eugen Hoyos wrote:
> > > 2016-12-08 18:37 GMT+01:00 Michael Niedermayer :
> > >
> > >> -{"max_streams", "maximum number of streams", OFFSET(max_streams),
> > AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D },
> > >> +{"max_streams", "maximum number of streams", OFFSET(max_streams),
> > AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D },
> > >
> > > I wanted to suggest 1000 which is still a magnitude less than the
> > provided
> > > crashing sample but 255 also sounds ok to me.
> >
> > Either value is OK. The important thing is that it's several orders of
> > magnitude lower than INT_MAX.
> 
> 
> On IRC, we discussed at what values OOM start occurring, which seems to be
> around 30k-60k, so from there I suggested a value like 10k or 5k. 1000
> seems a little low but I think I can live with it (I doubt ATM I can come
> up with legit use cases that use 1000 streams).
> 

> If people hit the limit (whatever value we choose), I would propose that we
> make the error message very specific, something similar to
> AVERROR_PATCHWELCOME. This way, people understand this is not a hard
> limitation and can be changed easily; fuzzers will obviously ignore this
> message.

new patchset with higher limit, error messsage and reference to the
CVE# posted

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] avformat/utils: Print verbose error message if stream count exceeds max_streams

[Michael Niedermayer]

Signed-off-by: Michael Niedermayer 
---
 libavformat/utils.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 9e979a7c79..897352924d 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -4217,8 +4217,11 @@ AVStream *avformat_new_stream(AVFormatContext *s, const 
AVCodec *c)
 int i;
 AVStream **streams;
 
-if (s->nb_streams >= FFMIN(s->max_streams, INT_MAX/sizeof(*streams)))
+if (s->nb_streams >= FFMIN(s->max_streams, INT_MAX/sizeof(*streams))) {
+if (s->max_streams < INT_MAX/sizeof(*streams))
+av_log(s, AV_LOG_ERROR, "Number of streams exceeds max_streams 
parameter (%d), see the documentation if you wish to increase it\n", 
s->max_streams);
 return NULL;
+}
 streams = av_realloc_array(s->streams, s->nb_streams + 1, 
sizeof(*streams));
 if (!streams)
 return NULL;
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] avformat/options_table: Set the default maximum number of streams to 1000

[Michael Niedermayer]

Fixes CVE-2016-9561

Suggested-by: Andreas Cadhalpun 
Signed-off-by: Michael Niedermayer 
---
 libavformat/options_table.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/options_table.h b/libavformat/options_table.h
index d5448e503f..a537dda95e 100644
--- a/libavformat/options_table.h
+++ b/libavformat/options_table.h
@@ -105,7 +105,7 @@ static const AVOption avformat_options[] = {
 {"format_whitelist", "List of demuxers that are allowed to be used", 
OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
CHAR_MAX, D },
 {"protocol_whitelist", "List of protocols that are allowed to be used", 
OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
CHAR_MAX, D },
 {"protocol_blacklist", "List of protocols that are not allowed to be used", 
OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
CHAR_MAX, D },
-{"max_streams", "maximum number of streams", OFFSET(max_streams), 
AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D },
+{"max_streams", "maximum number of streams", OFFSET(max_streams), 
AV_OPT_TYPE_INT, { .i64 = 1000 }, 0, INT_MAX, D },
 {NULL},
 };
 
-- 
2.11.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Andreas Cadhalpun]

On 10.12.2016 19:49, James Almer wrote:
> On 12/10/2016 3:47 PM, Andreas Cadhalpun wrote:
>> Suggested-by: Carl Eugen Hoyos 
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  src/index | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/src/index b/src/index
>> index c203676..8f533f5 100644
>> --- a/src/index
>> +++ b/src/index
>> @@ -37,6 +37,13 @@
>>  News
>>
>>  
>> +  December 10th, 2016, stricter configure 
>> behavior.
>> +  
>> +The configure script now fails if autodetect-libraries are requested 
>> but not found.
>> +  
>> +  
>> +If for example SDL headers are not available, '--enable-sdl2' will 
>> cause a configure failure.
>> +  
>>October 30th, 2016, Results: Summer Of Code 
>> 2016.
>>
>>  This has been a long time coming but we wanted to give a proper closure 
>> to our participation in this run of the program and it takes time. Sometimes 
>> it's just to get the final report for each project trimmed down, others, is 
>> finalizing whatever was still in progress when the program finished: final 
>> patches need to be merged, TODO lists stabilized, future plans agreed; you 
>> name it.
>>
> 
> This is a Changelog kind of line, not a news entry.
> 
> It could maybe be added to the news entry announcing the next release,
> but it's not worth alone on its own.

I don't mind either way, so discuss this with Carl Eugen.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[James Almer]

On 12/10/2016 3:47 PM, Andreas Cadhalpun wrote:
> Suggested-by: Carl Eugen Hoyos 
> Signed-off-by: Andreas Cadhalpun 
> ---
>  src/index | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/index b/src/index
> index c203676..8f533f5 100644
> --- a/src/index
> +++ b/src/index
> @@ -37,6 +37,13 @@
>  News
>
>  
> +  December 10th, 2016, stricter configure 
> behavior.
> +  
> +The configure script now fails if autodetect-libraries are requested but 
> not found.
> +  
> +  
> +If for example SDL headers are not available, '--enable-sdl2' will cause 
> a configure failure.
> +  
>October 30th, 2016, Results: Summer Of Code 
> 2016.
>
>  This has been a long time coming but we wanted to give a proper closure 
> to our participation in this run of the program and it takes time. Sometimes 
> it's just to get the final report for each project trimmed down, others, is 
> finalizing whatever was still in progress when the program finished: final 
> patches need to be merged, TODO lists stabilized, future plans agreed; you 
> name it.
> 

This is a Changelog kind of line, not a news entry.

It could maybe be added to the news entry announcing the next release,
but it's not worth alone on its own.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] news: add entry about stricter configure behavior

[Andreas Cadhalpun]

Suggested-by: Carl Eugen Hoyos 
Signed-off-by: Andreas Cadhalpun 
---
 src/index | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/index b/src/index
index c203676..8f533f5 100644
--- a/src/index
+++ b/src/index
@@ -37,6 +37,13 @@
 News
   
 
+  December 10th, 2016, stricter configure 
behavior.
+  
+The configure script now fails if autodetect-libraries are requested but 
not found.
+  
+  
+If for example SDL headers are not available, '--enable-sdl2' will cause a 
configure failure.
+  
   October 30th, 2016, Results: Summer Of Code 
2016.
   
 This has been a long time coming but we wanted to give a proper closure to 
our participation in this run of the program and it takes time. Sometimes it's 
just to get the final report for each project trimmed down, others, is 
finalizing whatever was still in progress when the program finished: final 
patches need to be merged, TODO lists stabilized, future plans agreed; you name 
it.
-- 
2.10.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/3] configure: fail if autodetect-libraries are requested but not found

[Andreas Cadhalpun]

On 03.12.2016 01:25, Michael Niedermayer wrote:
> On Fri, Dec 02, 2016 at 11:24:12PM +0100, Andreas Cadhalpun wrote:
>> On 02.12.2016 19:20, Michael Niedermayer wrote:
>>> iam ok with the patch if other are and its tested
>>
>> I've tested for all AUTODETECT_LIBS that enabling the when configure
>> doesn't find them triggers a configure failure after the patch.
>> I've also check for a few libs that all combinations of --enable,
>> --disable, no configure flag and library installed or not does the
>> right thing.
>>
> 
>> Do you think there is something else that should be tested?
> 
> i dont know, maybe give others a day to comment before pushing
> in case someone has an idea

After a week without further comments I've pushed this now.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Andreas Cadhalpun]

On 10.12.2016 18:40, Hendrik Leppkes wrote:
> On Sat, Dec 10, 2016 at 6:20 PM, Andreas Cadhalpun
>  wrote:
>> On 10.12.2016 17:29, Hendrik Leppkes wrote:
>>> On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
>>>  wrote:
 If that is done, the hard limit in av_image_check_size should check for
 the maximum allowed value of any pixel format.
 But a check here is needed to make sure that width * height doesn't 
 overflow.
>>>
>>> Why is that needed?
>>
>> Because the framework currently doesn't support larger sizes and setting
>> options to invalid values doesn't make much sense.
>>
>>> Also, overflow what range exactly? int64?
>>
>> The range of valid image dimension, which is what av_image_check_size
>> is documented to check.
>>
>>> The generic option code should not make any assumptions how the data is
>>> going to be used. As long as its not multiplied right here and now,
>>> there is no reason to check.
>>
>> It's a valid assumption that an option of type AV_OPT_TYPE_IMAGE_SIZE
>> is used as image size, so it shouldn't accept values that are invalid
>> dimensions in our framework. Also it already doesn't accept negative
>> values. Would you prefer to remove this check?
> 
> Negative size is inherently invalid for image sizes, and not an
> arbitrary limit, so that argument makes no sense.

However, some file formats encode image sizes as negative values to
give them a special meaning like reversed image buffer. So it's not
entirely hypothetical to set height to a negative value.
(The current ffmpeg code does this and only later uses FFABS.)

>>> As said in an earlier mail, the check doesn't negate the need to check
>>> in other places, because AVOption is only one way to set values, so I
>>> would rather prefer avoiding adding more artificial limits in very
>>> generic code.
>>
>> The alternative is that every program setting the image size needs to
>> check if it is valid before setting the option. That's quite inconvenient.
> 
> No, the point is that everything that _uses_ these values needs to
> check it either way, so adding checks here doesn't seem to improve
> anything

The improvement is that width/height are at no point set to invalid values.

> and just adds extra burden when these limits are
> changed/improved (say by making them pixfmt aware as discussed in
> another thread, which this function could never know).

There is no extra burden, because av_image_check_size would have to
be changed in that case anyway to accept the largest value valid
for any pixel format.

> What exactly is this check supposed to fix/improve? Since all
> libraries need to check it anyway and would error then, littering
> earlier code paths with extra checks seems to not help much.

In general, values should be checked for validity when they are set.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Hendrik Leppkes]

On Sat, Dec 10, 2016 at 6:20 PM, Andreas Cadhalpun
 wrote:
> On 10.12.2016 17:29, Hendrik Leppkes wrote:
>> On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
>>  wrote:
>>> If that is done, the hard limit in av_image_check_size should check for
>>> the maximum allowed value of any pixel format.
>>> But a check here is needed to make sure that width * height doesn't 
>>> overflow.
>>
>> Why is that needed?
>
> Because the framework currently doesn't support larger sizes and setting
> options to invalid values doesn't make much sense.
>
>> Also, overflow what range exactly? int64?
>
> The range of valid image dimension, which is what av_image_check_size
> is documented to check.
>
>> The generic option code should not make any assumptions how the data is
>> going to be used. As long as its not multiplied right here and now,
>> there is no reason to check.
>
> It's a valid assumption that an option of type AV_OPT_TYPE_IMAGE_SIZE
> is used as image size, so it shouldn't accept values that are invalid
> dimensions in our framework. Also it already doesn't accept negative
> values. Would you prefer to remove this check?

Negative size is inherently invalid for image sizes, and not an
arbitrary limit, so that argument makes no sense.

>
>> As said in an earlier mail, the check doesn't negate the need to check
>> in other places, because AVOption is only one way to set values, so I
>> would rather prefer avoiding adding more artificial limits in very
>> generic code.
>
> The alternative is that every program setting the image size needs to
> check if it is valid before setting the option. That's quite inconvenient.

No, the point is that everything that _uses_ these values needs to
check it either way, so adding checks here doesn't seem to improve
anything and just adds extra burden when these limits are
changed/improved (say by making them pixfmt aware as discussed in
another thread, which this function could never know).

What exactly is this check supposed to fix/improve? Since all
libraries need to check it anyway and would error then, littering
earlier code paths with extra checks seems to not help much.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Andreas Cadhalpun]

On 10.12.2016 17:29, Hendrik Leppkes wrote:
> On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
>  wrote:
>> If that is done, the hard limit in av_image_check_size should check for
>> the maximum allowed value of any pixel format.
>> But a check here is needed to make sure that width * height doesn't overflow.
> 
> Why is that needed?

Because the framework currently doesn't support larger sizes and setting
options to invalid values doesn't make much sense.

> Also, overflow what range exactly? int64?

The range of valid image dimension, which is what av_image_check_size
is documented to check.

> The generic option code should not make any assumptions how the data is
> going to be used. As long as its not multiplied right here and now,
> there is no reason to check.

It's a valid assumption that an option of type AV_OPT_TYPE_IMAGE_SIZE
is used as image size, so it shouldn't accept values that are invalid
dimensions in our framework. Also it already doesn't accept negative
values. Would you prefer to remove this check?

> As said in an earlier mail, the check doesn't negate the need to check
> in other places, because AVOption is only one way to set values, so I
> would rather prefer avoiding adding more artificial limits in very
> generic code.

The alternative is that every program setting the image size needs to
check if it is valid before setting the option. That's quite inconvenient.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf: add ffprobe demuxer

[Stefano Sabatini]

On date Sunday 2016-12-04 22:54:21 +0100, Andreas Cadhalpun encoded:
> On 31.10.2016 09:51, Stefano Sabatini wrote:
> > From 7f209e27aa33e8f43444e5cfc44c68f664b69e06 Mon Sep 17 00:00:00 2001
> > From: Nicolas George 
> > Date: Sat, 11 Jan 2014 19:42:41 +0100
> > Subject: [PATCH] lavf: add ffprobe demuxer
> > 
> > With several modifications and documentation by Stefano Sabatini
> > .
> > 
> > Signed-off-by: Nicolas George 
> > ---
> >  configure|   3 +
> >  doc/demuxers.texi|  18 ++
> >  doc/ffprobe-format.texi  | 121 +
> >  doc/formats.texi |   1 +
> >  libavformat/Makefile |   1 +
> >  libavformat/allformats.c |   1 +
> >  libavformat/ffprobedec.c | 439 
> > +++
> >  7 files changed, 584 insertions(+)
> >  create mode 100644 doc/ffprobe-format.texi
> >  create mode 100644 libavformat/ffprobedec.c
> > 
> > diff --git a/configure b/configure
> > index 72ffaea..71b9d73 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3349,6 +3349,9 @@ for n in $COMPONENT_LIST; do
> >  eval ${n}_if_any="\$$v"
> >  done
> >  
> > +# Disable ffprobe demuxer for security concerns
> > +disable ffprobe_demuxer
> > +
> 
> As I already wrote elsewhere, I don't think disabling this by default is good,
> as it will likely cause it to bitrot. Better require '-strict experimental'.
> 
> >  enable $ARCH_EXT_LIST
> >  
> >  die_unknown(){
> > diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> > index 2934a1c..0e6dfbd 100644
> > --- a/doc/demuxers.texi
> > +++ b/doc/demuxers.texi
> > @@ -243,6 +243,24 @@ file subdir/file-2.wav
> >  @end example
> >  @end itemize
> >  
> > +@section ffprobe
> > +
> > +ffprobe internal demuxer. It is able to demux files in the format
> > +specified in the @ref{FFprobe demuxer format} chapter.
> > +
> > +For security reasons this demuxer is disabled by default, should be
> > +enabled though the @code{--enable-demuxer=ffprobe} configure option.
> > +
> 
> In that case the documentation should mention '-strict experimental'
> instead of this.
> 
> > diff --git a/libavformat/ffprobedec.c b/libavformat/ffprobedec.c
> > new file mode 100644
> > index 000..0bc9a49
> > --- /dev/null
> > +++ b/libavformat/ffprobedec.c
> [...]
> > +static int read_section_stream(AVFormatContext *avf)
> > +{
> > +FFprobeContext *ffp = avf->priv_data;
> > +uint8_t buf[LINE_BUFFER_SIZE];
> > +int ret, index, val1, val2;
> > +AVStream *st = NULL;
> > +const char *val;
> > +
> > +av_bprint_clear(&ffp->data);
> > +while ((ret = read_section_line(avf, buf, sizeof(buf {
> > +if (ret < 0)
> > +return ret;
> > +if (!st) {
> > +if (sscanf(buf, "index=%d", &index) >= 1) {
> > +if (index == avf->nb_streams) {
> > +if (!avformat_new_stream(avf, NULL))
> > +return AVERROR(ENOMEM);
> > +}
> > +if ((unsigned)index >= avf->nb_streams) {
> > +av_log(avf, AV_LOG_ERROR, "Invalid stream index: %d\n",
> > +   index);
> > +return AVERROR_INVALIDDATA;
> > +}
> > +st = avf->streams[index];
> > +} else {
> > +av_log(avf, AV_LOG_ERROR, "Stream without index\n");
> > +return AVERROR_INVALIDDATA;
> > +}
> > +}
> > +if (av_strstart(buf, "codec_name=", &val)) {
> > +const AVCodecDescriptor *desc = 
> > avcodec_descriptor_get_by_name(val);
> > +if (desc) {
> > +st->codecpar->codec_id   = desc->id;
> > +st->codecpar->codec_type = desc->type;
> > +}
> > +if (!desc) {
> > +av_log(avf, AV_LOG_WARNING, "Cannot recognize codec name 
> > '%s'", val);
> > +}
> > +} else if (!strcmp(buf, "extradata=")) {
> > +if ((ret = read_data(avf, NULL)) < 0)
> > +return ret;
> > +if (ffp->data.len) {
> 
> This can leak st->codecpar->extradata and thus needs:
>av_freep(&st->codecpar->extradata);
> 
> > +if ((ret = ff_alloc_extradata(st->codecpar, 
> > ffp->data.len)) < 0)
> > +return ret;
> > +memcpy(st->codecpar->extradata, ffp->data.str, 
> > ffp->data.len);
> > +}
> > +} else if (sscanf(buf, "time_base=%d/%d", &val1, &val2) >= 2) {
> > +st->time_base.num = val1;
> > +st->time_base.den = val2;
> > +if (st->time_base.num <= 0 || st->time_base.den <= 0) {
> 
> This should check val1 and val2 and only afterwards set st->time_base.
> Otherwise the check doesn't have the desired effect.

No, this doesn't make any difference but changed anyway as it reduces
the character count.
 
> > +av_log(avf, AV_LOG_ERROR, "Invalid time base %d/%d\n",
> > +   

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Hendrik Leppkes]

On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
 wrote:
> On 10.12.2016 16:26, wm4 wrote:
>> On Sat, 10 Dec 2016 16:11:15 +0100
>> Andreas Cadhalpun  wrote:
>>
>>> Signed-off-by: Andreas Cadhalpun 
>>> ---
>>>  libavutil/opt.c | 10 +-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index f855ccb..f713d3f 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -32,6 +32,7 @@
>>>  #include "common.h"
>>>  #include "dict.h"
>>>  #include "eval.h"
>>> +#include "imgutils.h"
>>>  #include "log.h"
>>>  #include "parseutils.h"
>>>  #include "pixdesc.h"
>>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const 
>>> AVOption *o, const char *val,
>>>  return 0;
>>>  }
>>>  ret = av_parse_video_size(dst, dst + 1, val);
>>> -if (ret < 0)
>>> +if (ret < 0) {
>>>  av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as 
>>> image size\n", val);
>>> +return ret;
>>> +}
>>> +ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>>> +if (ret < 0) {
>>> +*dst = 0;
>>> +*(dst + 1) = 0;
>>> +}
>>>  return ret;
>>>  }
>>>
>>
>> I'd argue that rather than doing this, image allocation functions etc.
>> should error out if the dimensions are too large.
>
> That can be done in addition to this.
>
>> This way the allowed dimensions could be enlarged (e.g. by taking the
>> pixel format into account) without changing everything from int to
>> size_t.
>
> If that is done, the hard limit in av_image_check_size should check for
> the maximum allowed value of any pixel format.
> But a check here is needed to make sure that width * height doesn't overflow.

Why is that needed? Also, overflow what range exactly? int64? The
generic option code should not make any assumptions how the data is
going to be used. As long as its not multiplied right here and now,
there is no reason to check.
As said in an earlier mail, the check doesn't negate the need to check
in other places, because AVOption is only one way to set values, so I
would rather prefer avoiding adding more artificial limits in very
generic code.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Carl Eugen Hoyos]

2016-12-10 16:55 GMT+01:00 Andreas Cadhalpun :
> On 10.12.2016 16:19, Carl Eugen Hoyos wrote:
>> 2016-12-10 16:11 GMT+01:00 Andreas Cadhalpun 
>> :
>>> Signed-off-by: Andreas Cadhalpun 
>>> ---
>>>  libavutil/opt.c | 10 +-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index f855ccb..f713d3f 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -32,6 +32,7 @@
>>>  #include "common.h"
>>>  #include "dict.h"
>>>  #include "eval.h"
>>> +#include "imgutils.h"
>>>  #include "log.h"
>>>  #include "parseutils.h"
>>>  #include "pixdesc.h"
>>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const 
>>> AVOption *o, const char *val,
>>>  return 0;
>>>  }
>>>  ret = av_parse_video_size(dst, dst + 1, val);
>>> -if (ret < 0)
>>> +if (ret < 0) {
>>>  av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as 
>>> image size\n", val);
>>> +return ret;
>>> +}
>>> +ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>>> +if (ret < 0) {
>>> +*dst = 0;
>>> +*(dst + 1) = 0;
>>> +}
>>
>> Doesn't this break valid usecases?
>
> None that I'm aware of. Which usecases were you thinking about?

I thought that if this check is more strict than it was, there must
be a usecase that breaks but I wasn't immediately successful
in finding one.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Carl Eugen Hoyos]

2016-12-10 16:54 GMT+01:00 Hendrik Leppkes :

> Allocations would need to be checked anyway with or without this check

Do you think this is related?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Andreas Cadhalpun]

On 10.12.2016 16:26, wm4 wrote:
> On Sat, 10 Dec 2016 16:11:15 +0100
> Andreas Cadhalpun  wrote:
> 
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavutil/opt.c | 10 +-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>> index f855ccb..f713d3f 100644
>> --- a/libavutil/opt.c
>> +++ b/libavutil/opt.c
>> @@ -32,6 +32,7 @@
>>  #include "common.h"
>>  #include "dict.h"
>>  #include "eval.h"
>> +#include "imgutils.h"
>>  #include "log.h"
>>  #include "parseutils.h"
>>  #include "pixdesc.h"
>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const 
>> AVOption *o, const char *val,
>>  return 0;
>>  }
>>  ret = av_parse_video_size(dst, dst + 1, val);
>> -if (ret < 0)
>> +if (ret < 0) {
>>  av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as 
>> image size\n", val);
>> +return ret;
>> +}
>> +ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>> +if (ret < 0) {
>> +*dst = 0;
>> +*(dst + 1) = 0;
>> +}
>>  return ret;
>>  }
>>  
> 
> I'd argue that rather than doing this, image allocation functions etc.
> should error out if the dimensions are too large.

That can be done in addition to this.

> This way the allowed dimensions could be enlarged (e.g. by taking the
> pixel format into account) without changing everything from int to
> size_t.

If that is done, the hard limit in av_image_check_size should check for
the maximum allowed value of any pixel format.
But a check here is needed to make sure that width * height doesn't overflow.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Andreas Cadhalpun]

On 10.12.2016 16:19, Carl Eugen Hoyos wrote:
> 2016-12-10 16:11 GMT+01:00 Andreas Cadhalpun 
> :
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavutil/opt.c | 10 +-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>> index f855ccb..f713d3f 100644
>> --- a/libavutil/opt.c
>> +++ b/libavutil/opt.c
>> @@ -32,6 +32,7 @@
>>  #include "common.h"
>>  #include "dict.h"
>>  #include "eval.h"
>> +#include "imgutils.h"
>>  #include "log.h"
>>  #include "parseutils.h"
>>  #include "pixdesc.h"
>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const 
>> AVOption *o, const char *val,
>>  return 0;
>>  }
>>  ret = av_parse_video_size(dst, dst + 1, val);
>> -if (ret < 0)
>> +if (ret < 0) {
>>  av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as 
>> image size\n", val);
>> +return ret;
>> +}
>> +ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>> +if (ret < 0) {
>> +*dst = 0;
>> +*(dst + 1) = 0;
>> +}
> 
> Doesn't this break valid usecases?

None that I'm aware of. Which usecases were you thinking about?

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Hendrik Leppkes]

On Sat, Dec 10, 2016 at 4:26 PM, wm4  wrote:
> On Sat, 10 Dec 2016 16:11:15 +0100
> Andreas Cadhalpun  wrote:
>
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavutil/opt.c | 10 +-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>> index f855ccb..f713d3f 100644
>> --- a/libavutil/opt.c
>> +++ b/libavutil/opt.c
>> @@ -32,6 +32,7 @@
>>  #include "common.h"
>>  #include "dict.h"
>>  #include "eval.h"
>> +#include "imgutils.h"
>>  #include "log.h"
>>  #include "parseutils.h"
>>  #include "pixdesc.h"
>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const 
>> AVOption *o, const char *val,
>>  return 0;
>>  }
>>  ret = av_parse_video_size(dst, dst + 1, val);
>> -if (ret < 0)
>> +if (ret < 0) {
>>  av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as 
>> image size\n", val);
>> +return ret;
>> +}
>> +ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
>> +if (ret < 0) {
>> +*dst = 0;
>> +*(dst + 1) = 0;
>> +}
>>  return ret;
>>  }
>>
>
> I'd argue that rather than doing this, image allocation functions etc.
> should error out if the dimensions are too large.
>
> This way the allowed dimensions could be enlarged (e.g. by taking the
> pixel format into account) without changing everything from int to
> size_t.

I agree, av_image_check_size has some arbitrary technical limits in it
right now that may not necessarily apply everywhere you want to set an
image size.
Allocations would need to be checked anyway with or without this check
because most options accessible through AVOptions are also accessible
through other means, so I would rather leave the decision of the
maximum size up to the code that uses these values.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH]lavd/v4l2: Do not set frame_set to a negative value

[Carl Eugen Hoyos]

Hi!

Attached patch fixes a minor bug, displaying a negative bitrate.

Please comment, Carl Eugen
From e2e4999c9e91fb2f6bcc782fdfe7a2f5d73d7600 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos 
Date: Sat, 10 Dec 2016 16:43:00 +0100
Subject: [PATCH] lavd/v4l2: Avoid setting frame_size to a negative value.

---
 libavdevice/v4l2.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
index ae51d83..b57909bd 100644
--- a/libavdevice/v4l2.c
+++ b/libavdevice/v4l2.c
@@ -936,6 +936,7 @@ static int v4l2_read_header(AVFormatContext *ctx)
 goto fail;
 
 st->codecpar->format = ff_fmt_v4l2ff(desired_format, codec_id);
+if (st->codecpar->format != AV_PIX_FMT_NONE)
 s->frame_size = av_image_get_buffer_size(st->codecpar->format,
  s->width, s->height, 1);
 
-- 
1.7.10.4

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[Carl Eugen Hoyos]

2016-12-10 16:11 GMT+01:00 Andreas Cadhalpun :
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavutil/opt.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index f855ccb..f713d3f 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -32,6 +32,7 @@
>  #include "common.h"
>  #include "dict.h"
>  #include "eval.h"
> +#include "imgutils.h"
>  #include "log.h"
>  #include "parseutils.h"
>  #include "pixdesc.h"
> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const 
> AVOption *o, const char *val,
>  return 0;
>  }
>  ret = av_parse_video_size(dst, dst + 1, val);
> -if (ret < 0)
> +if (ret < 0) {
>  av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as 
> image size\n", val);
> +return ret;
> +}
> +ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
> +if (ret < 0) {
> +*dst = 0;
> +*(dst + 1) = 0;
> +}

Doesn't this break valid usecases?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] opt: check image size when setting it

[wm4]

On Sat, 10 Dec 2016 16:11:15 +0100
Andreas Cadhalpun  wrote:

> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavutil/opt.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index f855ccb..f713d3f 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -32,6 +32,7 @@
>  #include "common.h"
>  #include "dict.h"
>  #include "eval.h"
> +#include "imgutils.h"
>  #include "log.h"
>  #include "parseutils.h"
>  #include "pixdesc.h"
> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const 
> AVOption *o, const char *val,
>  return 0;
>  }
>  ret = av_parse_video_size(dst, dst + 1, val);
> -if (ret < 0)
> +if (ret < 0) {
>  av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as 
> image size\n", val);
> +return ret;
> +}
> +ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
> +if (ret < 0) {
> +*dst = 0;
> +*(dst + 1) = 0;
> +}
>  return ret;
>  }
>  

I'd argue that rather than doing this, image allocation functions etc.
should error out if the dimensions are too large.

This way the allowed dimensions could be enlarged (e.g. by taking the
pixel format into account) without changing everything from int to
size_t.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] opt: check image size when setting it

[Andreas Cadhalpun]

Signed-off-by: Andreas Cadhalpun 
---
 libavutil/opt.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/libavutil/opt.c b/libavutil/opt.c
index f855ccb..f713d3f 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -32,6 +32,7 @@
 #include "common.h"
 #include "dict.h"
 #include "eval.h"
+#include "imgutils.h"
 #include "log.h"
 #include "parseutils.h"
 #include "pixdesc.h"
@@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption 
*o, const char *val,
 return 0;
 }
 ret = av_parse_video_size(dst, dst + 1, val);
-if (ret < 0)
+if (ret < 0) {
 av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as 
image size\n", val);
+return ret;
+}
+ret = av_image_check_size(*dst, *(dst + 1), 0, obj);
+if (ret < 0) {
+*dst = 0;
+*(dst + 1) = 0;
+}
 return ret;
 }
 
-- 
2.10.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Michael Niedermayer]

On Sat, Dec 10, 2016 at 12:43:49PM +0100, Carl Eugen Hoyos wrote:
> 2016-12-10 12:13 GMT+01:00 wm4 :
> > On Sat, 10 Dec 2016 11:51:04 +0100
> > Carl Eugen Hoyos  wrote:
> >
> >> 2016-12-09 19:45 GMT+01:00 wm4 :
> >>
> >> > But users switching from FFmpeg to other software because
> >> > it fails at the limit does happen.
> >>
> >> Could you elaborate?
> >> Was there a bug report that we ignored?
> >
> > Reporting bugs to ffmpeg is so tedious, why would anyone do that?
> 
> We have a unreasonable large amount of bug reports that are
> unlikely to get fixed (by purely technical reasons), I wonder

Somewhat off topic but if we want more bugs to be fixed than what
volunteers are doing then its needed to
either motivate more volunteers somehow or
1. get more sponsors and a real budget (like 100k per year)
2. put bounties on most tickets

I would expect that if one can live of fixing ffmpeg bugs full time,
someone would probably do exactly that

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] movenc: Tag files generated with strict experimental with a warning

[Carl Eugen Hoyos]

2016-12-02 21:00 GMT+01:00 Vittorio Giovara :
> This will simplify identifying files that were generated with
> unfinished/incomplete/non-standard specifications.

Doesn't the muxer version information already provide this information?
Are random metadata tags valid?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] vaapi_vp9: Do not set bit_depth on old versions

[Carl Eugen Hoyos]

2016-12-10 14:57 GMT+01:00 Mark Thompson :
> I've applied the second patch and closed the ticket.

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avutil/tests: run the cpu_init.c test conditionally on HAVE_THREADS

[Carl Eugen Hoyos]

2016-12-06 20:00 GMT+01:00 Wan-Teh Chang :
> Also declare the main() function with void arguments because argc and
> argv are unused.

Sounds like an unrelated change that should be part of an independent patch.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] vaapi_vp9: Do not set bit_depth on old versions

[Mark Thompson]

On 10/12/16 12:52, Carl Eugen Hoyos wrote:
> 2016-12-10 13:50 GMT+01:00 Mark Thompson :
>> Fixes ticket #6003.
>> ---
>> On 10/12/16 12:09, Carl Eugen Hoyos wrote:
>>> 2016-12-08 16:51 GMT+01:00 Mark Thompson :
 Fixes ticket #6003.
>>>
> May I suggest to revert this?
>
>> +pp->bit_depth = h->h.bpp;
>
> Google doesn't easily find a version of va_dec_vp9.h
> with _VADecPictureParameterBufferVP9->bit_depth.

 Urgh: libva 1.6.0 to 1.6.2 don't have this field.

 Hacky fix enclosing, only compile tested.
>>>
>>> As long as you work on a "better" fix, please either
>>> apply or revert the original patch, build regressions
>>> are not acceptable.
>>
>> Please review the two suggested fixes and hopefully express
>> approval for one or other of them
> 
> No, no, no  - I mean: Not necessarily.
> 
> This is a build issue in a file that you maintain, you fix it
> the way you like.

I've applied the second patch and closed the ticket.

Thanks,

- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/3] avformat/udp: Use avutil compat pthread_cond_timedwait.

[Carl Eugen Hoyos]

2016-12-07 7:04 GMT+01:00 Matt Oliver :

> -#if HAVE_PTHREAD_CANCEL
> -#include 
> -#endif
> -

>  #ifndef HAVE_PTHREAD_CANCEL
>  #define HAVE_PTHREAD_CANCEL 0
>  #endif

Do you know what this block is supposed to do?
If you remove it first, the diff gets much smaller...

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Ronald S. Bultje]

Hi,

On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos 
wrote:

> 2016-12-09 12:56 GMT+01:00 Ronald S. Bultje :
>
> > On IRC, we discussed at what values OOM start occurring, which seems to
> be
> > around 30k-60k
>
> This is not true, why do you think so?


http://lists.ffmpeg.org/pipermail/ffmpeg-devel-irc/2016-December/003980.html
:
[21:01:43 CET]  michaelni: can you provide a graph with memory usage
depending on the number of streams?
[21:01:52 CET]  michaelni: I think based on that we can define a
sensible default
[21:02:25 CET]  if we use 100GB with 10 streams, we should probably
limit it to <10, but if memory usage doesn t change much between 10k and
100k streams, maybe a limit of 1M streams is acceptable
[21:02:35 CET]  michaelni: assuming the goal is to prevent OOM
scenarios
[21:11:41 CET]  i think one actual OOM case was with around
30k-60k streams, the exact usage likely depends on the demuxer

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avdevice/decklink_enc: do not reference this after freeing it

[Marton Balint]

Fixes Coverity CID 1396863.

Signed-off-by: Marton Balint 
---
 libavdevice/decklink_enc.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
index dc4a24b..ad00224 100644
--- a/libavdevice/decklink_enc.cpp
+++ b/libavdevice/decklink_enc.cpp
@@ -55,7 +55,7 @@ public:
 
 virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID iid, LPVOID *ppv) 
{ return E_NOINTERFACE; }
 virtual ULONG   STDMETHODCALLTYPE AddRef(void)
{ return ++_refs; }
-virtual ULONG   STDMETHODCALLTYPE Release(void)   
{ if (!--_refs) delete this; return _refs; }
+virtual ULONG   STDMETHODCALLTYPE Release(void)   
{ if (!--_refs) {delete this; return 0;} return _refs; }
 
 struct decklink_ctx *_ctx;
 AVFrame *_avframe;
-- 
2.10.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] vaapi_vp9: Do not set bit_depth on old versions

[Carl Eugen Hoyos]

2016-12-10 13:50 GMT+01:00 Mark Thompson :
> Fixes ticket #6003.
> ---
> On 10/12/16 12:09, Carl Eugen Hoyos wrote:
>> 2016-12-08 16:51 GMT+01:00 Mark Thompson :
>>> Fixes ticket #6003.
>>
 May I suggest to revert this?

> +pp->bit_depth = h->h.bpp;

 Google doesn't easily find a version of va_dec_vp9.h
 with _VADecPictureParameterBufferVP9->bit_depth.
>>>
>>> Urgh: libva 1.6.0 to 1.6.2 don't have this field.
>>>
>>> Hacky fix enclosing, only compile tested.
>>
>> As long as you work on a "better" fix, please either
>> apply or revert the original patch, build regressions
>> are not acceptable.
>
> Please review the two suggested fixes and hopefully express
> approval for one or other of them

No, no, no  - I mean: Not necessarily.

This is a build issue in a file that you maintain, you fix it
the way you like.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] vaapi_vp9: Do not set bit_depth on old versions

[Mark Thompson]

Fixes ticket #6003.
---
On 10/12/16 12:09, Carl Eugen Hoyos wrote:
> 2016-12-08 16:51 GMT+01:00 Mark Thompson :
>> Fixes ticket #6003.
> 
>>> May I suggest to revert this?
>>>
 +pp->bit_depth = h->h.bpp;
>>>
>>> Google doesn't easily find a version of va_dec_vp9.h
>>> with _VADecPictureParameterBufferVP9->bit_depth.
>>
>> Urgh: libva 1.6.0 to 1.6.2 don't have this field.
>>
>> Hacky fix enclosing, only compile tested.
> 
> As long as you work on a "better" fix, please either
> apply or revert the original patch, build regressions
> are not acceptable.

Please review the two suggested fixes and hopefully express approval for one or 
other of them - a fixed and tested version of the first is in this mail, the 
second can be found in 
.

Thanks,

- Mark


 libavcodec/vaapi_vp9.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/libavcodec/vaapi_vp9.c b/libavcodec/vaapi_vp9.c
index 9b3e81a..a355a99 100644
--- a/libavcodec/vaapi_vp9.c
+++ b/libavcodec/vaapi_vp9.c
@@ -38,7 +38,10 @@ static void fill_picture_parameters(AVCodecContext   
  *avctx,
 pp->first_partition_size = h->h.compressed_header_size;
 
 pp->profile = h->h.profile;
+
+#if VA_CHECK_VERSION(0, 39, 0)
 pp->bit_depth = h->h.bpp;
+#endif
 
 pp->filter_level = h->h.filter.level;
 pp->sharpness_level = h->h.filter.sharpness;
@@ -97,6 +100,14 @@ static int vaapi_vp9_start_frame(AVCodecContext  
*avctx,
 FFVAContext * const vactx = ff_vaapi_get_context(avctx);
 VADecPictureParameterBufferVP9 *pic_param;
 
+#if !VA_CHECK_VERSION(0, 39, 0)
+if (h->h.profile >= 2) {
+av_log(avctx, AV_LOG_ERROR, "VP9 profile %d is not supported "
+   "with this libva version.\n", h->h.profile);
+return AVERROR(ENOSYS);
+}
+#endif
+
 vactx->slice_param_size = sizeof(VASliceParameterBufferVP9);
 
 pic_param = ff_vaapi_alloc_pic_param(vactx, 
sizeof(VADecPictureParameterBufferVP9));
-- 
2.10.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] opus_parser: fix leaking channel_maps on error

[Andreas Cadhalpun]

On 10.12.2016 04:19, Michael Niedermayer wrote:
> On Fri, Dec 09, 2016 at 11:42:59PM +0100, Andreas Cadhalpun wrote:
>>  opus.c|1 +
>>  opusdec.c |1 -
>>  2 files changed, 1 insertion(+), 1 deletion(-)
>> 3e534ddbb39e05f8c7a5cfeba4b65ed587c3a519  
>> 0001-opus_parser-fix-leaking-channel_maps-on-error.patch
>> From 2edf5f9a571d483858928dd6bb9e84031ef8a391 Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun 
>> Date: Fri, 9 Dec 2016 00:00:18 +0100
>> Subject: [PATCH 1/2] opus_parser: fix leaking channel_maps on error
>>
>> Make ff_opus_parse_extradata free allocated memory on error instead of
>> expecting callers to free it in that case.
>>
>> Signed-off-by: Andreas Cadhalpun 
>> ---
>>  libavcodec/opus.c| 1 +
>>  libavcodec/opusdec.c | 1 -
>>  2 files 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

Re: [FFmpeg-devel] [PATCH] configure: Require bit_depth field for VAAPI VP9 decode hwaccel

[Carl Eugen Hoyos]

2016-12-08 17:26 GMT+01:00 Mark Thompson :
> libva versions from 1.6.0 to 1.6.2 do not include it, and therefore
> cannot work with VP9 profile >= 2.

(I missed this, sorry.)

As said, please commit any of these patches soon.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] opus_parser: replace ff_parse_close with opus_parse_close

[Andreas Cadhalpun]

On 10.12.2016 01:17, Hendrik Leppkes wrote:
> On Fri, Dec 9, 2016 at 11:46 PM, Andreas Cadhalpun
>  wrote:
>> On 09.12.2016 15:02, Hendrik Leppkes wrote:
>>> On Fri, Dec 9, 2016 at 12:09 AM, Andreas Cadhalpun
>>>  wrote:
 The former expects priv_data to be the ParseContext directly, so using
 it does not work.

>>>
>>> As an alternative re-order the OpusParseContext so that ParseContext
>>> comes first, it then would work, and thats basically how its done in
>>> the other parsers from what I can tell.
>>
>> Good idea, that makes the patch a bit shorter.
>>
> 
> LGTM.

Pushed.

Best regards,
Andreas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Carl Eugen Hoyos]

2016-12-10 13:33 GMT+01:00 Marton Balint :
>
> On Sat, 10 Dec 2016, Carl Eugen Hoyos wrote:
>
>> 2016-12-09 20:33 GMT+01:00 Marton Balint :
>>>
>>> If we still want a default limit, that limit should be IMHO
>>> _insanely_ high, tens of thousands, not hundreds.
>>
>> Do you have a file that has hundreds of streams?
>
> No I don't, but that does not mean that nobody has such a file,
> and we should not support it by default. Also I mentioned that
> corrupted sources can generate dummy streams.

I thought that I am normally on the user's (and broken stream's)
side (and I believe that 16k is not so unusual) but 100 streams
seem already unrealistic to me, 1000 even more so.

>> Note that I suggested 1000 as limit.
>
> Even 1000 does not seems so unthinkable to me, so I
> suggested tens of thousands.

Then we can leave the option at INT_MAX imo.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Andreas Cadhalpun]

On 09.12.2016 20:31, Michael Niedermayer wrote:
> Adds av_image_check_size2() with max_pixels and pix_fmt parameters.
> pix_fmt is unused and is added to avoid a 2nd API change later
> 
> The old function uses AVOptions to obtain the max_pixels value to simplify
> the transition.
> 
> TODO: split into 2 patches (one per lib), docs & bump
> 
> This allows preventing some OOM and "slow decoding" cases by limiting the 
> maximum resolution
> this may be useful to avoid fuzzers getting stuck in boring cases

Setting this option to a reasonably low value like 100 fixes a
significant amount of my slow samples. So having it is definitely useful.

> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/avcodec.h |  8 
>  libavcodec/options_table.h   |  1 +
>  libavutil/imgutils.c | 31 ++-
>  libavutil/imgutils.h | 14 ++
>  tests/ref/fate/api-mjpeg-codec-param |  2 ++
>  tests/ref/fate/api-png-codec-param   |  2 ++
>  6 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 7ac2adaf66..81052b10ef 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
>   */
>  int trailing_padding;
>  
> +/**
> + * The number of pixels per image to maximally accept.
> + *
> + * - decoding: set by user
> + * - encoding: unused

It should work for encoding as well, doesn't it?

> + */
> +int max_pixels;
> +
>  } AVCodecContext;
>  
>  AVRational av_codec_get_pkt_timebase (const AVCodecContext *avctx);
> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> index ee79859953..2f5eb252fe 100644
> --- a/libavcodec/options_table.h
> +++ b/libavcodec/options_table.h
> @@ -570,6 +570,7 @@ static const AVOption avcodec_options[] = {
>  {"codec_whitelist", "List of decoders that are allowed to be used", 
> OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
> CHAR_MAX, A|V|S|D },
>  {"pixel_format", "set pixel format", OFFSET(pix_fmt), AV_OPT_TYPE_PIXEL_FMT, 
> {.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
>  {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, 
> {.str=NULL}, 0, INT_MAX, 0 },
> +{"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), 
> AV_OPT_TYPE_INT, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D },

Setting the default to INT_MAX is a bit misleading, because the existing check
doesn't allow anything larger than 264241280.

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Marton Balint]

On Sat, 10 Dec 2016, Carl Eugen Hoyos wrote:


2016-12-09 20:33 GMT+01:00 Marton Balint :

If we still want a default limit, that limit should be IMHO
_insanely_ high, tens of thousands, not hundreds.


Do you have a file that has hundreds of streams?


No I don't, but that does not mean that nobody has such a file, and we 
should not support it by default. Also I mentioned that corrupted sources 
can generate dummy streams.



Note that I suggested 1000 as limit.


Even 1000 does not seems so unthinkable to me, so I suggested tens of thousands.

Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avdevice/decklink_dec: properly initialize no_video variable

[Marton Balint]

Fixes Coverity CID 1396859.

Signed-off-by: Marton Balint 
---
 libavdevice/decklink_dec.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
index c98c51f..7df841b 100644
--- a/libavdevice/decklink_dec.cpp
+++ b/libavdevice/decklink_dec.cpp
@@ -208,6 +208,7 @@ 
decklink_input_callback::decklink_input_callback(AVFormatContext *_avctx) : m_re
 avctx = _avctx;
 decklink_cctx   *cctx = (struct decklink_cctx *)avctx->priv_data;
 ctx = (struct decklink_ctx *)cctx->ctx;
+no_video = 0;
 initial_audio_pts = initial_video_pts = AV_NOPTS_VALUE;
 pthread_mutex_init(&m_mutex, NULL);
 }
-- 
2.10.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Carl Eugen Hoyos]

2016-12-09 20:33 GMT+01:00 Marton Balint :
> If we still want a default limit, that limit should be IMHO
> _insanely_ high, tens of thousands, not hundreds.

Do you have a file that has hundreds of streams?
Note that I suggested 1000 as limit.

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/options_table: Set the default maximum number of streams to 100

[Carl Eugen Hoyos]

2016-12-09 12:56 GMT+01:00 Ronald S. Bultje :

> On IRC, we discussed at what values OOM start occurring, which seems to be
> around 30k-60k

This is not true, why do you think so?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] vaapi_vp9: Do not set bit_depth on old versions

[Carl Eugen Hoyos]

2016-12-08 16:51 GMT+01:00 Mark Thompson :
> Fixes ticket #6003.

>> May I suggest to revert this?
>>
>>> +pp->bit_depth = h->h.bpp;
>>
>> Google doesn't easily find a version of va_dec_vp9.h
>> with _VADecPictureParameterBufferVP9->bit_depth.
>
> Urgh: libva 1.6.0 to 1.6.2 don't have this field.
>
> Hacky fix enclosing, only compile tested.

As long as you work on a "better" fix, please either
apply or revert the original patch, build regressions
are not acceptable.

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] ffplay: fix sws_scale possible out of bounds array access

[Marton Balint]

As I used simple RGBA formats for subtitles and for the video texture if
avfilter is disabled I kind of assumed that sws_scale won't access data
pointers and strides above index 0, but apparently that is not the case.

Fixes Coverity CID 1396737, 1396738, 1396739, 1396740.

Signed-off-by: Marton Balint 
---
 ffplay.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ffplay.c b/ffplay.c
index bb781a2..911fd7f 100644
--- a/ffplay.c
+++ b/ffplay.c
@@ -883,11 +883,11 @@ static int upload_texture(SDL_Texture *tex, AVFrame 
*frame, struct SwsContext **
 frame->width, frame->height, frame->format, frame->width, 
frame->height,
 AV_PIX_FMT_BGRA, sws_flags, NULL, NULL, NULL);
 if (*img_convert_ctx != NULL) {
-uint8_t *pixels;
-int pitch;
-if (!SDL_LockTexture(tex, NULL, (void **)&pixels, &pitch)) {
+uint8_t *pixels[4];
+int pitch[4];
+if (!SDL_LockTexture(tex, NULL, (void **)pixels, pitch)) {
 sws_scale(*img_convert_ctx, (const uint8_t * const 
*)frame->data, frame->linesize,
-  0, frame->height, &pixels, &pitch);
+  0, frame->height, pixels, pitch);
 SDL_UnlockTexture(tex);
 }
 } else {
@@ -913,8 +913,8 @@ static void video_image_display(VideoState *is)
 
 if (vp->pts >= sp->pts + ((float) sp->sub.start_display_time / 
1000)) {
 if (!sp->uploaded) {
-uint8_t *pixels;
-int pitch;
+uint8_t* pixels[4];
+int pitch[4];
 int i;
 if (!sp->width || !sp->height) {
 sp->width = vp->width;
@@ -939,9 +939,9 @@ static void video_image_display(VideoState *is)
 av_log(NULL, AV_LOG_FATAL, "Cannot initialize 
the conversion context\n");
 return;
 }
-if (!SDL_LockTexture(is->sub_texture, (SDL_Rect 
*)sub_rect, (void **)&pixels, &pitch)) {
+if (!SDL_LockTexture(is->sub_texture, (SDL_Rect 
*)sub_rect, (void **)pixels, pitch)) {
 sws_scale(is->sub_convert_ctx, (const uint8_t 
* const *)sub_rect->data, sub_rect->linesize,
-  0, sub_rect->h, &pixels, &pitch);
+  0, sub_rect->h, pixels, pitch);
 SDL_UnlockTexture(is->sub_texture);
 }
 }
-- 
2.10.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Carl Eugen Hoyos]

2016-12-10 12:13 GMT+01:00 wm4 :
> On Sat, 10 Dec 2016 11:51:04 +0100
> Carl Eugen Hoyos  wrote:
>
>> 2016-12-09 19:45 GMT+01:00 wm4 :
>>
>> > But users switching from FFmpeg to other software because
>> > it fails at the limit does happen.
>>
>> Could you elaborate?
>> Was there a bug report that we ignored?
>
> Reporting bugs to ffmpeg is so tedious, why would anyone do that?

We have a unreasonable large amount of bug reports that are
unlikely to get fixed (by purely technical reasons), I wonder
why something as trivial as increasing image size for actual
use cases is not reported.

> I think in the specific case I had in mind, imagemagick was
> used instead, but I could be wrong. The use-case was
> processing high-res scans.
>
> Surely not really a worry for ffmpeg, since it's concerned
> about video, and 16K video is still "a bit" in the future.

I just tested 16k and it works for some cases, reports "codec
isn't specified for 16k" for others and fails with x264.
What exactly was the issue that could be fixed in FFmpeg?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Reto Kromer]

wm4 wrote:

>Surely not really a worry for ffmpeg, since it's concerned
>about video, and 16K video is still "a bit" in the future.

If by "video" you mean "YUV", then may I disagree with you?
We use daily FFmpeg, and mainly in a "film" ("RGB") context
and often with a 5K or 6.6K horizontal resolution. We are
more than happy that FFmpeg is a such valuable tool!

Best regards, Reto

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[wm4]

On Fri,  9 Dec 2016 20:31:40 +0100
Michael Niedermayer  wrote:

> Adds av_image_check_size2() with max_pixels and pix_fmt parameters.
> pix_fmt is unused and is added to avoid a 2nd API change later
> 
> The old function uses AVOptions to obtain the max_pixels value to simplify
> the transition.
> 
> TODO: split into 2 patches (one per lib), docs & bump
> 
> This allows preventing some OOM and "slow decoding" cases by limiting the 
> maximum resolution
> this may be useful to avoid fuzzers getting stuck in boring cases
> 
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/avcodec.h |  8 
>  libavcodec/options_table.h   |  1 +
>  libavutil/imgutils.c | 31 ++-
>  libavutil/imgutils.h | 14 ++
>  tests/ref/fate/api-mjpeg-codec-param |  2 ++
>  tests/ref/fate/api-png-codec-param   |  2 ++
>  6 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 7ac2adaf66..81052b10ef 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3570,6 +3570,14 @@ typedef struct AVCodecContext {
>   */
>  int trailing_padding;
>  
> +/**
> + * The number of pixels per image to maximally accept.
> + *
> + * - decoding: set by user
> + * - encoding: unused
> + */
> +int max_pixels;
> +
>  } AVCodecContext;
>  
>  AVRational av_codec_get_pkt_timebase (const AVCodecContext *avctx);
> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> index ee79859953..2f5eb252fe 100644
> --- a/libavcodec/options_table.h
> +++ b/libavcodec/options_table.h
> @@ -570,6 +570,7 @@ static const AVOption avcodec_options[] = {
>  {"codec_whitelist", "List of decoders that are allowed to be used", 
> OFFSET(codec_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN, 
> CHAR_MAX, A|V|S|D },
>  {"pixel_format", "set pixel format", OFFSET(pix_fmt), AV_OPT_TYPE_PIXEL_FMT, 
> {.i64=AV_PIX_FMT_NONE}, -1, INT_MAX, 0 },
>  {"video_size", "set video size", OFFSET(width), AV_OPT_TYPE_IMAGE_SIZE, 
> {.str=NULL}, 0, INT_MAX, 0 },
> +{"max_pixels", "Maximum number of pixels", OFFSET(max_pixels), 
> AV_OPT_TYPE_INT, {.i64 = INT_MAX }, 0, INT_MAX, A|V|S|D },
>  {NULL},
>  };
>  
> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> index 37808e53d0..96f2bbdc4f 100644
> --- a/libavutil/imgutils.c
> +++ b/libavutil/imgutils.c
> @@ -30,6 +30,7 @@
>  #include "mathematics.h"
>  #include "pixdesc.h"
>  #include "rational.h"
> +#include "opt.h"
>  
>  void av_image_fill_max_pixsteps(int max_pixsteps[4], int 
> max_pixstep_comps[4],
>  const AVPixFmtDescriptor *pixdesc)
> @@ -248,7 +249,7 @@ static const AVClass imgutils_class = {
>  .parent_log_context_offset = offsetof(ImgUtils, log_ctx),
>  };
>  
> -int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void 
> *log_ctx)
> +int av_image_check_size2(unsigned int w, unsigned int h, int64_t max_pixels, 
> enum AVPixelFormat pix_fmt, int log_offset, void *log_ctx)
>  {
>  ImgUtils imgutils = {
>  .class  = &imgutils_class,
> @@ -256,11 +257,31 @@ int av_image_check_size(unsigned int w, unsigned int h, 
> int log_offset, void *lo
>  .log_ctx= log_ctx,
>  };
>  
> -if ((int)w>0 && (int)h>0 && (w+128)*(uint64_t)(h+128) < INT_MAX/8)
> -return 0;
> +if ((int)w<=0 || (int)h<=0 || (w+128)*(uint64_t)(h+128) >= INT_MAX/8) {
> +av_log(&imgutils, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", 
> w, h);
> +return AVERROR(EINVAL);
> +}
>  
> -av_log(&imgutils, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, h);
> -return AVERROR(EINVAL);
> +if (max_pixels < INT64_MAX) {
> +if (w*(int64_t)h > max_pixels) {
> +av_log(&imgutils, AV_LOG_ERROR,
> +"Picture size %ux%u exceeds specified max pixel count 
> %"PRId64"\n",
> +w, h, max_pixels);
> +return AVERROR(EINVAL);
> +}
> +}
> +
> +return 0;
> +}
> +
> +int av_image_check_size(unsigned int w, unsigned int h, int log_offset, void 
> *log_ctx)
> +{
> +int64_t max = INT64_MAX;
> +
> +if (log_ctx)
> +av_opt_get_int(log_ctx, "max_pixels",  AV_OPT_SEARCH_CHILDREN, &max);
> +
> +return av_image_check_size2(w, h, max, AV_PIX_FMT_NONE, log_offset, 
> log_ctx);
>  }

Still against implicitly using an AVOption. Explicitly passing limits is
better. Also, while your suggestion is convenient, it doesn't guarantee
that ever caller will pass the correct context to it (i.e. an
AVCodecContext that happens to have max_pixels somehow set from the
CLI), or that someone inspecting the code on the callee side is aware
about the AVOption retrieval and does not fix it. Someone new to the
codebase will also be puzzled what the seemingly unused max_pixels
field does at all.

>  
>  int av_image_check_sar(unsigned int w, uns

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[wm4]

On Sat, 10 Dec 2016 11:51:04 +0100
Carl Eugen Hoyos  wrote:

> 2016-12-09 19:45 GMT+01:00 wm4 :
> 
> > But users switching from FFmpeg to other software because
> > it fails at the limit does happen.  
> 
> Could you elaborate?
> Was there a bug report that we ignored?

Reporting bugs to ffmpeg is so tedious, why would anyone do that?

I think in the specific case I had in mind, imagemagick was used
instead, but I could be wrong. The use-case was processing high-res
scans.

Surely not really a worry for ffmpeg, since it's concerned about video,
and 16K video is still "a bit" in the future.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] lavfi/af_atempo: add support for unknown channel layouts

[Marton Balint]

On Sun, 4 Dec 2016, Nicolas George wrote:


Le quartidi 14 frimaire, an CCXXV, Marton Balint a écrit :

Signed-off-by: Marton Balint 
---
 libavfilter/af_atempo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


All three patches look good, thanks.



Thanks, applied all three.

Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv2] avfilter/formats: allow unknown channel layouts by default

[Marton Balint]

On Fri, 9 Dec 2016, Nicolas George wrote:


L'octidi 18 frimaire, an CCXXV, Marton Balint a écrit :

Since the default in the libav fork is to only allow known layouts, making
unknown layouts allowed by default here can be a security risk for filters
directly merged from libav. However, usually it is simple to detect such cases,
use of av_get_channel_layout_nb_channels is a good indicator, so I suggest we
change this regardless.


No objection from me, and the changes look consistent.



Thanks, applied.

Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [DECISION] Revoke the decision of dropping ffserver

[wm4]

On Sat, 10 Dec 2016 11:28:07 +0100
Carl Eugen Hoyos  wrote:

> 2016-12-08 15:53 GMT+01:00 wm4 :
> > advanced hardware transcoding (I'm still waiting for related
> > work to be merged from Libav).  
> 
> Didn't you tell the responsible developer not to send his patches
> here implying you did not want them committed to FFmpeg?

The answer to that is "no".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Support limiting the number of pixels per image

[Carl Eugen Hoyos]

2016-12-09 19:45 GMT+01:00 wm4 :

> But users switching from FFmpeg to other software because
> it fails at the limit does happen.

Could you elaborate?
Was there a bug report that we ignored?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [DECISION] Revoke the decision of dropping ffserver

[Carl Eugen Hoyos]

2016-12-08 15:53 GMT+01:00 wm4 :
> advanced hardware transcoding (I'm still waiting for related
> work to be merged from Libav).

Didn't you tell the responsible developer not to send his patches
here implying you did not want them committed to FFmpeg?

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel