Re: [FFmpeg-devel] [PATCH]Two patches from github

[Benoit Fouet]

Hi,


On 15/05/2017 12:40, Carl Eugen Hoyos wrote:
>
> 0002-avutil-Use-_SC_NPROCESSORS_CONF.patch
>
>
> From 42766f345dbf398716c6fd9072f072f5fa91c940 Mon Sep 17 00:00:00 2001
> From: Steve Kondik 
> Date: Tue, 16 Dec 2014 01:37:57 -0800
> Subject: [PATCH 2/2] avutil: Use _SC_NPROCESSORS_CONF
>
>  * On most Android devices, CPUs can appear and disappear due to hotplug
>or CPU cluster management. Use the total number of CPUs instead so
>that multithreaded decoding is properly optimized.

I'm not convinced the patch below fixes the issue described above.
The idea is to optimize the number of threads given the number of usable
CPUs.
If a user of libav* wants to make sure they use the right number, they
should make sure that the CPUs are woken up prior to calling this.
There are ways to force the system's power management to make sure that
the number of online CPUs is maximized, and, IMHO, that should be used
instead of returning the total number of CPUs on the platform.

> Change-Id: I1cbf000a1bda7b3abf0a84e971e752f176857385
> ---
>  libavutil/cpu.c |2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavutil/cpu.c b/libavutil/cpu.c
> index 16e0c92..ab0965b 100644
> --- a/libavutil/cpu.c
> +++ b/libavutil/cpu.c
> @@ -282,6 +282,8 @@ int av_cpu_count(void)
>  
>  if (sysctl(mib, 2, _cpus, , NULL, 0) == -1)
>  nb_cpus = 0;
> +#elif defined(__ANDROID__) && HAVE_SYSCONF && defined(_SC_NPROCESSORS_CONF)
> +nb_cpus = sysconf(_SC_NPROCESSORS_CONF);
>  #elif HAVE_SYSCONF && defined(_SC_NPROC_ONLN)
>  nb_cpus = sysconf(_SC_NPROC_ONLN);
>  #elif HAVE_SYSCONF && defined(_SC_NPROCESSORS_ONLN)
> -- 1.7.10.4

-- 
Ben

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

Re: [FFmpeg-devel] [PATCH] lavc/aarch64/simple_idct: separate macro arguments with commas

[Benoit Fouet]

Hi,


On 28/04/2017 21:58, Matthieu Bouron wrote:
> Untested: fixes ticket #6324.
> ---
>  libavcodec/aarch64/simple_idct_neon.S | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/libavcodec/aarch64/simple_idct_neon.S 
> b/libavcodec/aarch64/simple_idct_neon.S
> index 52273420f9..d31f72a609 100644
> --- a/libavcodec/aarch64/simple_idct_neon.S
> +++ b/libavcodec/aarch64/simple_idct_neon.S
> @@ -61,19 +61,19 @@ endconst
>  br  x10
>  .endm
>  
> -.macro smull1 a b c
> +.macro smull1 a, b, c
>  smull   \a, \b, \c
>  .endm
>  
> -.macro smlal1 a b c
> +.macro smlal1 a, b, c
>  smlal   \a, \b, \c
>  .endm
>  
> -.macro smlsl1 a b c
> +.macro smlsl1 a, b, c
>  smlsl   \a, \b, \c
>  .endm
>  
> -.macro idct_col4_top y1 y2 y3 y4 i l
> +.macro idct_col4_top y1, y2, y3, y4, i, l
>  smull\i v7.4S,  \y3\().\l, z2
>  smull\i v16.4S, \y3\().\l, z6
>  smull\i v17.4S, \y2\().\l, z1
> @@ -91,7 +91,7 @@ endconst
>  smlsl\i v6.4S,  \y4\().\l, z5
>  .endm
>  
> -.macro idct_row4_neon y1 y2 y3 y4 pass
> +.macro idct_row4_neon y1, y2, y3, y4, pass
>  ld1 {\y1\().2D-\y2\().2D}, [x2], #32
>  moviv23.4S, #1<<2, lsl #8
>  orr v5.16B, \y1\().16B, \y2\().16B
> @@ -153,7 +153,7 @@ endconst
>  trn2\y4\().4S, v17.4S, v19.4S
>  .endm
>  
> -.macro declare_idct_col4_neon i l
> +.macro declare_idct_col4_neon i, l
>  function idct_col4_neon\i
>  dup v23.4H, z4c
>  .if \i == 1

Sounds sane, but shouldn't we be doing this for all instances of
multiple arguments macros without commas?

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

Re: [FFmpeg-devel] [PATCH] [UPDATE] HLS, add option to skip down streams,

[Benoit Fouet]

Hi,


On 20/04/2017 18:18, Amine kabab wrote:
> From 5079f9b7114589626a4c9fff0fbb8f6e0d2f4fd9 Mon Sep 17 00:00:00 2001
> From: amine kabab 
> Date: Thu, 20 Apr 2017 15:59:42 +
> Subject: [PATCH] HLS skip down streams
>
> ---
>  libavformat/hls.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index bac53a4..26d7679 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -194,6 +194,7 @@ typedef struct HLSContext {
>  
>  int cur_seq_no;
>  int live_start_index;
> +int skip_down_streams;
>  int first_packet;
>  int64_t first_timestamp;
>  int64_t cur_timestamp;
> @@ -1652,11 +1653,20 @@ static int hls_read_header(AVFormatContext *s)
>  /* If the playlist only contained playlists (Master Playlist),
>   * parse each individual playlist. */
>  if (c->n_playlists > 1 || c->playlists[0]->n_segments == 0) {
> +int all_failed = 1;
>  for (i = 0; i < c->n_playlists; i++) {
>  struct playlist *pls = c->playlists[i];
> -if ((ret = parse_playlist(c, pls->url, pls, NULL)) < 0)
> +av_log(NULL, AV_LOG_WARNING, "Trying %s\n", pls->url);

Do you really want to keep this trace?

> +ret = parse_playlist(c, pls->url, pls, NULL);
> +if (c->skip_down_streams && ret >= 0) {
> +all_failed = 0;
> +} else if (!c->skip_down_streams && ret < 0){
>  goto fail;
> +}

If I understand correctly, that means that if you do not set
skip_down_streams and the playlist parsing is OK, you will not unset
all_failed, and bail out below. Is this really what you want?

>  }
> +
> +if (all_failed)
> +goto fail;
>  }
>  
>  if (c->variants[0]->playlists[0]->n_segments == 0) {
> @@ -2126,6 +2136,8 @@ static int hls_probe(AVProbeData *p)
>  static const AVOption hls_options[] = {
>  {"live_start_index", "segment index to start live streams at (negative 
> values are from the end)",
>  OFFSET(live_start_index), AV_OPT_TYPE_INT, {.i64 = -3}, INT_MIN, 
> INT_MAX, FLAGS},
> +{"skip_down_streams", "continue playback of HLS when one of the variant 
> streams are down",
> +OFFSET(skip_down_streams), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, 
> FLAGS},
>  {NULL}
>  };

-- 
Ben

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

Re: [FFmpeg-devel] [PATCH 1/3] avutil/spherical: add av_spherical_projection_name()

[Benoit Fouet]

Hi,


On 29/03/2017 04:55, James Almer wrote:
> Signed-off-by: James Almer 
> ---
>  doc/APIchanges|  3 +++
>  libavutil/spherical.c | 15 +++
>  libavutil/spherical.h |  9 +
>  3 files changed, 27 insertions(+)
>

[...]

> diff --git a/libavutil/spherical.c b/libavutil/spherical.c
> index f0b622128a..1d06e7c552 100644
> --- a/libavutil/spherical.c
> +++ b/libavutil/spherical.c
> @@ -50,3 +50,18 @@ void av_spherical_tile_bounds(const AVSphericalMapping 
> *map,
>  *right  = orig_width  - width  - *left;
>  *bottom = orig_height - height - *top;
>  }
> +
> +static const char *spherical_projection_names[] = {
> +[AV_SPHERICAL_EQUIRECTANGULAR]  = "equirectangular",
> +[AV_SPHERICAL_CUBEMAP]  = "cubemap",
> +[AV_SPHERICAL_EQUIRECTANGULAR_TILE] = "tiled equirectangular",
> +
> +};
> +
> +const char *av_spherical_projection_name(enum AVSphericalProjection 
> projection)
> +{
> +if (projection >= FF_ARRAY_ELEMS(spherical_projection_names))
> +return "unknown";
> +

You should also check for projection to be negative, or cast it to
unsigned when checking.

-- 
Ben

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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/apng: set max_fps to no limit by default

[Benoit Fouet]

Hi,


On 21/03/2017 14:03, James Almer wrote:
> On 3/21/2017 9:52 AM, Michael Niedermayer wrote:
>> On Mon, Mar 20, 2017 at 11:03:23PM -0300, James Almer wrote:
>>> Should fix ticket #6252
>>>
>>> Signed-off-by: James Almer 
>>> ---
>>>  libavformat/apngdec.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
>>> index 7a284e32c2..75dcf74a0c 100644
>>> --- a/libavformat/apngdec.c
>>> +++ b/libavformat/apngdec.c
>>> @@ -421,7 +421,7 @@ static const AVOption options[] = {
>>>  { "ignore_loop", "ignore loop setting" , 
>>> offsetof(APNGDemuxContext, ignore_loop),
>>>AV_OPT_TYPE_BOOL, { .i64 = 1 }  , 0, 1  , 
>>> AV_OPT_FLAG_DECODING_PARAM },
>>>  { "max_fps", "maximum framerate (0 is no limit)"   , 
>>> offsetof(APNGDemuxContext, max_fps),
>>> -  AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, 
>>> AV_OPT_FLAG_DECODING_PARAM },
>>> +  AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, 
>>> AV_OPT_FLAG_DECODING_PARAM },
>>>  { "default_fps", "default framerate (0 is as fast as possible)", 
>>> offsetof(APNGDemuxContext, default_fps),
>>>AV_OPT_TYPE_INT, { .i64 = DEFAULT_APNG_FPS }, 0, INT_MAX, 
>>> AV_OPT_FLAG_DECODING_PARAM },
>>>  { NULL },
>> why was there a max fps set ?
>> are there files which have huge and incorrect fps ?
> I have no idea. The author of the decoder may know.
>

A bit late, but honestly, I don't remember why I did it that way, though
both patches look fine as they are.
It's easy enough to come back to that code when/if needed.

Thanks,
-- 
Ben

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

Re: [FFmpeg-devel] [PATCHv3] mov: Evaluate the movie display matrix

[Benoit Fouet]

Hi,

On 14/10/2016 00:50, Vittorio Giovara wrote:
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index a15c8d1..e8da77f 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c

[...]

> @@ -3798,16 +3804,33 @@ static int mov_read_meta(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  return 0;
>  }
>  
> +// return 0 when matrix is identity, 1 otherwise
> +#define IS_MATRIX_FULL(matrix)   \
> +(matrix[0][0] != (1 << 16) ||\
> + matrix[1][1] != (1 << 16) ||\
> + matrix[2][2] != (1 << 30) ||\
> + matrix[0][1] || matrix[0][2] || \
> + matrix[1][0] || matrix[1][2] || \
> + matrix[2][0] || matrix[2][1])
> +

should be "(matrix)" everywhere
Also, reversing the logic would allow preventing the evaluation of all
conditions when the matrix is not identity (i.e. making it
IS_MATRIX_IDENTITY)

> +// fixed point to double
> +#define CONV_FP(x, sh) ((double) (x)) / (1 << (sh))
> +

((double) (x) / (1 << (sh)))

> +// double to fixed point
> +#define CONV_DB(x, sh) ((int32_t) ((x) * (1 << (sh
> +

Cheers,
-- 
Ben


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

Re: [FFmpeg-devel] [PATCH] matroskadec: fix NULL pointer dereference

[Benoit Fouet]

Hi,

On 17/10/2016 06:49, James Almer wrote:

On 10/16/2016 9:30 PM, James Almer wrote:

On 10/16/2016 5:11 PM, Andreas Cadhalpun wrote:

The problem was introduced in commit 1273bc6.

Signed-off-by: Andreas Cadhalpun 
---
  libavformat/matroskadec.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 8847c62..a5d3c0e 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1759,7 +1759,7 @@ static int mkv_field_order(MatroskaDemuxContext 
*matroska, int64_t field_order)
  
  /* workaround a bug in our Matroska muxer, introduced in version 57.36 alongside

   * this function, and fixed in 57.52 */
-if (sscanf(matroska->muxingapp, "Lavf%d.%d.%d", , , ) == 
3)
+if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", , 
, ) == 3)

LGTM.

Matroska files are supposed to always have that element, but even ffmpeg used
to mux files without it at some point when bitexact flag was enabled, so i
guess plenty of files out there are missing it.


  bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100);
  
  switch (field_order) {



Just tried a file missing the muxingapp element, meaning matroska->muxingapp
is NULL, and sscanf simply returns -1 and sets errno to EINVAL.

Where does it crash for you and using what file?


FWIW, I've just written a quick test on my machine, and an "sscanf(NULL, 
"%d", )" segfaults.


--
Ben

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

Re: [FFmpeg-devel] [PATCH] avformat/matroskadec: retain error codes in matroska_resync() and matroska_read_packet()

[Benoit Fouet]

Hi,


On 22/09/2016 23:03, Sophia Wang wrote:

Signed-off-by: Sophia Wang 
---
  libavformat/matroskadec.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 77b8a5d..936690d 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext *s);
  static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
  {
  AVIOContext *pb = matroska->ctx->pb;
+int64_t ret;
  uint32_t id;
  matroska->current_id = 0;
  matroska->num_levels = 0;
  
  /* seek to next position to resync from */

-if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0)
-goto eof;
+if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) {
+matroska->done = 1;
+return ret;


doesn't this generate a warning, returning an int64 from a function 
supposed to return an int?



+}
  
  id = avio_rb32(pb);
  
@@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)

  id = (id << 8) | avio_r8(pb);
  }
  
-eof:

  matroska->done = 1;
  return AVERROR_EOF;
  }
@@ -3317,13 +3319,14 @@ static int matroska_parse_cluster(MatroskaDemuxContext 
*matroska)
  static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt)
  {
  MatroskaDemuxContext *matroska = s->priv_data;
+int ret = 0;
  
  while (matroska_deliver_packet(matroska, pkt)) {

  int64_t pos = avio_tell(matroska->ctx->pb);
  if (matroska->done)
-return AVERROR_EOF;
+return (ret < 0) ? ret : AVERROR_EOF;
  if (matroska_parse_cluster(matroska) < 0)
-matroska_resync(matroska, pos);
+ret = matroska_resync(matroska, pos);
  }
  
  return 0;


You might want to return ret instead of 0 here.

--
Ben

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

Re: [FFmpeg-devel] [PATCH] avfilter: add nlmeans filter

[Benoit Fouet]

Hi,


On 20/09/2016 21:52, Clément Bœsch wrote:

Fixes Ticket #4910
---
I actually tried to implement the better defaults suggestion from ipol (see
@todo) but it wasn't convincing; probably because of different scales, so I
need to investigate.

Also, integral is still inplace in the filter for now as I didn't find a clean
way of testing it outside the filter without a long trip in dependency hell. I
think it can wait until the SIMD are implemented and the need to expose it
comes up.

I've made several changes from the initial WIP. The most important one is the
fix in the patch distance calculation, followed by the the addition of chroma
parameters.

I believe the filter is ready for integration as a first version.

Two interesting examples: http://imgur.com/a/XXhJP
---
  Changelog|   1 +
  doc/filters.texi |  35 +++
  libavfilter/Makefile |   3 +-
  libavfilter/allfilters.c |   1 +
  libavfilter/tests/integral.c |  92 
  libavfilter/version.h|   2 +-
  libavfilter/vf_nlmeans.c | 548 +++
  7 files changed, 680 insertions(+), 2 deletions(-)
  create mode 100644 libavfilter/tests/integral.c
  create mode 100644 libavfilter/vf_nlmeans.c

diff --git a/Changelog b/Changelog
index 2d0a449..a5282b4 100644
--- a/Changelog
+++ b/Changelog
@@ -31,6 +31,7 @@ version :
  - MediaCodec HEVC decoding
  - TrueHD encoder
  - Meridian Lossless Packing (MLP) encoder
+- nlmeans filter (denoiser)
  


The full name could be used here: Non-Local Means (nlmeans) denoising filter

  
  version 3.1:

diff --git a/doc/filters.texi b/doc/filters.texi
index 070e57d..7e9ab60 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -9695,6 +9695,41 @@ Negate input video.
  It accepts an integer in input; if non-zero it negates the
  alpha component (if available). The default value in input is 0.
  
+@section nlmeans

+
+Denoise frames using Non-Local Means algorithm.
+
+Each pixel is adjusted by looking for other pixels with similar contexts. This
+context similarity is defined by their surrounding patch of size


"is defined by comparing their surrounding patches" ?


+@option{p}x@option{p}. Patches are researched in an area of
+@option{r}x@option{r} surrouding the pixel.
+


surrounding, or even simply "around"
Also "research" sounds weird (I'd use "search"), but maybe wait for 
someone native to comment


[...]


diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c
new file mode 100644
index 000..f923f80
--- /dev/null
+++ b/libavfilter/vf_nlmeans.c
@@ -0,0 +1,548 @@
+/*
+ * Copyright (c) 2016 Clément Bœsch 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * @todo
+ * - SIMD for compute_safe_ssd_integral_image
+ * - SIMD for final weighted averaging
+ * - better automatic defaults? see "Parameters" @ 
http://www.ipol.im/pub/art/2011/bcm_nlm/
+ * - temporal support (probably doesn't need any displacement according to
+ *   "Denoising image sequences does not require motion estimation")
+ * - bayer support?
+ * - FATE test (probably needs visual threshold test mechanism due to the use 
of floats)
+ */
+
+#include "libavutil/avassert.h"
+#include "libavutil/opt.h"
+#include "libavutil/pixdesc.h"
+#include "avfilter.h"
+#include "formats.h"
+#include "internal.h"
+#include "video.h"
+
+struct weighted_avg {
+double total_weight;
+double sum;
+};
+
+#define WEIGHT_LUT_NBITS 9
+#define WEIGHT_LUT_SIZE  (1<

Re: [FFmpeg-devel] [PATCH] avcodec/h264: Use ptrdiff_t for (bi)width functions

[Benoit Fouet]

Hi,


On 21/09/2016 03:34, Michael Niedermayer wrote:

Might fix some mysterious asm related issues like Ticket5579

Signed-off-by: Michael Niedermayer 
---
  libavcodec/h264dsp.h   |  5 +++--
  libavcodec/h264dsp_template.c  |  4 ++--
  libavcodec/mips/h264dsp_mips.h | 26 +-
  libavcodec/mips/h264dsp_mmi.c  | 12 ++--
  libavcodec/x86/h264dsp_init.c  |  8 
  5 files changed, 28 insertions(+), 27 deletions(-)


Typo in the commit message: should be weight, not width.

--
Ben

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

Re: [FFmpeg-devel] [PATCH] lavc/mediacodecdec_h264: fix SODB escaping

[Benoit Fouet]

Hi,


On 06/09/2016 16:53, Matthieu Bouron wrote:

From: Matthieu Bouron 

Fixes escaping of consecutive 0x00, 0x00, 0x0{0-3} sequences.
---
  libavcodec/mediacodecdec_h264.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavcodec/mediacodecdec_h264.c b/libavcodec/mediacodecdec_h264.c
index a141174..4f9d737 100644
--- a/libavcodec/mediacodecdec_h264.c
+++ b/libavcodec/mediacodecdec_h264.c
@@ -104,9 +104,9 @@ static int h264_ps_to_nalu(const uint8_t *src, int 
src_size, uint8_t **out, int
  }
  *out = p = new;
  
-i = i + 3;

-memmove(p + i, p + i - 1, *out_size - i);
-p[i - 1] = 0x03;
+i = i + 2;
+memmove(p + i + 1, p + i, *out_size - (i + 1));
+p[i] = 0x03;


LGTM

--
Ben

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

Re: [FFmpeg-devel] [PATCH] lavc/ffjni: replace ff_jni_{attach, detach} with ff_jni_get_env

[Benoit Fouet]

Hi,

On 24/07/2016 23:05, Matthieu Bouron wrote:

From: Matthieu Bouron

If a JNI environment is not already attached to the thread where the
MediaCodec calls are made the current implementation will attach /
detach an environment for each MediaCodec call wasting some CPU time.

ff_jni_get_env replaces ff_jni_{attach,detach} by permanently attaching
an environment (if it is not already the case) to the current thread.
The environment will be automatically detached at the thread destruction
using a pthread_key callback.

Saves around 5% of CPU time (out of 20%) while decoding a stream with
MediaCodec.
---
  libavcodec/ffjni.c  |  43 +
  libavcodec/ffjni.h  |  15 +--


LGTM


  libavcodec/mediacodec.c |  14 +--
  libavcodec/mediacodec_surface.c |  14 +--
  libavcodec/mediacodec_wrapper.c | 200 
  5 files changed, 74 insertions(+), 212 deletions(-)


Just had a quick look at those ones.

Cheers,
--
Ben

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

Re: [FFmpeg-devel] [PATCH] lavc/mediacodec: add hwaccel support

[Benoit Fouet]

On 08/07/2016 15:40, Matthieu Bouron wrote:

On Fri, Jul 08, 2016 at 01:21:48PM +0200, Benoit Fouet wrote:

Hi,

On 07/07/2016 17:43, Matthieu Bouron wrote:

[...]


0001-lavc-add-mediacodec-hwaccel-support.patch


  From 9bb86990f0f7a26d25878a771f5977ae83d14769 Mon Sep 17 00:00:00 2001
From: Matthieu Bouron<matthieu.bou...@stupeflix.com>
Date: Fri, 11 Mar 2016 17:21:04 +0100
Subject: [PATCH] lavc: add mediacodec hwaccel support

---
   configure   |   1 +
   libavcodec/Makefile |   6 +-
   libavcodec/allcodecs.c  |   1 +
   libavcodec/mediacodec.c | 133 
   libavcodec/mediacodec.h |  88 +
   libavcodec/mediacodec_surface.c |  66 ++
   libavcodec/mediacodec_surface.h |  31 +
   libavcodec/mediacodec_wrapper.c |   5 +-
   libavcodec/mediacodecdec.c  | 270 
+---
   libavcodec/mediacodecdec.h  |  17 +++
   libavcodec/mediacodecdec_h264.c |  44 ++-
   libavcodec/version.h|   2 +-
   libavutil/pixdesc.c |   4 +
   libavutil/pixfmt.h  |   2 +
   14 files changed, 611 insertions(+), 59 deletions(-)
   create mode 100644 libavcodec/mediacodec.c
   create mode 100644 libavcodec/mediacodec.h
   create mode 100644 libavcodec/mediacodec_surface.c
   create mode 100644 libavcodec/mediacodec_surface.h


[...]


diff --git a/libavcodec/mediacodecdec.h b/libavcodec/mediacodecdec.h
index 646b628..8613352 100644
--- a/libavcodec/mediacodecdec.h
+++ b/libavcodec/mediacodecdec.h
@@ -34,12 +34,17 @@
   typedef struct MediaCodecDecContext {
+volatile int refcount;
+

I don't think this needs to be marked volatile

The avpriv_atomic_[...] functions take a volatile int *ptr. Also there are
examples in our code base that declare the atomic as volatile int (see
libavutil/buffer_internal.h, libavcodec/mmaldec.c for example). I can
be missing something though.


as the atomic api is working on addresses, it should not make any 
difference.

I don't really mind, though, keep it this way if you prefer
I'm not a volatile fan boy, so I try to refrain from using them when 
it's not strictly needed :-)


[...]


If you are OK with my comments, I will push the patch.


Yes, sure.

Cheers,
--
Ben

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

Re: [FFmpeg-devel] [PATCH] lavc/mediacodec: add hwaccel support

[Benoit Fouet]

Hi,

On 07/07/2016 17:43, Matthieu Bouron wrote:

[...]



0001-lavc-add-mediacodec-hwaccel-support.patch


 From 9bb86990f0f7a26d25878a771f5977ae83d14769 Mon Sep 17 00:00:00 2001
From: Matthieu Bouron
Date: Fri, 11 Mar 2016 17:21:04 +0100
Subject: [PATCH] lavc: add mediacodec hwaccel support

---
  configure   |   1 +
  libavcodec/Makefile |   6 +-
  libavcodec/allcodecs.c  |   1 +
  libavcodec/mediacodec.c | 133 
  libavcodec/mediacodec.h |  88 +
  libavcodec/mediacodec_surface.c |  66 ++
  libavcodec/mediacodec_surface.h |  31 +
  libavcodec/mediacodec_wrapper.c |   5 +-
  libavcodec/mediacodecdec.c  | 270 +---
  libavcodec/mediacodecdec.h  |  17 +++
  libavcodec/mediacodecdec_h264.c |  44 ++-
  libavcodec/version.h|   2 +-
  libavutil/pixdesc.c |   4 +
  libavutil/pixfmt.h  |   2 +
  14 files changed, 611 insertions(+), 59 deletions(-)
  create mode 100644 libavcodec/mediacodec.c
  create mode 100644 libavcodec/mediacodec.h
  create mode 100644 libavcodec/mediacodec_surface.c
  create mode 100644 libavcodec/mediacodec_surface.h

[...]

diff --git a/libavcodec/mediacodec.c b/libavcodec/mediacodec.c
new file mode 100644
index 000..5b79798
--- /dev/null
+++ b/libavcodec/mediacodec.c
@@ -0,0 +1,133 @@
+/*
+ * Android MediaCodec public API functions
+ *
+ * Copyright (c) 2016 Matthieu Bouron 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "config.h"
+
+#if CONFIG_H264_MEDIACODEC_HWACCEL
+
+#include 
+
+#include "libavcodec/avcodec.h"
+#include "libavutil/atomic.h"
+#include "libavutil/mem.h"
+
+#include "ffjni.h"
+#include "mediacodec.h"
+#include "mediacodecdec.h"
+
+AVMediaCodecContext *av_mediacodec_alloc_context(void)
+{
+return av_mallocz(sizeof(AVMediaCodecContext));
+}
+
+int av_mediacodec_default_init(AVCodecContext *avctx, AVMediaCodecContext 
*ctx, void *surface)
+{
+int ret = 0;
+JNIEnv *env = NULL;
+int attached = 0;
+


nit: env and attached don't need to be initialized

+env = ff_jni_attach_env(, avctx);
+if (!env) {
+return AVERROR_EXTERNAL;
+}
+
+ctx->surface = (*env)->NewGlobalRef(env, surface);
+if (ctx->surface) {
+avctx->hwaccel_context = ctx;
+} else {
+av_log(avctx, AV_LOG_ERROR, "Could not create new global reference\n");
+ret = AVERROR_EXTERNAL;
+}
+
+if (attached) {
+ff_jni_detach_env(avctx);
+}
+
+return ret;
+}
+
+void av_mediacodec_default_free(AVCodecContext *avctx)
+{
+JNIEnv *env = NULL;
+int attached = 0;
+


ditto

[...]


diff --git a/libavcodec/mediacodec.h b/libavcodec/mediacodec.h
new file mode 100644
index 000..f755bd1
--- /dev/null
+++ b/libavcodec/mediacodec.h
@@ -0,0 +1,88 @@
+/*
+ * Android MediaCodec public API
+ *
+ * Copyright (c) 2016 Matthieu Bouron 
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef AVCODEC_MEDIACODEC_H
+#define AVCODEC_MEDIACODEC_H
+
+#include "libavcodec/avcodec.h"
+
+/**
+ * This structure holds a reference to a android/view/Surface object that will
+ * be used as output by the decoder.
+ *
+ */
+typedef struct AVMediaCodecContext {
+
+/**
+ * android/view/Surface object reference.
+ */
+void *surface;
+
+} AVMediaCodecContext;
+
+/**
+ * Allocate and initialize a MediaCodec context.
+ *
+ * When decoding with MediaCodec is finished, the caller 

Re: [FFmpeg-devel] [PATCH] mediacodecdec_h264: properly convert extradata to annex-b

[Benoit Fouet]

Hi,

On 04/07/2016 10:12, Matthieu Bouron wrote:

From: Matthieu Bouron 

H264ParamSets has its SPS/PPS stored raw (SODB) and needs to be
converted to NAL units before sending them to MediaCodec.

This patch adds the missing convertion of the SPS/PPS from SOBP to RBSP
which makes the resulting NAL units correct.

Fixes codec initialization on Nexus 4 and Nexus 7.
---
  libavcodec/mediacodecdec_h264.c | 69 +
  1 file changed, 56 insertions(+), 13 deletions(-)

diff --git a/libavcodec/mediacodecdec_h264.c b/libavcodec/mediacodecdec_h264.c
index 0664e49..f07c7cc 100644
--- a/libavcodec/mediacodecdec_h264.c
+++ b/libavcodec/mediacodecdec_h264.c
@@ -65,6 +65,54 @@ static av_cold int mediacodec_decode_close(AVCodecContext 
*avctx)
  return 0;
  }
  
+static int h264_ps_to_nalu(const uint8_t *src, int src_size, uint8_t **out, int *out_size)

+{
+int i;
+int ret = 0;
+uint8_t *p = NULL;
+static const uint8_t nalu_header[] = { 0x00, 0x00, 0x00, 0x01 };
+
+if (!out) {
+return AVERROR(EINVAL);
+}
+


out_size should also be checked


+*out_size = sizeof(nalu_header) + src_size;
+*out = p = av_malloc(*out_size);
+if (!*out) {
+return AVERROR(ENOMEM);
+}
+


nit: the size affectation could be done once the allocation is OK.


+memcpy(p, nalu_header, sizeof(nalu_header));
+memcpy(p + sizeof(nalu_header), src, src_size);
+
+/* Escape 0x00, 0x00, 0x0{0-3} pattern */
+for (i = 4; i < *out_size; i++) {
+if (i < *out_size - 3 &&
+p[i + 0] == 0 &&
+p[i + 1] == 0 &&
+p[i + 2] <= 3) {
+uint8_t *new;
+
+*out_size += 1;
+new = av_realloc(*out, *out_size);
+if (!new) {
+ret = AVERROR(ENOMEM);
+goto done;
+}
+*out = p = new;
+
+i = i + 3;
+memmove(p + i, p + i - 1, *out_size - i);
+p[i - 1] = 0x03;
+}
+}
+done:
+if (ret < 0)
+av_freep(out);
+
+return ret;
+}
+
  static av_cold int mediacodec_decode_init(AVCodecContext *avctx)
  {
  int i;
@@ -112,24 +160,19 @@ static av_cold int mediacodec_decode_init(AVCodecContext 
*avctx)
  }
  
  if (pps && sps) {

-static const uint8_t nal_headers[] = { 0x00, 0x00, 0x00, 0x01 };
-
  uint8_t *data = NULL;
-size_t data_size = sizeof(nal_headers) + FFMAX(sps->data_size, 
pps->data_size);
+size_t data_size = 0;
  
-data = av_mallocz(data_size);

-if (!data) {
-ret = AVERROR(ENOMEM);
+if ((ret = h264_ps_to_nalu(sps->data, sps->data_size, , _size)) 
< 0) {
  goto done;
  }
+ff_AMediaFormat_setBuffer(format, "csd-0", (void*)data, data_size);
+av_freep();
  
-memcpy(data, nal_headers, sizeof(nal_headers));

-memcpy(data + sizeof(nal_headers), sps->data, sps->data_size);
-ff_AMediaFormat_setBuffer(format, "csd-0", (void*)data, 
sizeof(nal_headers) + sps->data_size);
-
-memcpy(data + sizeof(nal_headers), pps->data, pps->data_size);
-ff_AMediaFormat_setBuffer(format, "csd-1", (void*)data, 
sizeof(nal_headers) + pps->data_size);
-
+if ((ret = h264_ps_to_nalu(pps->data, pps->data_size, , _size)) 
< 0) {
+goto done;
+}
+ff_AMediaFormat_setBuffer(format, "csd-1", (void*)data, data_size);
  av_freep();
  } else {
  av_log(avctx, AV_LOG_ERROR, "Could not extract PPS/SPS from 
extradata");


LGTM otherwise

--
Ben

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

Re: [FFmpeg-devel] [PATCH] H264ParamSets: make sps const

[Benoit Fouet]

Hi,

On 27/06/2016 14:37, Benoit Fouet wrote:

Hi,

First patch change decode_scaling_matrices function, so that it does 
not affect the SPS structure it gets, and marks SPS and PPS const in 
its arguments.


Second patch straightens the check for macroblock sizes in 
ff_h264_decode_seq_parameter_set and removes the unneeded check in 
H.264 slice init_dimensions.


Last patch make SPS const in H264ParamSets and fixes its usages.



Patchset pushed

--
Ben

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

Re: [FFmpeg-devel] [PATCH] git: ignore test binaries

[Benoit Fouet]

Hi,

On 29/06/2016 17:57, Clément Bœsch wrote:

On Wed, Jun 29, 2016 at 05:54:37PM +0200, Benoit Fouet wrote:

 From e514644033781cb431641ae088482f5a8aa2de42 Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Wed, 29 Jun 2016 17:53:50 +0200
Subject: [PATCH] git: ignore test binaries

---
  .gitignore | 1 +
  1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 524fb73..670d1d2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -19,6 +19,7 @@
  *.swp
  *.ver
  *_g
+*-test
  \#*
  .\#*
  /.config

You shouldn't have any, they moved somewhere else without the -test suffix
(with the exception of fate api tests which have the proper entry in their
own .gitignore)

Maybe previously built programs?


indeed, dropped.

Thanks,
--
Ben

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

[FFmpeg-devel] [PATCH] git: ignore test binaries

[Benoit Fouet]

From e514644033781cb431641ae088482f5a8aa2de42 Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Wed, 29 Jun 2016 17:53:50 +0200
Subject: [PATCH] git: ignore test binaries

---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 524fb73..670d1d2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -19,6 +19,7 @@
 *.swp
 *.ver
 *_g
+*-test
 \#*
 .\#*
 /.config
-- 
2.9.0.37.g6d523a3

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

Re: [FFmpeg-devel] [PATCH] H264ParamSets: make sps const

[Benoit Fouet]

Hi,

On 27/06/2016 14:37, Benoit Fouet wrote:

Hi,

First patch change decode_scaling_matrices function, so that it does 
not affect the SPS structure it gets, and marks SPS and PPS const in 
its arguments.


Second patch straightens the check for macroblock sizes in 
ff_h264_decode_seq_parameter_set and removes the unneeded check in 
H.264 slice init_dimensions.


Last patch make SPS const in H264ParamSets and fixes its usages.



I'll apply the patchset in a day or two, if there is no more feedback.

Thanks,
--
Ben

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

Re: [FFmpeg-devel] [PATCH 1/3] h264_ps: change decode_scaling_matrices so that it takes

[Benoit Fouet]

Hi,

On 27/06/2016 18:31, Michael Niedermayer wrote:

On Mon, Jun 27, 2016 at 02:38:50PM +0200, Benoit Fouet wrote:

  h264_ps.c |   11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)
ea8cc471972e1dbaa4f4f03cd7a5fe92a3b848e9  
0001-h264_ps-change-decode_scaling_matrices-so-that-it-ta.patch
 From c2606da98ecd04762305734f4f45ca8eaf266459 Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Mon, 27 Jun 2016 12:00:39 +0200
Subject: [PATCH 1/3] h264_ps: change decode_scaling_matrices so that it takes
  const {s,p}ps

In order to be able to make SPS const in H264ParamSets,
modify decode_scaling_matrices so that it returns if the scaling
matrix are present in the SPS, instead of altering the input SPS
structure.
---
  libavcodec/h264_ps.c | 11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)

please document the return value
LGTM otherwise


See if attached patch is OK in that regard.

Thanks,

--
Ben

From 67e714cbdabcb9ed808e8b10c70bbdf670cf3c2d Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Mon, 27 Jun 2016 12:00:39 +0200
Subject: [PATCH 1/3] h264_ps: change decode_scaling_matrices so that it takes
 const {s,p}ps

In order to be able to make SPS const in H264ParamSets,
modify decode_scaling_matrices so that it returns if the scaling
matrix are present in the SPS, instead of altering the input SPS
structure.
---
 libavcodec/h264_ps.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index 943d953..2f166c5 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -265,8 +265,9 @@ static void decode_scaling_list(GetBitContext *gb, uint8_t 
*factors, int size,
 }
 }
 
-static void decode_scaling_matrices(GetBitContext *gb, SPS *sps,
-PPS *pps, int is_sps,
+/* returns non zero if the provided SPS scaling matrix has been filled */
+static int decode_scaling_matrices(GetBitContext *gb, const SPS *sps,
+const PPS *pps, int is_sps,
 uint8_t(*scaling_matrix4)[16],
 uint8_t(*scaling_matrix8)[64])
 {
@@ -277,8 +278,9 @@ static void decode_scaling_matrices(GetBitContext *gb, SPS 
*sps,
 fallback_sps ? sps->scaling_matrix8[0] : default_scaling8[0],
 fallback_sps ? sps->scaling_matrix8[3] : default_scaling8[1]
 };
+int ret = 0;
 if (get_bits1(gb)) {
-sps->scaling_matrix_present |= is_sps;
+ret = is_sps;
 decode_scaling_list(gb, scaling_matrix4[0], 16, default_scaling4[0], 
fallback[0]);// Intra, Y
 decode_scaling_list(gb, scaling_matrix4[1], 16, default_scaling4[0], 
scaling_matrix4[0]); // Intra, Cr
 decode_scaling_list(gb, scaling_matrix4[2], 16, default_scaling4[0], 
scaling_matrix4[1]); // Intra, Cb
@@ -296,6 +298,8 @@ static void decode_scaling_matrices(GetBitContext *gb, SPS 
*sps,
 }
 }
 }
+
+return ret;
 }
 
 void ff_h264_ps_uninit(H264ParamSets *ps)
@@ -401,7 +405,7 @@ int ff_h264_decode_seq_parameter_set(GetBitContext *gb, 
AVCodecContext *avctx,
 goto fail;
 }
 sps->transform_bypass = get_bits1(gb);
-decode_scaling_matrices(gb, sps, NULL, 1,
+sps->scaling_matrix_present |= decode_scaling_matrices(gb, sps, NULL, 
1,
 sps->scaling_matrix4, sps->scaling_matrix8);
 } else {
 sps->chroma_format_idc = 1;
-- 
2.9.0.37.g6d523a3

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

[FFmpeg-devel] [PATCH 3/3] h264: make H264ParamSets sps const

[Benoit Fouet]

From 735362df589eb5f8b05063c56862ff18589475ad Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Tue, 21 Jun 2016 14:17:13 +0200
Subject: [PATCH 3/3] h264: make H264ParamSets sps const

---
 libavcodec/h264.h| 3 +--
 libavcodec/h264_parser.c | 2 +-
 libavcodec/h264_ps.c | 4 ++--
 libavcodec/h264_sei.c| 4 ++--
 libavcodec/h264_slice.c  | 4 ++--
 5 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/libavcodec/h264.h b/libavcodec/h264.h
index efe3555..ce4f8bf 100644
--- a/libavcodec/h264.h
+++ b/libavcodec/h264.h
@@ -234,8 +234,7 @@ typedef struct H264ParamSets {
 AVBufferRef *sps_ref;
 /* currently active parameters sets */
 const PPS *pps;
-// FIXME this should properly be const
-SPS *sps;
+const SPS *sps;
 } H264ParamSets;
 
 /**
diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index ce4bab2..7af2a8d 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -373,7 +373,7 @@ static inline int parse_nal_units(AVCodecParserContext *s,
"non-existing SPS %u referenced\n", p->ps.pps->sps_id);
 goto fail;
 }
-p->ps.sps = (SPS*)p->ps.sps_list[p->ps.pps->sps_id]->data;
+p->ps.sps = (const SPS*)p->ps.sps_list[p->ps.pps->sps_id]->data;
 
 sps = p->ps.sps;
 
diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index 0a97649..fdbe548 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -711,7 +711,7 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, 
AVCodecContext *avct
  H264ParamSets *ps, int bit_length)
 {
 AVBufferRef *pps_buf;
-SPS *sps;
+const SPS *sps;
 unsigned int pps_id = get_ue_golomb(gb);
 PPS *pps;
 int qp_bd_offset;
@@ -742,7 +742,7 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, 
AVCodecContext *avct
 ret = AVERROR_INVALIDDATA;
 goto fail;
 }
-sps = (SPS*)ps->sps_list[pps->sps_id]->data;
+sps = (const SPS*)ps->sps_list[pps->sps_id]->data;
 if (sps->bit_depth_luma > 14) {
 av_log(avctx, AV_LOG_ERROR,
"Invalid luma bit depth=%d\n",
diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
index 0bbd7e5..3bdbaa0 100644
--- a/libavcodec/h264_sei.c
+++ b/libavcodec/h264_sei.c
@@ -278,7 +278,7 @@ static int decode_buffering_period(H264SEIBufferingPeriod 
*h, GetBitContext *gb,
 {
 unsigned int sps_id;
 int sched_sel_idx;
-SPS *sps;
+const SPS *sps;
 
 sps_id = get_ue_golomb_31(gb);
 if (sps_id > 31 || !ps->sps_list[sps_id]) {
@@ -286,7 +286,7 @@ static int decode_buffering_period(H264SEIBufferingPeriod 
*h, GetBitContext *gb,
"non-existing SPS %d referenced in buffering period\n", sps_id);
 return sps_id > 31 ? AVERROR_INVALIDDATA : AVERROR_PS_NOT_FOUND;
 }
-sps = (SPS*)ps->sps_list[sps_id]->data;
+sps = (const SPS*)ps->sps_list[sps_id]->data;
 
 // NOTE: This is really so duplicated in the standard... See H.264, D.1.1
 if (sps->nal_hrd_parameters_present_flag) {
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index a470da6..afda9cd 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -356,7 +356,7 @@ int ff_h264_update_thread_context(AVCodecContext *dst,
 h->ps.sps_ref = av_buffer_ref(h1->ps.sps_ref);
 if (!h->ps.sps_ref)
 return AVERROR(ENOMEM);
-h->ps.sps = (SPS*)h->ps.sps_ref->data;
+h->ps.sps = (const SPS*)h->ps.sps_ref->data;
 }
 
 if (need_reinit || !inited) {
@@ -873,7 +873,7 @@ static enum AVPixelFormat get_pixel_format(H264Context *h, 
int force_callback)
 /* export coded and cropped frame dimensions to AVCodecContext */
 static int init_dimensions(H264Context *h)
 {
-SPS *sps = h->ps.sps;
+const SPS *sps = (const SPS*)h->ps.sps;
 int width  = h->width  - (sps->crop_right + sps->crop_left);
 int height = h->height - (sps->crop_top   + sps->crop_bottom);
 av_assert0(sps->crop_right + sps->crop_left < (unsigned)h->width);
-- 
2.9.0.37.g6d523a3

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

[FFmpeg-devel] [PATCH 2/3] h264: straighten dimensions check

[Benoit Fouet]

From 91b000bf2e0b01695803c5ef98cfb06590f5f409 Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Mon, 27 Jun 2016 13:31:21 +0200
Subject: [PATCH 2/3] h264: straighten dimensions check
 ff_h264_decode_seq_parameter_set

The MBS only flag was not taken into account when checking macroblock 
dimensions.
Also removes the unneeded check in init_dimensions for slices.
---
 libavcodec/h264_ps.c| 15 ---
 libavcodec/h264_slice.c | 17 -
 2 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index 5d4ddea..0a97649 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -463,13 +463,6 @@ int ff_h264_decode_seq_parameter_set(GetBitContext *gb, 
AVCodecContext *avctx,
 sps->gaps_in_frame_num_allowed_flag = get_bits1(gb);
 sps->mb_width   = get_ue_golomb(gb) + 1;
 sps->mb_height  = get_ue_golomb(gb) + 1;
-if ((unsigned)sps->mb_width  >= INT_MAX / 16 ||
-(unsigned)sps->mb_height >= INT_MAX / 16 ||
-av_image_check_size(16 * sps->mb_width,
-16 * sps->mb_height, 0, avctx)) {
-av_log(avctx, AV_LOG_ERROR, "mb_width/height overflow\n");
-goto fail;
-}
 
 sps->frame_mbs_only_flag = get_bits1(gb);
 if (!sps->frame_mbs_only_flag)
@@ -477,6 +470,14 @@ int ff_h264_decode_seq_parameter_set(GetBitContext *gb, 
AVCodecContext *avctx,
 else
 sps->mb_aff = 0;
 
+if ((unsigned)sps->mb_width  >= INT_MAX / 16 ||
+(unsigned)sps->mb_height >= INT_MAX / (16 * (2 - 
sps->frame_mbs_only_flag)) ||
+av_image_check_size(16 * sps->mb_width,
+16 * sps->mb_height * (2 - 
sps->frame_mbs_only_flag), 0, avctx)) {
+av_log(avctx, AV_LOG_ERROR, "mb_width/height overflow\n");
+goto fail;
+}
+
 sps->direct_8x8_inference_flag = get_bits1(gb);
 
 #ifndef ALLOW_INTERLACE
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 474400b..a470da6 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -889,23 +889,6 @@ static int init_dimensions(H264Context *h)
 height = h->avctx->height;
 }
 
-if (width <= 0 || height <= 0) {
-av_log(h->avctx, AV_LOG_ERROR, "Invalid cropped dimensions: %dx%d.\n",
-   width, height);
-if (h->avctx->err_recognition & AV_EF_EXPLODE)
-return AVERROR_INVALIDDATA;
-
-av_log(h->avctx, AV_LOG_WARNING, "Ignoring cropping information.\n");
-sps->crop_bottom =
-sps->crop_top=
-sps->crop_right  =
-sps->crop_left   =
-sps->crop= 0;
-
-width  = h->width;
-height = h->height;
-}
-
 h->avctx->coded_width  = h->width;
 h->avctx->coded_height = h->height;
 h->avctx->width= width;
-- 
2.9.0.37.g6d523a3

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

[FFmpeg-devel] [PATCH 1/3] h264_ps: change decode_scaling_matrices so that it takes

[Benoit Fouet]

From c2606da98ecd04762305734f4f45ca8eaf266459 Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Mon, 27 Jun 2016 12:00:39 +0200
Subject: [PATCH 1/3] h264_ps: change decode_scaling_matrices so that it takes
 const {s,p}ps

In order to be able to make SPS const in H264ParamSets,
modify decode_scaling_matrices so that it returns if the scaling
matrix are present in the SPS, instead of altering the input SPS
structure.
---
 libavcodec/h264_ps.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index 943d953..5d4ddea 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -265,8 +265,8 @@ static void decode_scaling_list(GetBitContext *gb, uint8_t 
*factors, int size,
 }
 }
 
-static void decode_scaling_matrices(GetBitContext *gb, SPS *sps,
-PPS *pps, int is_sps,
+static int decode_scaling_matrices(GetBitContext *gb, const SPS *sps,
+const PPS *pps, int is_sps,
 uint8_t(*scaling_matrix4)[16],
 uint8_t(*scaling_matrix8)[64])
 {
@@ -277,8 +277,9 @@ static void decode_scaling_matrices(GetBitContext *gb, SPS 
*sps,
 fallback_sps ? sps->scaling_matrix8[0] : default_scaling8[0],
 fallback_sps ? sps->scaling_matrix8[3] : default_scaling8[1]
 };
+int ret = 0;
 if (get_bits1(gb)) {
-sps->scaling_matrix_present |= is_sps;
+ret = is_sps;
 decode_scaling_list(gb, scaling_matrix4[0], 16, default_scaling4[0], 
fallback[0]);// Intra, Y
 decode_scaling_list(gb, scaling_matrix4[1], 16, default_scaling4[0], 
scaling_matrix4[0]); // Intra, Cr
 decode_scaling_list(gb, scaling_matrix4[2], 16, default_scaling4[0], 
scaling_matrix4[1]); // Intra, Cb
@@ -296,6 +297,8 @@ static void decode_scaling_matrices(GetBitContext *gb, SPS 
*sps,
 }
 }
 }
+
+return ret;
 }
 
 void ff_h264_ps_uninit(H264ParamSets *ps)
@@ -401,7 +404,7 @@ int ff_h264_decode_seq_parameter_set(GetBitContext *gb, 
AVCodecContext *avctx,
 goto fail;
 }
 sps->transform_bypass = get_bits1(gb);
-decode_scaling_matrices(gb, sps, NULL, 1,
+sps->scaling_matrix_present |= decode_scaling_matrices(gb, sps, NULL, 
1,
 sps->scaling_matrix4, sps->scaling_matrix8);
 } else {
 sps->chroma_format_idc = 1;
-- 
2.9.0.37.g6d523a3

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

[FFmpeg-devel] [PATCH] H264ParamSets: make sps const

[Benoit Fouet]

Hi,

First patch change decode_scaling_matrices function, so that it does not 
affect the SPS structure it gets, and marks SPS and PPS const in its 
arguments.


Second patch straightens the check for macroblock sizes in 
ff_h264_decode_seq_parameter_set and removes the unneeded check in H.264 
slice init_dimensions.


Last patch make SPS const in H264ParamSets and fixes its usages.

Cheers,
--
Ben

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

Re: [FFmpeg-devel] [PATCH] h264: make H264ParamSets sps const

[Benoit Fouet]

Hi,

On 25/06/2016 14:15, Clément Bœsch wrote:

On Fri, Jun 24, 2016 at 09:20:35AM +0200, Benoit Fouet wrote:
[...]

Any objection to this patch now?

iam ok with the patch, maybe give others a bit of time to reply
before applying though

Yeah, I'm in no hurry, I just saw this FIXME in the context of one of
Mateo's patches.
I was more waiting for some feedback from Hendrik or Clément, anyway, as
they were the ones raising the points.

Should be fine if indeed the following computation:

 int width  = 16 * sps->mb_width;
 int height = 16 * sps->mb_height * (2 - sps->frame_mbs_only_flag);

...is always correct.


Good one. It seems the check that is done today on the height part is 
not enough.

I've also found another place which was writing to the SPS and shouldn't.
I'm sending a new patchset for all this.

Thanks for the review,
--
Ben

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

Re: [FFmpeg-devel] [PATCH] MAINTAINERS: drop people who do not appear in git history > 2013

[Benoit Fouet]

aenc.cDavid Conrad
-  libvorbis.c   David Conrad
libvpx*   James Zern
-  libxavs.c Stefan Gehrer
libzvbi-teletextdec.c Marton Balint
lzo.h, lzo.c  Reimar Doeffinger
mdec.cMichael Niedermayer
@@ -201,56 +178,34 @@ Codecs:
mpegvideo.c, mpegvideo.h  Michael Niedermayer
mqc*  Nicolas Bertrand
msmpeg4.c, msmpeg4data.h  Michael Niedermayer
-  msrle.c   Mike Melanson
-  msvideo1.cMike Melanson
nuv.c Reimar Doeffinger
nvenc*Timo Rothenpieler
paf.* Paul B Mahol
-  pcx.c Ivo van Poorten
pgssubdec.c   Reimar Doeffinger
-  ptx.c Ivo van Poorten
qcelp*Reynaldo H. Verdejo Pinochet
qdm2.c, qdm2data.hRoberto Togni
qsv*  Ivan Uskov
-  qtrle.c   Mike Melanson
ra144.c, ra144.h, ra288.c, ra288.hRoberto Togni
resample2.c   Michael Niedermayer
-  rl2.c Sascha Sommer
rpza.cRoberto Togni
rtjpeg.c, rtjpeg.hReimar Doeffinger
rv10.cMichael Niedermayer
rv4*  Christophe Gisquet
-  s3tc* Ivo van Poorten
-  smc.c Mike Melanson
smvjpegdec.c  Ash Hughes
snow* Michael Niedermayer, Loren Merritt
-  sonic.c   Alex Beregszaszi
-  srt*  Aurelien Jacobs
-  sunrast.c Ivo van Poorten
svq3.cMichael Niedermayer
tak*  Paul B Mahol
-  truemotion1*  Mike Melanson
-  tta.c Alex Beregszaszi, Jaikrishnan Menon
ttaenc.c  Paul B Mahol
-  txd.c Ivo van Poorten
vc1*  Christophe Gisquet
vc2*  Rostislav Pehlivanov
vcr1.cMichael Niedermayer
vda_h264_dec.cXidorn Quan
videotoolboxenc.c Rick Kern
vima.cPaul B Mahol
-  vorbisdec.c   Denes Balatoni, David Conrad
-  vorbisenc.c   Oded Shimon
-  vp3*  Mike Melanson
-  vp5   Aurelien Jacobs
-  vp6   Aurelien Jacobs
-  vp8   David Conrad, Ronald Bultje
+  vp8   Ronald Bultje
vp9   Ronald Bultje
-  vqavideo.cMike Melanson
-  wmaprodec.c   Sascha Sommer
wmavoice.cRonald S. Bultje
wmv2.cMichael Niedermayer
-  xan.c Mike Melanson
xbm*  Paul B Mahol
xface Stefano Sabatini
xvmc.cIvan Kalvachev
@@ -258,7 +213,7 @@ Codecs:
  
  Hardware acceleration:

crystalhd.c   Philip Langdale
-  dxva2*Hendrik Leppkes, Laurent Aimar
+  dxva2*Hendrik Leppkes
mediacodec*   Matthieu Bouron
vaapi*Gwenole Beauchesne
vaapi_encode* Mark Thompson
@@ -361,14 +316,11 @@ Generic parts:
  
  
  Muxers/Demuxers:

-  4xm.c Mike Melanson
aadec.c   Vesselin Bontchev (vesselin.bontchev 
at yandex dot com)
-  adtsenc.c Robert Swain
afc.c Paul B Mahol
-  aiffdec.c Baptiste Coudurier, Matthieu Bouron
-  aiffenc.c Baptiste Coudurier, Matthieu Bouron
+  aiffdec.c Matthieu Bouron
+  aiffenc.c Matthieu Bouron
apngdec.c     Benoit Fouet
-  ass*  Aurelien Jacobs
astdec.c  Paul B Mahol
astenc.c  James Almer
 

Re: [FFmpeg-devel] [PATCH] h264: make H264ParamSets sps const

[Benoit Fouet]

Hi,

On 23/06/2016 22:37, Michael Niedermayer wrote:

On Thu, Jun 23, 2016 at 03:28:10PM +0200, Benoit Fouet wrote:

Hi,


On 21/06/2016 16:42, Benoit Fouet wrote:

Hi,

On 21/06/2016 16:29, Hendrik Leppkes wrote:

On Tue, Jun 21, 2016 at 4:20 PM, Benoit Fouet
<benoit.fo...@free.fr> wrote:

Hi,


On 21/06/2016 14:52, Hendrik Leppkes wrote:

On Tue, Jun 21, 2016 at 2:40 PM, Clément Bœsch <u...@pkh.me> wrote:

On Tue, Jun 21, 2016 at 02:34:33PM +0200, Benoit Fouet wrote:

Hi,

Unless I totally missed something, the FIXME in
H264ParamSets structure
should be fixed by attached patch.

--
Ben

  From 28ae10498f81070539bdb8f40236326743350101 Mon Sep
17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Tue, 21 Jun 2016 14:17:13 +0200
Subject: [PATCH] h264: make H264ParamSets sps const

---
   libavcodec/h264.h   | 3 +--
   libavcodec/h264_slice.c | 2 +-
   2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/libavcodec/h264.h b/libavcodec/h264.h
index c4d2921..b809ee5 100644
--- a/libavcodec/h264.h
+++ b/libavcodec/h264.h
@@ -234,8 +234,7 @@ typedef struct H264ParamSets {
   AVBufferRef *sps_ref;
   /* currently active parameters sets */
   const PPS *pps;
-// FIXME this should properly be const
-SPS *sps;
+const SPS *sps;
   } H264ParamSets;

   /**
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 6e7b940..da7f9dd 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -873,7 +873,7 @@ static enum AVPixelFormat
get_pixel_format(H264Context *h, int force_callback)
   /* export coded and cropped frame dimensions to AVCodecContext */
   static int init_dimensions(H264Context *h)
   {
-SPS *sps = h->ps.sps;
+SPS *sps = (SPS*)h->ps.sps_ref->data;
   int width  = h->width  - (sps->crop_right + sps->crop_left);
   int height = h->height - (sps->crop_top   +
sps->crop_bottom);
   av_assert0(sps->crop_right + sps->crop_left <
(unsigned)h->width);

So it's not actually const, right?


Indeed, the FIXME wasn't just there because someone forgot to write
"const" in front of it, but because it was used in some parts as
not-const.

OK, right... Thanks for reminding me of reading the code better before
sending a patch.

As far as I can see, the only place where this constness is
not preserved is
in the init_dimensions function (in h264_slice), in a dead
part of the code,
as crop is asserted at the beginning of the very same function.
Please correct me if I've missed other places.


If anything the asserts should probably be removed, because bad files
should never be able to trigger assertions, and the existing check
remain.

Well, the SPS "decoder" already takes care of the check (see
ff_h264_decode_seq_parameter_set).
So I could remove the check, because it seems useless, instead of
removing it because "bad things happen", what do you think?


Any objection to this patch now?

iam ok with the patch, maybe give others a bit of time to reply
before applying though


Yeah, I'm in no hurry, I just saw this FIXME in the context of one of 
Mateo's patches.
I was more waiting for some feedback from Hendrik or Clément, anyway, as 
they were the ones raising the points.


--
Ben

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

Re: [FFmpeg-devel] [PATCH] h264: make H264ParamSets sps const

[Benoit Fouet]

Hi,


On 21/06/2016 16:42, Benoit Fouet wrote:

Hi,

On 21/06/2016 16:29, Hendrik Leppkes wrote:
On Tue, Jun 21, 2016 at 4:20 PM, Benoit Fouet <benoit.fo...@free.fr> 
wrote:

Hi,


On 21/06/2016 14:52, Hendrik Leppkes wrote:

On Tue, Jun 21, 2016 at 2:40 PM, Clément Bœsch <u...@pkh.me> wrote:

On Tue, Jun 21, 2016 at 02:34:33PM +0200, Benoit Fouet wrote:

Hi,

Unless I totally missed something, the FIXME in H264ParamSets 
structure

should be fixed by attached patch.

--
Ben

  From 28ae10498f81070539bdb8f40236326743350101 Mon Sep 17 
00:00:00 2001

From: Benoit Fouet <benoit.fo...@free.fr>
Date: Tue, 21 Jun 2016 14:17:13 +0200
Subject: [PATCH] h264: make H264ParamSets sps const

---
   libavcodec/h264.h   | 3 +--
   libavcodec/h264_slice.c | 2 +-
   2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/libavcodec/h264.h b/libavcodec/h264.h
index c4d2921..b809ee5 100644
--- a/libavcodec/h264.h
+++ b/libavcodec/h264.h
@@ -234,8 +234,7 @@ typedef struct H264ParamSets {
   AVBufferRef *sps_ref;
   /* currently active parameters sets */
   const PPS *pps;
-// FIXME this should properly be const
-SPS *sps;
+const SPS *sps;
   } H264ParamSets;

   /**
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 6e7b940..da7f9dd 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -873,7 +873,7 @@ static enum AVPixelFormat
get_pixel_format(H264Context *h, int force_callback)
   /* export coded and cropped frame dimensions to AVCodecContext */
   static int init_dimensions(H264Context *h)
   {
-SPS *sps = h->ps.sps;
+SPS *sps = (SPS*)h->ps.sps_ref->data;
   int width  = h->width  - (sps->crop_right + sps->crop_left);
   int height = h->height - (sps->crop_top   + 
sps->crop_bottom);
   av_assert0(sps->crop_right + sps->crop_left < 
(unsigned)h->width);

So it's not actually const, right?


Indeed, the FIXME wasn't just there because someone forgot to write
"const" in front of it, but because it was used in some parts as
not-const.


OK, right... Thanks for reminding me of reading the code better before
sending a patch.

As far as I can see, the only place where this constness is not 
preserved is
in the init_dimensions function (in h264_slice), in a dead part of 
the code,

as crop is asserted at the beginning of the very same function.
Please correct me if I've missed other places.


If anything the asserts should probably be removed, because bad files
should never be able to trigger assertions, and the existing check
remain.


Well, the SPS "decoder" already takes care of the check (see 
ff_h264_decode_seq_parameter_set).
So I could remove the check, because it seems useless, instead of 
removing it because "bad things happen", what do you think?




Any objection to this patch now?

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

Re: [FFmpeg-devel] [PATCH] h264: make H264ParamSets sps const

[Benoit Fouet]

Hi,

On 21/06/2016 14:52, Hendrik Leppkes wrote:

On Tue, Jun 21, 2016 at 2:40 PM, Clément Bœsch <u...@pkh.me> wrote:

On Tue, Jun 21, 2016 at 02:34:33PM +0200, Benoit Fouet wrote:

Hi,

Unless I totally missed something, the FIXME in H264ParamSets structure
should be fixed by attached patch.

--
Ben

 From 28ae10498f81070539bdb8f40236326743350101 Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Tue, 21 Jun 2016 14:17:13 +0200
Subject: [PATCH] h264: make H264ParamSets sps const

---
  libavcodec/h264.h   | 3 +--
  libavcodec/h264_slice.c | 2 +-
  2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/libavcodec/h264.h b/libavcodec/h264.h
index c4d2921..b809ee5 100644
--- a/libavcodec/h264.h
+++ b/libavcodec/h264.h
@@ -234,8 +234,7 @@ typedef struct H264ParamSets {
  AVBufferRef *sps_ref;
  /* currently active parameters sets */
  const PPS *pps;
-// FIXME this should properly be const
-SPS *sps;
+const SPS *sps;
  } H264ParamSets;

  /**
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 6e7b940..da7f9dd 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -873,7 +873,7 @@ static enum AVPixelFormat get_pixel_format(H264Context *h, 
int force_callback)
  /* export coded and cropped frame dimensions to AVCodecContext */
  static int init_dimensions(H264Context *h)
  {
-SPS *sps = h->ps.sps;
+SPS *sps = (SPS*)h->ps.sps_ref->data;
  int width  = h->width  - (sps->crop_right + sps->crop_left);
  int height = h->height - (sps->crop_top   + sps->crop_bottom);
  av_assert0(sps->crop_right + sps->crop_left < (unsigned)h->width);

So it's not actually const, right?


Indeed, the FIXME wasn't just there because someone forgot to write
"const" in front of it, but because it was used in some parts as
not-const.


OK, right... Thanks for reminding me of reading the code better before 
sending a patch.


As far as I can see, the only place where this constness is not 
preserved is in the init_dimensions function (in h264_slice), in a dead 
part of the code, as crop is asserted at the beginning of the very same 
function.

Please correct me if I've missed other places.

Updated patch attached (which I can very well split in two different 
ones if it's (more) correct).


Thanks,
--
Ben

From cc56a2f787c64ca4051be9b53dd2e4e9b472339c Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Tue, 21 Jun 2016 14:17:13 +0200
Subject: [PATCH] h264: make H264ParamSets sps const

Only init_dimension was abusing the data, in a case that is asserted can
never happen.
---
 libavcodec/h264.h|  3 +--
 libavcodec/h264_parser.c |  2 +-
 libavcodec/h264_ps.c |  2 +-
 libavcodec/h264_sei.c|  4 ++--
 libavcodec/h264_slice.c  | 21 ++---
 5 files changed, 7 insertions(+), 25 deletions(-)

diff --git a/libavcodec/h264.h b/libavcodec/h264.h
index c4d2921..b809ee5 100644
--- a/libavcodec/h264.h
+++ b/libavcodec/h264.h
@@ -234,8 +234,7 @@ typedef struct H264ParamSets {
 AVBufferRef *sps_ref;
 /* currently active parameters sets */
 const PPS *pps;
-// FIXME this should properly be const
-SPS *sps;
+const SPS *sps;
 } H264ParamSets;
 
 /**
diff --git a/libavcodec/h264_parser.c b/libavcodec/h264_parser.c
index 42ad932..76ed2a3 100644
--- a/libavcodec/h264_parser.c
+++ b/libavcodec/h264_parser.c
@@ -373,7 +373,7 @@ static inline int parse_nal_units(AVCodecParserContext *s,
"non-existing SPS %u referenced\n", p->ps.pps->sps_id);
 goto fail;
 }
-p->ps.sps = (SPS*)p->ps.sps_list[p->ps.pps->sps_id]->data;
+p->ps.sps = (const SPS*)p->ps.sps_list[p->ps.pps->sps_id]->data;
 
 sps = p->ps.sps;
 
diff --git a/libavcodec/h264_ps.c b/libavcodec/h264_ps.c
index fb05b05..3867918 100644
--- a/libavcodec/h264_ps.c
+++ b/libavcodec/h264_ps.c
@@ -738,7 +738,7 @@ int ff_h264_decode_picture_parameter_set(GetBitContext *gb, 
AVCodecContext *avct
 ret = AVERROR_INVALIDDATA;
 goto fail;
 }
-sps = (SPS*)ps->sps_list[pps->sps_id]->data;
+sps = (const SPS*)ps->sps_list[pps->sps_id]->data;
 if (sps->bit_depth_luma > 14) {
 av_log(avctx, AV_LOG_ERROR,
"Invalid luma bit depth=%d\n",
diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
index d0596dc..fedb172 100644
--- a/libavcodec/h264_sei.c
+++ b/libavcodec/h264_sei.c
@@ -276,7 +276,7 @@ static int decode_buffering_period(H264SEIBufferingPeriod 
*h, GetBitContext *gb,
 {
 unsigned int sps_id;
 int sched_sel_idx;
-SPS *sps;
+const SPS *sps;
 
 sps_id = get_ue_golomb_31(gb);
 if (sps_id > 31 || !ps->sps_list[sps_id]) {
@@ -284,7 +284,7 @@ static int decode_buffering_period(H264SEIBufferingPeriod 
*h, GetBitContext *gb

[FFmpeg-devel] [PATCH] h264: make H264ParamSets sps const

[Benoit Fouet]

Hi,

Unless I totally missed something, the FIXME in H264ParamSets structure 
should be fixed by attached patch.


--
Ben

From 28ae10498f81070539bdb8f40236326743350101 Mon Sep 17 00:00:00 2001
From: Benoit Fouet <benoit.fo...@free.fr>
Date: Tue, 21 Jun 2016 14:17:13 +0200
Subject: [PATCH] h264: make H264ParamSets sps const

---
 libavcodec/h264.h   | 3 +--
 libavcodec/h264_slice.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/libavcodec/h264.h b/libavcodec/h264.h
index c4d2921..b809ee5 100644
--- a/libavcodec/h264.h
+++ b/libavcodec/h264.h
@@ -234,8 +234,7 @@ typedef struct H264ParamSets {
 AVBufferRef *sps_ref;
 /* currently active parameters sets */
 const PPS *pps;
-// FIXME this should properly be const
-SPS *sps;
+const SPS *sps;
 } H264ParamSets;
 
 /**
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 6e7b940..da7f9dd 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -873,7 +873,7 @@ static enum AVPixelFormat get_pixel_format(H264Context *h, 
int force_callback)
 /* export coded and cropped frame dimensions to AVCodecContext */
 static int init_dimensions(H264Context *h)
 {
-SPS *sps = h->ps.sps;
+SPS *sps = (SPS*)h->ps.sps_ref->data;
 int width  = h->width  - (sps->crop_right + sps->crop_left);
 int height = h->height - (sps->crop_top   + sps->crop_bottom);
 av_assert0(sps->crop_right + sps->crop_left < (unsigned)h->width);
-- 
2.9.0.rc2

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

Re: [FFmpeg-devel] [PATCH] lavd/v4l2: allow devices not implementing VIDIOC_G_PARM

[Benoit Fouet]

Hi,


On 15/06/2016 17:21, Niklas Söderlund wrote:

Not all v4l2 devices implement the VIDIOC_G_PARM ioctl. This patch allow
ffmpeg to open such device and treat it the same as devices that do
implement the ioctl but returns that it do not implement the
V4L2_CAP_TIMEPERFRAME capability.

Signed-off-by: Niklas Söderlund 
---
  libavdevice/v4l2.c | 7 ++-
  1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
index 103fb10..c8915e0 100644
--- a/libavdevice/v4l2.c
+++ b/libavdevice/v4l2.c
@@ -715,11 +715,8 @@ static int v4l2_set_parameters(AVFormatContext *ctx)
  streamparm.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
  if (v4l2_ioctl(s->fd, VIDIOC_G_PARM, ) < 0) {
  ret = AVERROR(errno);
-av_log(ctx, AV_LOG_ERROR, "ioctl(VIDIOC_G_PARM): %s\n", 
av_err2str(ret));
-return ret;
-}
-
-if (framerate_q.num && framerate_q.den) {
+av_log(ctx, AV_LOG_WARNING, "ioctl(VIDIOC_G_PARM): %s\n", 
av_err2str(ret));
+} else if (framerate_q.num && framerate_q.den) {
  if (streamparm.parm.capture.capability & V4L2_CAP_TIMEPERFRAME) {
  tpf = 
  


LGTM

--
Ben

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

Re: [FFmpeg-devel] [PATCH] hls muxer doc: clarify segment splitting option

[Benoit Fouet]

Hi,

Le 08/06/2016 11:46, Benoit Fouet a écrit :

Hi,

find attached a patch to $subj
This would have been useful at least to me :-)



Anyone against this patch?
If not, can someone please apply it?

Thanks,
--
Ben


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

Re: [FFmpeg-devel] [FFmpeg-cvslog] lavf/os_support.h: Fix for unicode filenames on windows.

[Benoit Fouet]

Hi,


On 13/06/2016 10:21, Clément Bœsch wrote:

On Mon, Jun 13, 2016 at 05:50:18AM +0200, Matt Oliver wrote:

ffmpeg | branch: master | Matt Oliver  | Mon Jun  6 
17:04:39 2016 +1000| [37787f261639c53998487400e874741c17e85fc6] | committer: Matt 
Oliver

lavf/os_support.h: Fix for unicode filenames on windows.

Fixes #819 #5256 #5281

Signed-off-by: Matt Oliver 


http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=37787f261639c53998487400e874741c17e85fc6

---

  libavformat/file.c   |4 
  libavformat/os_support.h |   24 
  2 files changed, 28 insertions(+)

diff --git a/libavformat/file.c b/libavformat/file.c
index 5765ce7..264542a 100644
--- a/libavformat/file.c
+++ b/libavformat/file.c
@@ -148,7 +148,11 @@ static int file_check(URLContext *h, int mask)
  ret |= AVIO_FLAG_WRITE;
  #else
  struct stat st;
+#   ifndef _WIN32
  ret = stat(filename, );
+#   else
+ret = win32_stat(filename, );
+#   endif

why this chunk?


  if (ret < 0)
  return AVERROR(errno);
  
diff --git a/libavformat/os_support.h b/libavformat/os_support.h

index a332911..9e312a5 100644
--- a/libavformat/os_support.h
+++ b/libavformat/os_support.h
@@ -182,6 +182,29 @@ DEF_FS_FUNCTION(unlink, _wunlink, _unlink)
  DEF_FS_FUNCTION(mkdir,  _wmkdir,  _mkdir)
  DEF_FS_FUNCTION(rmdir,  _wrmdir , _rmdir)
  
+#define DEF_FS_FUNCTION2(name, wfunc, afunc, partype) \

+static inline int win32_##name(const char *filename_utf8, partype par) \
+{ \
+wchar_t *filename_w;  \
+int ret;  \
+  \
+if (utf8towchar(filename_utf8, _w))  \
+return -1;\
+if (!filename_w)  \
+goto fallback;\
+  \
+ret = wfunc(filename_w, par); \
+av_free(filename_w);  \
+return ret;   \
+  \
+fallback: \
+/* filename may be be in CP_ACP */\
+return afunc(filename_utf8, par); \
+}
+
+DEF_FS_FUNCTION2(access, _waccess, _access, int)
+DEF_FS_FUNCTION2(stat, _wstat64, _stat64, struct stat*)
+
  static inline int win32_rename(const char *src_utf8, const char *dest_utf8)
  {
  wchar_t *src_w, *dest_w;
@@ -231,6 +254,7 @@ fallback:
  #define rename  win32_rename
  #define rmdir   win32_rmdir
  #define unlink  win32_unlink
+#define access  win32_access
  

...instead of #define stat win32_stat here?


as already noted by someone else, this should be
#define stat(a, b) win32_stat((a), (b))
in order not to conflict with "struct stat" definition.

--
Ben

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

Re: [FFmpeg-devel] [VOTE] Ban Carl Eugen Hoyos

[Benoit Fouet]

Hi,


On 12/06/2016 22:58, Paul B Mahol wrote:

Hi,

As requested in the IRC meeting I hereby request for the
voting committee to begin voting on whatever to ban Carl
Eugen Hoyos from mailing list, trac and IRC for 4 months,
starting after the voting has finished.

Voting will last 7 days from now.
In order for the vote to pass, at least 50% of all votes
from committee need to agree to do so.



Disclaimer: I'm not part of this committee


All developers and users are welcome to write about their
experiences with Carl.



My feeling here is that all the things that have been discussed about 
the CoC are a consequence of Carl's behaviour on the ML.
I can understand (I think) your feelings, but this is still problematic 
to me as a rule of thumb. It does not seem fair to decide on a CoC 
because of certain facts and then rule on those very facts on the charge 
that they fail to follow the rules that they have led to create.
In addition to this, I really feel that 4 months is way too long for a 
ban, as in "forever".


In short, I feel that this is unfair and too long a period, though I can 
understand your feelings on this.


Cheers,
--
Ben

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

Re: [FFmpeg-devel] [PATCH] lavf/mpegtsenc: move putstr8 definition up

[Benoit Fouet]

Hi,

Le 19/05/2016 18:45, Stefano Sabatini a écrit :

This allows to use the function in a future commit.
---
  libavformat/mpegtsenc.c | 34 +-
  1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index 93cbac1..5f22032 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -253,6 +253,23 @@ static void mpegts_write_pat(AVFormatContext *s)
data, q - data);
  }
  
+/* NOTE: !str is accepted for an empty string */

+static void putstr8(uint8_t **q_ptr, const char *str)
+{
+uint8_t *q;
+int len;
+
+q = *q_ptr;
+if (!str)
+len = 0;
+else
+len = strlen(str);
+*q++ = len;
+memcpy(q, str, len);


Side note on this one, unrelated to your change.
Isn't this undefined behavior to call memcpy with one of the pointers 
being NULL?


--
Ben

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

Re: [FFmpeg-devel] [PATCH] doc/developer.texi: Add a code of conduct

[Benoit Fouet]

Hi,

Le 18 mai 2016 20:40:08 GMT+02:00, Michael Niedermayer  
a écrit :
>Signed-off-by: Michael Niedermayer 
>---
> doc/developer.texi |   29 +
> 1 file changed, 29 insertions(+)
>
>diff --git a/doc/developer.texi b/doc/developer.texi
>index 6db93ce..4d3a7ae 100644
>--- a/doc/developer.texi
>+++ b/doc/developer.texi
>@@ -403,6 +403,35 @@ finding a new maintainer and also don't forget to
>update the @file{MAINTAINERS}
> 
> We think our rules are not too hard. If you have comments, contact us.
> 
>+@section Code of conduct
>+
>+Be friendly and respectful towards others and third parties.
>+Treat others the way you yourself want to be treated.
>+
>+Be considerate. Not everyone shares the same viewpoint and priorities
>as you do.
>+Different opinions and interpretations help the project.
>+Looking at issues from a different perspective assists development.
>+
>+Do not assume malice for things that can be attributed to
>incompetence. Even if
>+it is malice, it's rarely good to start with that as initial
>assumption.
>+
>+Stay friendly even if someone acts contrarily. Everyone has a bad day
>+once in a while.
>+If you yourself have a bad day or are angry then try to take a break
>and reply
>+once you are calm and without anger if you have to.
>+
>+Try to help other team members and cooperate if you can.
>+
>+The goal of software development is to create technical excellence,
>not for any
>+individual to be better and "win" against the others. Large software
>projects
>+are only possible and successful through teamwork.
>+
>+If someone struggles do not put them down. Give them a helping hand
>+instead and point them in the right direction.
>+
>+Finally, keep in mind the immortal words of Bill and Ted,
>+"Be excellent to each other."
>+
> @anchor{Submitting patches}
> @section Submitting patches
> 

FWIW, fine by me too.

-- 
Ben 


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

Re: [FFmpeg-devel] [PATCH] swresample/arm: add ff_resample_common_apply_filter_{x4, x8}_{float, s16}_neon

[Benoit Fouet]

Hi,

On 12/05/2016 15:22, Matthieu Bouron wrote:

On Thu, May 12, 2016 at 10:01 AM, Benoit Fouet <benoit.fo...@free.fr> wrote:


Hi,

I mostly have nits remarks.

On 11/05/2016 18:39, Matthieu Bouron wrote:


From: Matthieu Bouron <matthieu.bou...@stupeflix.com>



[...]

diff --git a/libswresample/arm/resample.S b/libswresample/arm/resample.S

new file mode 100644
index 000..13462e3
--- /dev/null
+++ b/libswresample/arm/resample.S
@@ -0,0 +1,77 @@

[...]

+function ff_resample_common_apply_filter_x4_float_neon, export=1
+vmov.f32q0, #0.0   @
accumulator
+1:  vld1.32 {q1}, [r1]!@
src
+vld1.32 {q2}, [r2]!@
filter
+vmla.f32q0, q1, q2 @
src + {0..3} * filter + {0..3}


nit: the comment could be "accu += src[0..3] . filter[0..3]"
same for the other ones below

[...]

+subsr3, #4 @

filter_length -= 4
+bgt 1b @
loop until filter_length
+vpadd.f32   d0, d0, d1 @
pair adding of the 4x32-bit accumulated values
+vpadd.f32   d0, d0, d0 @
pair adding of the 4x32-bit accumulator values
+vst1.32 {d0[0]}, [r0]  @
write accumulator
+mov pc, lr
+endfunc
+
+function ff_resample_common_apply_filter_x8_float_neon, export=1
+vmov.f32q0, #0.0   @
accumulator
+1:  vld1.32 {q1}, [r1]!@
src1
+vld1.32 {q2}, [r2]!@
filter1
+vld1.32 {q8}, [r1]!@
src2
+vld1.32 {q9}, [r2]!@
filter2
+vmla.f32q0, q1, q2 @
src1 + {0..3} * filter1 + {0..3}
+vmla.f32q0, q8, q9 @
src2 + {0..3} * filter2 + {0..3}


instead of using src1 and src2, you may want to use src[0..3] and src[4..7]
so, if I reuse the formulation I proposed above:
accu += src[0..3] . filter[0..3]
accu += src[4..7] . filter[4..7]


Fixed locally (as well as the other case you mentionned) with:
-vmla.f32q0, q1, q2 @
src1 + {0..3} * filter1 + {0..3}
-vmla.f32q0, q8, q9 @
src2 + {0..3} * filter2 + {0..3}
+vmla.f32q0, q1, q2 @
accumulator += src1 + {0..3} * filter1 + {0..3}
+vmla.f32q0, q8, q9 @
accumulator += src2 + {4..7} * filter2 + {4..7}

I prefer to use + {0..3} instead of [0..3] to make the comments consistent
with what has been done in swscale/arm.



Fine for me (I chose the "[]" notation to be consistent with the "." 
notation also, in order to do as if it were a dot product between two 
vectors).



+subsr3, #8 @

filter_length -= 4


-= 8


Fixed locally.



[...]

diff --git a/libswresample/arm/resample_init.c

b/libswresample/arm/resample_init.c
new file mode 100644
index 000..c817d03
--- /dev/null
+++ b/libswresample/arm/resample_init.c

[...]

+static int ff_resample_common_##TYPE##_neon(ResampleContext *c, void
*dest, const void *source,   \
+int n, int update_ctx)
   \
+{
  \
+DELEM *dst = dest;
   \
+const DELEM *src = source;
   \
+int dst_index;
   \
+int index= c->index;
   \
+int frac= c->frac;
   \
+int sample_index = index >> c->phase_shift;
  \
+int x4_aligned_filter_length = c->filter_length & ~3;
  \
+int x8_aligned_filter_length = c->filter_length & ~7;
  \
+
   \
+index &= c->phase_mask;
  \
+for (dst_index = 0; dst_index < n; dst_index++) {
  \
+FELEM *filter = ((FELEM *) c->filter_bank) + c->filter_alloc *
index; \
+
   \
+FELEM2 val=0;
  \
+int i = 0;
   \
+if (x8_aligned_filter_length >= 8) {
   \
+ff_resample_common_apply_filter_x8_##TYPE##_neon(,
[sample_index],\
+

Re: [FFmpeg-devel] [PATCH] swresample/arm: add ff_resample_common_apply_filter_{x4, x8}_{float, s16}_neon

[Benoit Fouet]

Hi,

I mostly have nits remarks.

On 11/05/2016 18:39, Matthieu Bouron wrote:

From: Matthieu Bouron 



[...]


diff --git a/libswresample/arm/resample.S b/libswresample/arm/resample.S
new file mode 100644
index 000..13462e3
--- /dev/null
+++ b/libswresample/arm/resample.S
@@ -0,0 +1,77 @@

[...]

+function ff_resample_common_apply_filter_x4_float_neon, export=1
+vmov.f32q0, #0.0   @ 
accumulator
+1:  vld1.32 {q1}, [r1]!@ src
+vld1.32 {q2}, [r2]!@ filter
+vmla.f32q0, q1, q2 @ src + 
{0..3} * filter + {0..3}


nit: the comment could be "accu += src[0..3] . filter[0..3]"
same for the other ones below

[...]


+subsr3, #4 @ 
filter_length -= 4
+bgt 1b @ loop 
until filter_length
+vpadd.f32   d0, d0, d1 @ pair 
adding of the 4x32-bit accumulated values
+vpadd.f32   d0, d0, d0 @ pair 
adding of the 4x32-bit accumulator values
+vst1.32 {d0[0]}, [r0]  @ write 
accumulator
+mov pc, lr
+endfunc
+
+function ff_resample_common_apply_filter_x8_float_neon, export=1
+vmov.f32q0, #0.0   @ 
accumulator
+1:  vld1.32 {q1}, [r1]!@ src1
+vld1.32 {q2}, [r2]!@ 
filter1
+vld1.32 {q8}, [r1]!@ src2
+vld1.32 {q9}, [r2]!@ 
filter2
+vmla.f32q0, q1, q2 @ src1 
+ {0..3} * filter1 + {0..3}
+vmla.f32q0, q8, q9 @ src2 
+ {0..3} * filter2 + {0..3}


instead of using src1 and src2, you may want to use src[0..3] and src[4..7]
so, if I reuse the formulation I proposed above:
accu += src[0..3] . filter[0..3]
accu += src[4..7] . filter[4..7]


+subsr3, #8 @ 
filter_length -= 4


-= 8

[...]


diff --git a/libswresample/arm/resample_init.c 
b/libswresample/arm/resample_init.c
new file mode 100644
index 000..c817d03
--- /dev/null
+++ b/libswresample/arm/resample_init.c

[...]

+static int ff_resample_common_##TYPE##_neon(ResampleContext *c, void *dest, 
const void *source,   \
+int n, int update_ctx) 
   \
+{  
   \
+DELEM *dst = dest; 
   \
+const DELEM *src = source; 
   \
+int dst_index; 
   \
+int index= c->index;   
   \
+int frac= c->frac; 
   \
+int sample_index = index >> c->phase_shift;
   \
+int x4_aligned_filter_length = c->filter_length & ~3;  
   \
+int x8_aligned_filter_length = c->filter_length & ~7;  
   \
+   
   \
+index &= c->phase_mask;
   \
+for (dst_index = 0; dst_index < n; dst_index++) {  
   \
+FELEM *filter = ((FELEM *) c->filter_bank) + c->filter_alloc * index;  
   \
+   
   \
+FELEM2 val=0;  
   \
+int i = 0; 
   \
+if (x8_aligned_filter_length >= 8) {   
   \
+ff_resample_common_apply_filter_x8_##TYPE##_neon(, 
[sample_index],\
+ filter, 
x8_aligned_filter_length);   \
+i += x8_aligned_filter_length; 
   \
+   
   \
+} else if (x4_aligned_filter_length >= 4) {   

Re: [FFmpeg-devel] [PATCH] swscale/arm: add yuv2planeX_8_neon

[Benoit Fouet]

Hi,

(again, thanks to both of you for documenting all this assembly /NEON code)

On 09/04/2016 10:22, Matthieu Bouron wrote:

From: Matthieu Bouron 

---

Hello,

The following patch add yuv2planeX_8_neon function for the arm platform.  It is
currently restricted to 8-bit per component sources until I fix fate issues
with 10-bit sources (the dnxhd-*-10bit tests fail but I haven't figured out yet
where it comes from).

Matthieu

---
  libswscale/arm/Makefile  |  1 +
  libswscale/arm/output.S  | 78 
  libswscale/arm/swscale.c |  7 +
  libswscale/utils.c   |  3 +-
  4 files changed, 88 insertions(+), 1 deletion(-)
  create mode 100644 libswscale/arm/output.S

[...]

diff --git a/libswscale/arm/output.S b/libswscale/arm/output.S
new file mode 100644
index 000..4437447
--- /dev/null
+++ b/libswscale/arm/output.S
@@ -0,0 +1,78 @@


[...]


+function ff_yuv2planeX_8_neon, export=1
+push {r4-r12, lr}
+vpush {q4-q7}
+ldr r4, [sp, #104] @ dstW
+ldr r5, [sp, #108] @ dither
+ldr r6, [sp, #112] @ offset
+vld1.8  {d0}, [r5] @ load 
8x8-bit dither values
+tst r6, #0 @ check 
offsetting which can be 0 or 3 only
+beq 1f
+vext.u8 d0, d0, d0, #3 @ honor 
offseting which can be 3 only
+1:  vmovl.u8q0, d0 @ 
extend dither to 16-bit
+vshll.u16   q1, d0, #12@ 
extend dither to 32-bit with left shift by 12 (part 1)
+vshll.u16   q2, d1, #12@ 
extend dither to 32-bit with left shift by 12 (part 2)
+mov r7, #0 @ i = 0
+2:  vmov.u8 q3, q1 @ 
initialize accumulator with dithering values (part 1)
+vmov.u8 q4, q2 @ 
initialize accumulator with dithering values (part 2)
+mov r8, r1 @ 
tmpFilterSize = filterSize
+mov r9, r2 @ srcp
+mov r10, r0@ 
filterp
+3:  ldr r11, [r9], #4  @ get 
pointer @ src[j]
+ldr r12, [r9], #4  @ get 
pointer @ src[j+1]
+add r11, r11, r7, lsl #1   @ 
[j][i]
+add r12, r12, r7, lsl #1   @ 
[j+1][i]
+vld1.16 {q5}, [r11]@ read 
8x16-bit @ src[j  ][i + {0..7}]: A,B,C,D,E,F,G,H
+vld1.16 {q6}, [r12]@ read 
8x16-bit @ src[j+1][i + {0..7}]: I,J,K,L,M,N,O,P
+ldr r11, [r10], #4 @ read 
2x16-bit coeffs (X, Y) at (filter[j], filter[j+1])
+vmov.16 q7, q5 @ copy 
8x16-bit @ src[j  ][i + {0..7}] for following inplace zip instruction
+vmov.16 q8, q6 @ copy 
8x16-bit @ src[j+1][i + {0..7}] for following inplace zip instruction
+vzip.16 q7, q8 @ 
A,I,B,J,C,K,D,L,E,M,F,N,G,O,H,L


nit: O,H,P

--
Ben

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

Re: [FFmpeg-devel] [PATCH v2 6/9] swscale/arm/yuv2rgb: macro-ify

[Benoit Fouet]

On 28/03/2016 21:19, Matthieu Bouron wrote:

---
  libswscale/arm/yuv2rgb_neon.S | 137 ++
  1 file changed, 60 insertions(+), 77 deletions(-)

diff --git a/libswscale/arm/yuv2rgb_neon.S b/libswscale/arm/yuv2rgb_neon.S
index ef7b0a6..e1b68c1 100644
--- a/libswscale/arm/yuv2rgb_neon.S
+++ b/libswscale/arm/yuv2rgb_neon.S
@@ -64,7 +64,7 @@
  vmov.u8 \a2, #255
  .endm
  
-.macro compute_16px dst y0 y1 ofmt

+.macro compute dst y0 y1 ofmt
  vmovl.u8q14, \y0   @ 8px 
of y
  vmovl.u8q15, \y1   @ 8px 
of y
  
@@ -99,23 +99,23 @@
  
  .endm
  
-.macro process_1l_16px ofmt

+.macro process_1l ofmt
  compute_premult d28, d29, d30, d31
  vld1.8  {q7}, [r4]!
-compute_16pxr2, d14, d15, \ofmt
+compute r2, d14, d15, \ofmt
  .endm
  
-.macro process_2l_16px ofmt

+.macro process_2l ofmt
  compute_premult d28, d29, d30, d31
  
  vld1.8  {q7}, [r4]!@ first line of luma

-compute_16pxr2, d14, d15, \ofmt
+compute r2, d14, d15, \ofmt
  
  vld1.8  {q7}, [r12]!   @ second line of luma

-compute_16pxr11, d14, d15, \ofmt
+compute r11, d14, d15, \ofmt
  .endm
  
-.macro load_args_nvx

+.macro load_args_nv12
  push{r4-r12, lr}
  vpush   {q4-q7}
  ldr r4, [sp, #104] @ r4  
= srcY
@@ -136,6 +136,10 @@
  sub r7, r7, r0 @ r7 = 
linesizeC - width (paddingC)
  .endm
  
+.macro load_args_nv21

+load_args_nv12
+.endm
+
  .macro load_args_yuv420p
  push{r4-r12, lr}
  vpush   {q4-q7}
@@ -176,55 +180,23 @@
  ldr r10,[sp, #120] @ r10 
= srcV
  .endm
  
-.macro declare_func ifmt ofmt

-function ff_\ifmt\()_to_\ofmt\()_neon, export=1
-
-.ifc \ifmt,nv12
-load_args_nvx
-.endif
-
-.ifc \ifmt,nv21
-load_args_nvx
-.endif
-
-.ifc \ifmt,yuv420p
-load_args_yuv420p
-.endif
-
-
-.ifc \ifmt,yuv422p
-load_args_yuv422p
-.endif
-
-1:
-mov r8, r0 @ r8 = 
width
-2:
-pld [r6, #64*3]
-pld [r4, #64*3]
-
-vmov.i8 d10, #128
-
-.ifc \ifmt,nv12
+.macro load_chroma_nv12
  pld [r12, #64*3]
  
  vld2.8  {d2, d3}, [r6]!@ q1: interleaved chroma line

  vsubl.u8q14, d2, d10   @ q14 
= U - 128
  vsubl.u8q15, d3, d10   @ q15 
= V - 128
+.endm
  
-process_2l_16px \ofmt

-.endif
-
-.ifc \ifmt,nv21
+.macro load_chroma_nv21
  pld [r12, #64*3]
  
  vld2.8  {d2, d3}, [r6]!@ q1: interleaved chroma line

  vsubl.u8q14, d3, d10   @ q14 
= U - 128
  vsubl.u8q15, d2, d10   @ q15 
= V - 128
+.endm
  
-process_2l_16px \ofmt

-.endif
-
-.ifc \ifmt,yuv420p
+.macro load_chroma_yuv420p
  pld [r10, #64*3]
  pld [r12, #64*3]
  
@@ -232,68 +204,79 @@ function ff_\ifmt\()_to_\ofmt\()_neon, export=1

  vld1.8  d3, [r10]! @ d3: 
chroma blue line
  vsubl.u8q14, d2, d10   @ q14 
= U - 128
  vsubl.u8q15, d3, d10   @ q15 
= V - 128
+.endm
  
-process_2l_16px \ofmt

-.endif
-
-.ifc \ifmt,yuv422p
+.macro load_chroma_yuv422p
  pld [r10, #64*3]
  
  vld1.8  d2, [r6]!  @ d2: chroma red line

  vld1.8  d3, [r10]! @ d3: 
chroma blue line
  vsubl.u8q14, d2, d10   @ q14 
= U - 128
  vsubl.u8q15, d3, d10   @ q15 
= V - 128
+.endm
  
-process_1l_16px \ofmt

-.endif
-
-subsr8, r8, #16@ width 
-= 16
-bgt 2b
-
-add r2, r2, r3 @ dst   
+= padding
-add r4, r4, r5 @ srcY  
+= paddingY
-
-.ifc \ifmt,nv12
+.macro increment_nv12
  add r11, r11, r3   @ dst2 
 += padding
  add r12, r12, r5   @ 
srcY2 += paddingY
-
  add r6, r6, r7 @ srcC 
 += paddingC
-
  subsr1, r1, #2 

Re: [FFmpeg-devel] [PATCH v2 8/9] swscale/arm/yuv2rgb: save a few instructions by processing the luma line interleaved

[Benoit Fouet]

Hi,

On 28/03/2016 21:19, Matthieu Bouron wrote:

---
  libswscale/arm/yuv2rgb_neon.S | 88 +--
  1 file changed, 34 insertions(+), 54 deletions(-)

diff --git a/libswscale/arm/yuv2rgb_neon.S b/libswscale/arm/yuv2rgb_neon.S
index 124d7d3..6b911c8 100644
--- a/libswscale/arm/yuv2rgb_neon.S
+++ b/libswscale/arm/yuv2rgb_neon.S

[...]

@@ -94,25 +67,29 @@
  .ifc \ofmt,bgra
  compute_rgbad8, d7, d6, d9, d12, d11, d10, d13
  .endif
+
+vzip.8  d6, d10
+vzip.8  d7, d11
+vzip.8  d8, d12
+vzip.8  d9, d13


Adding a comment to explain the resulting interleaving would be nice


  vst4.8  {q3, q4}, [\dst,:128]!
  vst4.8  {q5, q6}, [\dst,:128]!
-
  .endm
  
  .macro process_1l ofmt

-compute_premult d28, d29, d30, d31
-vld1.8  {q7}, [r4]!
-compute r2, d14, d15, \ofmt
+compute_premult
+vld2.8  {d14, d15}, [r4]!
+compute r2, \ofmt
  .endm
  
  .macro process_2l ofmt

-compute_premult d28, d29, d30, d31
+compute_premult
  
-vld1.8  {q7}, [r4]!@ first line of luma

-compute r2, d14, d15, \ofmt
+vld2.8  {d14, d15}, [r4]!  @ q7 = 
Y (interleaved)
+compute r2, \ofmt
  
-vld1.8  {q7}, [r12]!   @ second line of luma

-compute r11, d14, d15, \ofmt
+vld2.8  {d14, d15}, [r12]! @ q7 = 
Y (interleaved)
+compute r11, \ofmt
  .endm
  


What about adding a level of macro here? Something like:
.macro process_1l_internal ofmt src_addr res
compute_premult
vld2.8{d14, d15}, [\src_addr]!
compute\res, \ofmt
.endm

(again, the naming could be changed, according to your own taste :-) )

This way, we would get:
.macro process_1l ofmt
process_1l_internal \ofmt, r4, r2
.endm

.macro process_2l ofmt
process_1l_internal \ofmt, r4,  r2
process_1l_internal \ofmt, r12, r11
.endm

--
Ben

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

Re: [FFmpeg-devel] [PATCH v2 6/9] swscale/arm/yuv2rgb: macro-ify

[Benoit Fouet]

Hi,

(sorry for the first mail, fuzzy fingers...)

On 28/03/2016 21:19, Matthieu Bouron wrote:

---
  libswscale/arm/yuv2rgb_neon.S | 137 ++
  1 file changed, 60 insertions(+), 77 deletions(-)

diff --git a/libswscale/arm/yuv2rgb_neon.S b/libswscale/arm/yuv2rgb_neon.S
index ef7b0a6..e1b68c1 100644
--- a/libswscale/arm/yuv2rgb_neon.S
+++ b/libswscale/arm/yuv2rgb_neon.S
@@ -64,7 +64,7 @@
  vmov.u8 \a2, #255
  .endm
  
-.macro compute_16px dst y0 y1 ofmt

+.macro compute dst y0 y1 ofmt
  vmovl.u8q14, \y0   @ 8px 
of y
  vmovl.u8q15, \y1   @ 8px 
of y
  
@@ -99,23 +99,23 @@
  
  .endm
  
-.macro process_1l_16px ofmt

+.macro process_1l ofmt
  compute_premult d28, d29, d30, d31
  vld1.8  {q7}, [r4]!
-compute_16pxr2, d14, d15, \ofmt
+compute r2, d14, d15, \ofmt
  .endm
  
-.macro process_2l_16px ofmt

+.macro process_2l ofmt
  compute_premult d28, d29, d30, d31
  
  vld1.8  {q7}, [r4]!@ first line of luma

-compute_16pxr2, d14, d15, \ofmt
+compute r2, d14, d15, \ofmt
  
  vld1.8  {q7}, [r12]!   @ second line of luma

-compute_16pxr11, d14, d15, \ofmt
+compute r11, d14, d15, \ofmt
  .endm
  


This renaming could be split

[...]


@@ -232,68 +204,79 @@ function ff_\ifmt\()_to_\ofmt\()_neon, export=1
  vld1.8  d3, [r10]! @ d3: 
chroma blue line
  vsubl.u8q14, d2, d10   @ q14 
= U - 128
  vsubl.u8q15, d3, d10   @ q15 
= V - 128
+.endm
  
-process_2l_16px \ofmt

-.endif
-
-.ifc \ifmt,yuv422p
+.macro load_chroma_yuv422p
  pld [r10, #64*3]
  
  vld1.8  d2, [r6]!  @ d2: chroma red line

  vld1.8  d3, [r10]! @ d3: 
chroma blue line
  vsubl.u8q14, d2, d10   @ q14 
= U - 128
  vsubl.u8q15, d3, d10   @ q15 
= V - 128
+.endm
  
-process_1l_16px \ofmt

-.endif
-
-subsr8, r8, #16@ width 
-= 16
-bgt 2b
-
-add r2, r2, r3 @ dst   
+= padding
-add r4, r4, r5 @ srcY  
+= paddingY
-
-.ifc \ifmt,nv12
+.macro increment_nv12


How about increment_and test_nv12? Same for the other ones.
(I'm not happy with the name I found, but am trying to come up with a 
solution to have a more explicit naming)


--
Ben

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

Re: [FFmpeg-devel] [PATCH v2 6/9] swscale/arm/yuv2rgb: macro-ify

[Benoit Fouet]

On 28/03/2016 21:19, Matthieu Bouron wrote:

---
  libswscale/arm/yuv2rgb_neon.S | 137 ++
  1 file changed, 60 insertions(+), 77 deletions(-)

diff --git a/libswscale/arm/yuv2rgb_neon.S b/libswscale/arm/yuv2rgb_neon.S
index ef7b0a6..e1b68c1 100644
--- a/libswscale/arm/yuv2rgb_neon.S
+++ b/libswscale/arm/yuv2rgb_neon.S
@@ -64,7 +64,7 @@
  vmov.u8 \a2, #255
  .endm
  
-.macro compute_16px dst y0 y1 ofmt

+.macro compute dst y0 y1 ofmt
  vmovl.u8q14, \y0   @ 8px 
of y
  vmovl.u8q15, \y1   @ 8px 
of y
  
@@ -99,23 +99,23 @@
  
  .endm
  
-.macro process_1l_16px ofmt

+.macro process_1l ofmt
  compute_premult d28, d29, d30, d31
  vld1.8  {q7}, [r4]!
-compute_16pxr2, d14, d15, \ofmt
+compute r2, d14, d15, \ofmt
  .endm
  
-.macro process_2l_16px ofmt

+.macro process_2l ofmt
  compute_premult d28, d29, d30, d31
  
  vld1.8  {q7}, [r4]!@ first line of luma

-compute_16pxr2, d14, d15, \ofmt
+compute r2, d14, d15, \ofmt
  
  vld1.8  {q7}, [r12]!   @ second line of luma

-compute_16pxr11, d14, d15, \ofmt
+compute r11, d14, d15, \ofmt
  .endm
  
-.macro load_args_nvx

+.macro load_args_nv12
  push{r4-r12, lr}
  vpush   {q4-q7}
  ldr r4, [sp, #104] @ r4  
= srcY
@@ -136,6 +136,10 @@
  sub r7, r7, r0 @ r7 = 
linesizeC - width (paddingC)
  .endm
  
+.macro load_args_nv21

+load_args_nv12
+.endm
+
  .macro load_args_yuv420p
  push{r4-r12, lr}
  vpush   {q4-q7}
@@ -176,55 +180,23 @@
  ldr r10,[sp, #120] @ r10 
= srcV
  .endm
  
-.macro declare_func ifmt ofmt

-function ff_\ifmt\()_to_\ofmt\()_neon, export=1
-
-.ifc \ifmt,nv12
-load_args_nvx
-.endif
-
-.ifc \ifmt,nv21
-load_args_nvx
-.endif
-
-.ifc \ifmt,yuv420p
-load_args_yuv420p
-.endif
-
-
-.ifc \ifmt,yuv422p
-load_args_yuv422p
-.endif
-
-1:
-mov r8, r0 @ r8 = 
width
-2:
-pld [r6, #64*3]
-pld [r4, #64*3]
-
-vmov.i8 d10, #128
-
-.ifc \ifmt,nv12
+.macro load_chroma_nv12
  pld [r12, #64*3]
  
  vld2.8  {d2, d3}, [r6]!@ q1: interleaved chroma line

  vsubl.u8q14, d2, d10   @ q14 
= U - 128
  vsubl.u8q15, d3, d10   @ q15 
= V - 128
+.endm
  
-process_2l_16px \ofmt

-.endif
-
-.ifc \ifmt,nv21
+.macro load_chroma_nv21
  pld [r12, #64*3]
  
  vld2.8  {d2, d3}, [r6]!@ q1: interleaved chroma line

  vsubl.u8q14, d3, d10   @ q14 
= U - 128
  vsubl.u8q15, d2, d10   @ q15 
= V - 128
+.endm
  
-process_2l_16px \ofmt

-.endif
-
-.ifc \ifmt,yuv420p
+.macro load_chroma_yuv420p
  pld [r10, #64*3]
  pld [r12, #64*3]
  
@@ -232,68 +204,79 @@ function ff_\ifmt\()_to_\ofmt\()_neon, export=1

  vld1.8  d3, [r10]! @ d3: 
chroma blue line
  vsubl.u8q14, d2, d10   @ q14 
= U - 128
  vsubl.u8q15, d3, d10   @ q15 
= V - 128
+.endm
  
-process_2l_16px \ofmt

-.endif
-
-.ifc \ifmt,yuv422p
+.macro load_chroma_yuv422p
  pld [r10, #64*3]
  
  vld1.8  d2, [r6]!  @ d2: chroma red line

  vld1.8  d3, [r10]! @ d3: 
chroma blue line
  vsubl.u8q14, d2, d10   @ q14 
= U - 128
  vsubl.u8q15, d3, d10   @ q15 
= V - 128
+.endm
  
-process_1l_16px \ofmt

-.endif
-
-subsr8, r8, #16@ width 
-= 16
-bgt 2b
-
-add r2, r2, r3 @ dst   
+= padding
-add r4, r4, r5 @ srcY  
+= paddingY
-
-.ifc \ifmt,nv12
+.macro increment_nv12
  add r11, r11, r3   @ dst2 
 += padding
  add r12, r12, r5   @ 
srcY2 += paddingY
-
  add r6, r6, r7 @ srcC 
 += paddingC
-
  subsr1, r1, #2 

Re: [FFmpeg-devel] [PATCH 06/10] swscale/arm/yuv2rgb: only process one line at a time for the yuv420p and nv{12, 21} formats

[Benoit Fouet]

Hi,

Le 26/03/2016 13:05, Matthieu Bouron a écrit :

On Sat, Mar 26, 2016 at 2:09 AM, Michael Niedermayer wrote:
>On Fri, Mar 25, 2016 at 11:46:01PM +0100, Matthieu Bouron wrote:

> >From: Matthieu Bouron
> >
> >---
> >  libswscale/arm/yuv2rgb_neon.S | 89

>---

> >  1 file changed, 24 insertions(+), 65 deletions(-)

>
>breaks build
>
>  make distclean ; ../configure --cross-prefix=/usr/arm-linux-gnueabi/bin/
>--cc='ccache arm-linux-gnueabi-gcc-4.5' --extra-cflags='-mfpu=neon
>-mfloat-abi=softfp' --cpu=cortex-a8 --arch=armv7 --target-os=linux
>--enable-cross-compile && make -j12
>
>CC  libavutil/arm/float_dsp_init_arm.o
>src/libswscale/arm/yuv2rgb_neon.S: Assembler messages:
>src/libswscale/arm/yuv2rgb_neon.S:269: Error: thumb conditional
>instruction should be in IT block -- `subeq r6,r6,r0'
>src/libswscale/arm/yuv2rgb_neon.S:269: Error: thumb conditional
>instruction should be in IT block -- `addne r6,r7'
>

[...]

Patch updated with the relevant it instructions added. It still does build
on my rpi2 setup but is not tested on the same setup as yours.
Can you confirm it builds/works on your setup ?

If it works, i will send an updated version of the next patch (07/10) to
resolve the conflicts.

Matthieu

0006-swscale-arm-yuv2rgb-only-process-one-line-at-a-time-.patch


 From 7b3a405b2b483fb16f549b69ce6f21d8a946 Mon Sep 17 00:00:00 2001
From: Matthieu Bouron
Date: Wed, 23 Mar 2016 11:26:13 +
Subject: [PATCH 06/10] swscale/arm/yuv2rgb: only process one line at a time
  for the yuv420p and nv{12,21} formats

---
  libswscale/arm/yuv2rgb_neon.S | 92 +--
  1 file changed, 27 insertions(+), 65 deletions(-)

diff --git a/libswscale/arm/yuv2rgb_neon.S b/libswscale/arm/yuv2rgb_neon.S
index ef7b0a6..6aeccae 100644
--- a/libswscale/arm/yuv2rgb_neon.S
+++ b/libswscale/arm/yuv2rgb_neon.S
@@ -105,16 +105,6 @@
  compute_16pxr2, d14, d15, \ofmt
  .endm
  
-.macro process_2l_16px ofmt

-compute_premult d28, d29, d30, d31
-
-vld1.8  {q7}, [r4]!@ first 
line of luma
-compute_16pxr2, d14, d15, \ofmt
-
-vld1.8  {q7}, [r12]!   @ 
second line of luma
-compute_16pxr11, d14, d15, \ofmt
-.endm
-
  .macro load_args_nvx
  push{r4-r12, lr}
  vpush   {q4-q7}
@@ -127,13 +117,9 @@
  ldr r10,[sp, #128] @ r10 
= y_coeff
  vdup.16 d0, r10@ d0  
= y_coeff
  vld1.16 {d1}, [r8] @ d1  
= *table
-add r11, r2, r3@ r11 = 
dst + linesize (dst2)
-add r12, r4, r5@ r12 = 
srcY + linesizeY (srcY2)


Nit: this lets r11 and r12 unused by the NV conversions. It should be 
possible not to push/pop them
If not (which I would certainly understand), what would you think about 
moving the registers save out of the 'load_args_*' macro?
It seems weird to have all the push/vpush that are not factored, and the 
pop/vpop that is done in only one place, at the end of each function.


[snip]

Looks good to me anyway (as well as the remainder of the series).

--
Ben

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

Re: [FFmpeg-devel] [PATCH 02/10] swscale/arm/yuv2rgb: fix comments and factorize lsl in load_args_yuv422p

[Benoit Fouet]

Hi,

Le 25/03/2016 23:45, Matthieu Bouron a écrit :

From: Matthieu Bouron

---
  libswscale/arm/yuv2rgb_neon.S | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/libswscale/arm/yuv2rgb_neon.S b/libswscale/arm/yuv2rgb_neon.S
index f40327b..aac0773 100644
--- a/libswscale/arm/yuv2rgb_neon.S
+++ b/libswscale/arm/yuv2rgb_neon.S
@@ -172,11 +172,10 @@
  vdup.16 d0, r10@ d0  
= y_coeff
  vld1.16 {d1}, [r8] @ d1  
= *table
  add r11, r2, r3@ r11 
= dst + linesize (dst2)
-lsl r8, r0, #2
-sub r3, r3, r8 @ r3 = 
linesize  * 2 - width * 4 (padding)
-sub r5, r5, r0 @ r5 = 
linesizeY * 2 - width (paddingY)
-sub r7, r7, r0, lsr #1 @ r7 = 
linesizeU - width / 2 (paddingU)
-sub r12,r12,r0, lsr #1 @ r12 = 
linesizeV- width / 2 (paddingV)
+sub r3, r3, r0, lsl #2 @ r3  = 
linesize  - width * 4 (padding)
+sub r5, r5, r0 @ r5  = 
linesizeY - width (paddingY)
+sub r7, r7, r0, lsr #1 @ r7  = 
linesizeU - width / 2 (paddingU)
+sub r12,r12,r0, lsr #1 @ r12 = 
linesizeV - width / 2 (paddingV)
  ldr r10,[sp, #120] @ r10 
= srcV
  .endm
  


nit: it would be cool to split: one for the comments and the other one 
for the lsl factorization.


--
Ben

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

Re: [FFmpeg-devel] [PATCH] lavc/psymodel: check for av_malloc failure

[Benoit Fouet]

Hi,

Le 04/03/2016 04:06, Ganesh Ajjanagadde a écrit :

No idea why in commit 01ecb7172b684f1c4b3e748f95c5a9a494ca36ec the
checks were removed; this can lead to NULL pointer dereferences. This
effectively reverts that portion of the commit.

Signed-off-by: Ganesh Ajjanagadde 
---
  libavcodec/psymodel.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavcodec/psymodel.c b/libavcodec/psymodel.c
index 6274a49..edc5ac8 100644
--- a/libavcodec/psymodel.c
+++ b/libavcodec/psymodel.c
@@ -120,7 +120,11 @@ av_cold struct FFPsyPreprocessContext* 
ff_psy_preprocess_init(AVCodecContext *av
   FF_FILTER_MODE_LOWPASS, 
FILT_ORDER,
   cutoff_coeff, 0.0, 0.0);
  if (ctx->fcoeffs) {
-ctx->fstate = av_mallocz(sizeof(ctx->fstate[0]) * avctx->channels);
+ctx->fstate = av_mallocz_array(sizeof(ctx->fstate[0]), 
avctx->channels);
+if (!ctx->fstate) {
+av_free(ctx);
+return NULL;


you're leaking ctx->fcoeffs


+}
  for (i = 0; i < avctx->channels; i++)
  ctx->fstate[i] = ff_iir_filter_init_state(FILT_ORDER);
  }


--
Ben

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

Re: [FFmpeg-devel] [PATCH] wavdec: RIFX file format support

[Benoit Fouet]

Hi,

Le 17/12/2014 22:15, Reimar Döffinger a écrit :

On Wed, Dec 17, 2014 at 11:55:17AM +0100, Thomas Volkert wrote:

On 12/16/2014 08:36 AM, Reimar Döffinger wrote:

On Mon, Dec 15, 2014 at 10:24:55AM +, Carl Eugen Hoyos wrote:

   codec-sample_rate = avio_rl32(pb);
   codec-bit_rate= avio_rl32(pb) * 8;
   codec-block_align = avio_rl16(pb);
+if (big_endian) {
+id = ntohs(id);
+codec-channels= ntohs(codec-channels);
+codec-sample_rate = ntohl(codec-sample_rate);
+codec-bit_rate= ntohl(codec-bit_rate / 8) * 8;
+codec-block_align = ntohs(codec-block_align);
+}

Instead please do:
if (big_endian) {
   id = avio_rb32(pb);
   codec-channels = avio_rb32(pb);
  ...
} else {
id = avio_rl32(pb);
...
}

Not sure this is a good idea, as it duplicates the code.
It might be better to use the if as-is, just replacing ntoh*
by av_bswap*.

I would prefer this version:
if (!big_endian) {
  avio_rl32()
} else {
  avio_rb32()
}
In case of big endianess, your idea (and my former solution) would need two
instead of one updates per value

Why would that matter? Performance, especially for big-endian type is
hardly relevant.
Due to fewer branches it might actually give better performance for the
common case (but as said I don't think it matters).
It mostly avoids duplicating some actual code like the *8.
There are also other options to reduce the code size, though I am
sceptical if they are a good idea.
1) Macros like:
#define GET16(pb) (big_endian ? avio_rb16(pb) : avio_rl16(pb))
2) Functions that take an additional endian argument.


While we're at it:
3) Function pointers initialized based on the endianness

--
Ben

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

Re: [FFmpeg-devel] [PATCH] pngdsp x86: use scalar loop for unaligned dest buffers.

[Benoit Fouet]

Hi,

- Mail original -
 Hi,
 
 2014-12-02 14:31 GMT+01:00 Benoit Fouet benoit.fo...@free.fr:
  Fixes ticket #4148
 
 Please try that one instead.
 

Works fine, thanks.
Please fix commit message: s/onlu/only

 As all your changes have been reverted, I've put myself as the only
 author. I've left your signed-off-by, but I'm not sure of its
 purpose now.
 

Just remove my name.

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

[FFmpeg-devel] [PATCH 1/4] libavcodec/pngdec: support 'previous' dispose operation for APNG.

[Benoit Fouet]

---
 libavcodec/pngdec.c | 72 +
 1 file changed, 50 insertions(+), 22 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 229a6d6..b1e77e5 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -38,6 +38,7 @@ typedef struct PNGDecContext {
 AVCodecContext *avctx;
 
 GetByteContext gb;
+ThreadFrame previous_picture;
 ThreadFrame last_picture;
 ThreadFrame picture;
 
@@ -55,6 +56,7 @@ typedef struct PNGDecContext {
 int bits_per_pixel;
 int bpp;
 
+int frame_id;
 uint8_t *image_buf;
 int image_linesize;
 uint32_t palette[256];
@@ -827,13 +829,14 @@ static int decode_fctl_chunk(AVCodecContext *avctx, 
PNGDecContext *s,
 return AVERROR_INVALIDDATA;
 
 /* always (re)start with a clean frame */
-if (sequence_number == 0)
+if (sequence_number == 0) {
 s-dispose_op = APNG_DISPOSE_OP_BACKGROUND;
-
-if (s-dispose_op == APNG_DISPOSE_OP_PREVIOUS) {
-av_log(avctx, AV_LOG_ERROR,
-   Dispose operation 'previous' is not yet implemented, using 
'none'.\n);
-s-dispose_op = APNG_DISPOSE_OP_NONE;
+s-frame_id = 0;
+} else {
+s-frame_id++;
+if (s-frame_id == 1  s-dispose_op == APNG_DISPOSE_OP_PREVIOUS)
+/* previous for the second frame is the first frame */
+s-dispose_op = APNG_DISPOSE_OP_NONE;
 }
 
 return 0;
@@ -864,8 +867,9 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 {
 int i, j;
 uint8_t *pd  = p-data[0];
-/* TODO make pd_last point to the one before for APNG_DISPOSE_OP_PREVIOUS 
*/
 uint8_t *pd_last = s-last_picture.f-data[0];
+uint8_t *pd_last_region = s-dispose_op == APNG_DISPOSE_OP_PREVIOUS ?
+s-previous_picture.f-data[0] : 
s-last_picture.f-data[0];
 int ls = FFMIN(av_image_get_linesize(p-format, s-width, 0), s-width * 
s-bpp);
 
 if (s-blend_op == APNG_BLEND_OP_OVER 
@@ -876,6 +880,9 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 }
 
 ff_thread_await_progress(s-last_picture, INT_MAX, 0);
+if (s-dispose_op == APNG_DISPOSE_OP_PREVIOUS)
+ff_thread_await_progress(s-previous_picture, INT_MAX, 0);
+
 for (j = 0; j  s-y_offset; j++) {
 for (i = 0; i  ls; i++)
 pd[i] = pd_last[i];
@@ -886,6 +893,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 if (s-dispose_op != APNG_DISPOSE_OP_BACKGROUND  s-blend_op == 
APNG_BLEND_OP_OVER) {
 uint8_t ri, gi, bi, ai;
 
+pd_last_region += s-y_offset * s-image_linesize;
 if (avctx-pix_fmt == AV_PIX_FMT_RGBA) {
 ri = 0;
 gi = 1;
@@ -907,17 +915,17 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 /* output = alpha * foreground + (1-alpha) * background */
 switch (alpha) {
 case 0:
-pd[i+ri] = pd_last[i+ri];
-pd[i+gi] = pd_last[i+gi];
-pd[i+bi] = pd_last[i+bi];
+pd[i+ri] = pd_last_region[i+ri];
+pd[i+gi] = pd_last_region[i+gi];
+pd[i+bi] = pd_last_region[i+bi];
 pd[i+ai] = 0xff;
 break;
 case 255:
 break;
 default:
-pd[i+ri] = FAST_DIV255(alpha * pd[i+ri] + (255 - alpha) * 
pd_last[i+ri]);
-pd[i+gi] = FAST_DIV255(alpha * pd[i+gi] + (255 - alpha) * 
pd_last[i+gi]);
-pd[i+bi] = FAST_DIV255(alpha * pd[i+bi] + (255 - alpha) * 
pd_last[i+bi]);
+pd[i+ri] = FAST_DIV255(alpha * pd[i+ri] + (255 - alpha) * 
pd_last_region[i+ri]);
+pd[i+gi] = FAST_DIV255(alpha * pd[i+gi] + (255 - alpha) * 
pd_last_region[i+gi]);
+pd[i+bi] = FAST_DIV255(alpha * pd[i+bi] + (255 - alpha) * 
pd_last_region[i+bi]);
 pd[i+ai] = 0xff;
 break;
 }
@@ -926,6 +934,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 pd[i] = pd_last[i];
 pd  += s-image_linesize;
 pd_last += s-image_linesize;
+pd_last_region += s-image_linesize;
 }
 } else {
 for (j = s-y_offset; j  s-y_offset + s-cur_h; j++) {
@@ -955,6 +964,7 @@ static int decode_frame_common(AVCodecContext *avctx, 
PNGDecContext *s,
 uint32_t tag, length;
 int decode_next_dat = 0;
 int ret = AVERROR_INVALIDDATA;
+AVFrame *ref;
 
 for (;;) {
 length = bytestream2_get_bytes_left(s-gb);
@@ -1053,11 +1063,13 @@ exit_loop:
 handle_small_bpp(s, p);
 
 /* handle p-frames only if a predecessor frame is available */
-if (s-last_picture.f-data[0]) {
+ref = s-dispose_op == APNG_DISPOSE_OP_PREVIOUS 

[FFmpeg-devel] [PATCH 4/4] avcodec/pngdec: use memcpy instead of byte loops for P frames.

[Benoit Fouet]

Rely on the way memcpy is optimized for one's system instead of looping
on a byte buffer for buffer copies to handle P frames.
---
 libavcodec/pngdec.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 7e7b285..8b004bd 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -844,15 +844,14 @@ static int decode_fctl_chunk(AVCodecContext *avctx, 
PNGDecContext *s,
 
 static void handle_p_frame_png(PNGDecContext *s, AVFrame *p)
 {
-int i, j;
+int j;
 uint8_t *pd  = p-data[0];
 uint8_t *pd_last = s-last_picture.f-data[0];
 int ls = FFMIN(av_image_get_linesize(p-format, s-width, 0), s-width * 
s-bpp);
 
 ff_thread_await_progress(s-last_picture, INT_MAX, 0);
 for (j = 0; j  s-height; j++) {
-for (i = 0; i  ls; i++)
-pd[i] += pd_last[i];
+memcpy(pd, pd_last, ls);
 pd  += s-image_linesize;
 pd_last += s-image_linesize;
 }
@@ -884,8 +883,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 ff_thread_await_progress(s-previous_picture, INT_MAX, 0);
 
 for (j = 0; j  s-y_offset; j++) {
-for (i = 0; i  ls; i++)
-pd[i] = pd_last[i];
+memcpy(pd, pd_last, ls);
 pd  += s-image_linesize;
 pd_last += s-image_linesize;
 }
@@ -907,8 +905,9 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 }
 
 for (j = s-y_offset; j  s-y_offset + s-cur_h; j++) {
-for (i = 0; i  s-x_offset * s-bpp; i++)
-pd[i] = pd_last[i];
+i = s-x_offset * s-bpp;
+if (i)
+memcpy(pd, pd_last, i);
 for (; i  (s-x_offset + s-cur_w) * s-bpp; i += s-bpp) {
 uint8_t alpha = pd[i+ai];
 
@@ -930,26 +929,27 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 break;
 }
 }
-for (; i  ls; i++)
-pd[i] = pd_last[i];
+if (ls - i)
+memcpy(pd+i, pd_last+i, ls - i);
 pd  += s-image_linesize;
 pd_last += s-image_linesize;
 pd_last_region += s-image_linesize;
 }
 } else {
 for (j = s-y_offset; j  s-y_offset + s-cur_h; j++) {
-for (i = 0; i  s-x_offset * s-bpp; i++)
-pd[i] = pd_last[i];
-for (i = (s-x_offset + s-cur_w) * s-bpp; i  ls; i++)
-pd[i] = pd_last[i];
+int end_offset = (s-x_offset + s-cur_w) * s-bpp;
+int end_len= ls - end_offset;
+if (s-x_offset)
+memcpy(pd, pd_last, s-x_offset * s-bpp);
+if (end_len)
+memcpy(pd+end_offset, pd_last+end_offset, end_len);
 pd  += s-image_linesize;
 pd_last += s-image_linesize;
 }
 }
 
 for (j = s-y_offset + s-cur_h; j  s-height; j++) {
-for (i = 0; i  ls; i++)
-pd[i] = pd_last[i];
+memcpy(pd, pd_last, ls);
 pd  += s-image_linesize;
 pd_last += s-image_linesize;
 }
-- 
2.2.0

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

[FFmpeg-devel] [PATCH 2/4] avcodec/pngdec: fix mem leak in init()

[Benoit Fouet]

---
 libavcodec/pngdec.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index b1e77e5..3905e0f 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -1237,8 +1237,12 @@ static av_cold int png_dec_init(AVCodecContext *avctx)
 s-previous_picture.f = av_frame_alloc();
 s-last_picture.f = av_frame_alloc();
 s-picture.f = av_frame_alloc();
-if (!s-previous_picture.f || !s-last_picture.f || !s-picture.f)
+if (!s-previous_picture.f || !s-last_picture.f || !s-picture.f) {
+av_frame_free(s-previous_picture.f);
+av_frame_free(s-last_picture.f);
+av_frame_free(s-picture.f);
 return AVERROR(ENOMEM);
+}
 
 if (!avctx-internal-is_copy) {
 avctx-internal-allocate_progress = 1;
-- 
2.2.0

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

[FFmpeg-devel] [PATCH 3/4] avcodec/pngdec: fix indentation in handle_row()

[Benoit Fouet]

---
 libavcodec/pngdec.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 3905e0f..7e7b285 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -307,13 +307,13 @@ static void png_handle_row(PNGDecContext *s)
 
 if (!s-interlace_type) {
 ptr = s-image_buf + s-image_linesize * (s-y + s-y_offset) + 
s-x_offset * s-bpp;
-if (s-y == 0)
-last_row = s-last_row;
-else
-last_row = ptr - s-image_linesize;
+if (s-y == 0)
+last_row = s-last_row;
+else
+last_row = ptr - s-image_linesize;
 
-png_filter_row(s-dsp, ptr, s-crow_buf[0], s-crow_buf + 1,
-   last_row, s-row_size, s-bpp);
+png_filter_row(s-dsp, ptr, s-crow_buf[0], s-crow_buf + 1,
+   last_row, s-row_size, s-bpp);
 /* loco lags by 1 row so that it doesn't interfere with top prediction 
*/
 if (s-filter_type == PNG_FILTER_TYPE_LOCO  s-y  0) {
 if (s-bit_depth == 16) {
-- 
2.2.0

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

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/pngdec: use memcpy instead of byte loops for P frames.

[Benoit Fouet]

Hi,

- Mail original -
 Rely on the way memcpy is optimized for one's system instead of
 looping
 on a byte buffer for buffer copies to handle P frames.
 ---
  libavcodec/pngdec.c | 30 +++---
  1 file changed, 15 insertions(+), 15 deletions(-)
 
 diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
 index 7e7b285..8b004bd 100644
 --- a/libavcodec/pngdec.c
 +++ b/libavcodec/pngdec.c
 @@ -844,15 +844,14 @@ static int decode_fctl_chunk(AVCodecContext
 *avctx, PNGDecContext *s,
  
  static void handle_p_frame_png(PNGDecContext *s, AVFrame *p)
  {
 -int i, j;
 +int j;
  uint8_t *pd  = p-data[0];
  uint8_t *pd_last = s-last_picture.f-data[0];
  int ls = FFMIN(av_image_get_linesize(p-format, s-width, 0),
  s-width * s-bpp);
  
  ff_thread_await_progress(s-last_picture, INT_MAX, 0);
  for (j = 0; j  s-height; j++) {
 -for (i = 0; i  ls; i++)
 -pd[i] += pd_last[i];
 +memcpy(pd, pd_last, ls);
 

Ouch... Reverted locally...

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

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/pngdec: use memcpy instead of byte loops for P frames.

[Benoit Fouet]

Hi,

Le 03/12/2014 14:51, Christophe Gisquet a écrit :

Hi,

2014-12-03 14:16 GMT+01:00 Benoit Fouet benoit.fo...@free.fr:

Rely on the way memcpy is optimized for one's system instead of looping
on a byte buffer for buffer copies to handle P frames.

Are there many compilers left that actually perform a call here
instead of inlining code? Some people will smugly call that ricing,
but it would be interesting to bench this for small and large images.
As you mention P frames, that might be smaller widths here, where
the overhead of checking the various aligments etc in a C lib memcpy
is larger.



When the overhead here is high, then it means that we are going to 
perform a lot on the frame itself, looping using byte buffers.
I believe this is a good compromise (and also I had up to 20% better 
performances on the APNG samples I have).


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

Re: [FFmpeg-devel] [PATCH 4/4] avcodec/pngdec: use memcpy instead of byte loops for P frames.

[Benoit Fouet]

Hi,

On December 3, 2014 6:39:12 PM GMT+01:00, Christophe Gisquet 
christophe.gisq...@gmail.com wrote:
Hi,

2014-12-03 18:32 GMT+01:00 Benoit Fouet benoit.fo...@free.fr:
 When the overhead here is high, then it means that we are going to
perform a
 lot on the frame itself, looping using byte buffers.
 I believe this is a good compromise (and also I had up to 20% better
 performances on the APNG samples I have).

I have just benchmarked on a few animated pngs, and it's
systematically much better here.


Cool, and also thanks for testing this!

-- 
Ben


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

[FFmpeg-devel] [PATCH] apng: move shared header from avformat to avcodec.

[Benoit Fouet]

---
 {libavformat = libavcodec}/apng.h | 6 +++---
 libavcodec/pngdec.c| 2 +-
 libavformat/apngdec.c  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)
 rename {libavformat = libavcodec}/apng.h (93%)

diff --git a/libavformat/apng.h b/libavcodec/apng.h
similarity index 93%
rename from libavformat/apng.h
rename to libavcodec/apng.h
index 2abf011..41249e0 100644
--- a/libavformat/apng.h
+++ b/libavcodec/apng.h
@@ -24,8 +24,8 @@
  * APNG common header
  */
 
-#ifndef AVFORMAT_APNG_H
-#define AVFORMAT_APNG_H
+#ifndef AVCODEC_APNG_H
+#define AVCODEC_APNG_H
 
 enum {
APNG_DISPOSE_OP_NONE   = 0,
@@ -38,4 +38,4 @@ enum {
 APNG_BLEND_OP_OVER   = 1,
 };
 
-#endif /* AVFORMAT_APNG_H */
+#endif /* AVCODEC_APNG_H */
diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 9e52d0b..229a6d6 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -26,10 +26,10 @@
 #include avcodec.h
 #include bytestream.h
 #include internal.h
+#include apng.h
 #include png.h
 #include pngdsp.h
 #include thread.h
-#include libavformat/apng.h
 
 #include zlib.h
 
diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index 276d765..189480e 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -26,13 +26,13 @@
  * @see http://www.w3.org/TR/PNG
  */
 
-#include apng.h
 #include avformat.h
 #include avio_internal.h
 #include internal.h
 #include libavutil/imgutils.h
 #include libavutil/intreadwrite.h
 #include libavutil/opt.h
+#include libavcodec/apng.h
 #include libavcodec/png.h
 #include libavcodec/bytestream.h
 
-- 
2.2.0

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

[FFmpeg-devel] [PATCH] avformat/apngdec: exit probing when skipping is not possible.

[Benoit Fouet]

---
Found an infinite loop on probing while zzuf'ing.
---
 libavformat/apngdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index 189480e..d97b015 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -87,7 +87,7 @@ static int apng_probe(AVProbeData *p)
 /* we don't check IDAT size, as this is the last tag
  * we check, and it may be larger than the probe buffer */
 if (tag != MKTAG('I', 'D', 'A', 'T') 
-len  bytestream2_get_bytes_left(gb))
+len + 4  bytestream2_get_bytes_left(gb))
 return 0;
 
 switch (tag) {
-- 
2.2.0

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

[FFmpeg-devel] [PATCH] pngdsp x86: use scalar loop for unaligned dest buffers.

[Benoit Fouet]

Signed-off-by: Christophe Gisquet christophe.gisq...@gmail.com
Signed-off-by: Benoit Fouet benoit.fo...@gmail.com
---
 libavcodec/x86/pngdsp.asm | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavcodec/x86/pngdsp.asm b/libavcodec/x86/pngdsp.asm
index 8e23ccf..76b93a9 100644
--- a/libavcodec/x86/pngdsp.asm
+++ b/libavcodec/x86/pngdsp.asm
@@ -36,9 +36,13 @@ cglobal add_bytes_l2, 4, 6, %1, dst, src1, src2, wa, w, i
 movsxd waq, wad
 %endif
 xor iq, iq
+mov wq, waq
+
+; test unaligned dst buffer
+test dstq, (mmsize-1)
+jnz .end_s
 
 ; vector loop
-mov wq, waq
 andwaq, ~(mmsize*2-1)
 jmp .end_v
 .loop_v:
-- 
2.2.0

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

Re: [FFmpeg-devel] [PATCH] pngdsp x86: use scalar loop for unaligned dest buffers.

[Benoit Fouet]

Hi,

- Mail original -
 Hi,
 
 2014-12-02 13:51 GMT+01:00 Benoit Fouet benoit.fo...@free.fr:
  Signed-off-by: Christophe Gisquet christophe.gisq...@gmail.com
  Signed-off-by: Benoit Fouet benoit.fo...@gmail.com
 
 While I suggested that the change be written like this, I didn't
 think
 long about that specific code, and overall, I really don't think this
 change is what should be applied. Though you probably tested that it
 allows correct decoding.
 

It indeed allows decoding without crashing, which is IMHO a minimum.

 I'm for writing a SIMD unaligned loop, the question being whether it
 should be the only path. SSE4 version of add_hfyu_left_pred for
 instance has 3 paths for various alignments.
 

I understood that, though, as stated, I'm not able to do that, this is why I 
sent this one instead, just to have something working in the tree.
I can add a TODO or something like that instead, would that be OK?

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

Re: [FFmpeg-devel] [PATCH] pngdsp x86: use scalar loop for unaligned dest buffers.

[Benoit Fouet]

Hi,

- Mail original -
 Benoit Fouet benoit.fouet at free.fr writes:
 
 [...]
 
 Please mention ticket #4148 (if it is related).
 

It is, just forgot about it.
Also, I should be changing the libavcodec/pngdsp.h to no more mention the 
alignment constraints (or at least document that, for now, unaligned works, but 
slower).

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

Re: [FFmpeg-devel] [PATCH 1/2] configure: add optional pkg-config helper and use it.

[Benoit Fouet]

Hi,

- Mail original -
 The require variant dies if the package is not present.
 The check variant does not import the flags to the used list.
 The new variant imports the flags if the package is present
 but does not die if it is not.
 The new call graph is: require - use - check.
 
 Use use_pkg_config for libx264 and libsmbclient: more readable
 and three external call less per library.
 
 Signed-off-by: Nicolas George geo...@nsup.org
 ---
  configure | 16 +---
  1 file changed, 9 insertions(+), 7 deletions(-)
 
 diff --git a/configure b/configure
 index d4a86c0..af89e05 100755
 --- a/configure
 +++ b/configure
 @@ -1203,13 +1203,17 @@ require_cpp(){
  check_lib_cpp $headers $classes $@ || die ERROR: $name
  not found
  }
  
 -require_pkg_config(){
 +use_pkg_config(){
  pkg=$1
 -check_pkg_config $@ || die ERROR: $pkg not found
 +check_pkg_config $@ || return
 

IMHO, it would be clearer to have return 1 here.

LGTM otherwise.

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

[FFmpeg-devel] [PATCH] pngdsp x86: use scalar loop for unaligned dest buffers.

[Benoit Fouet]

Fixes ticket #4148

Signed-off-by: Christophe Gisquet christophe.gisq...@gmail.com
Signed-off-by: Benoit Fouet benoit.fo...@gmail.com
---
Add TODO
Update function prototype documentation
Mention ticket 4148
---
 libavcodec/pngdsp.h   | 2 ++
 libavcodec/x86/pngdsp.asm | 7 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/libavcodec/pngdsp.h b/libavcodec/pngdsp.h
index 1475b0c..24d5b7d 100644
--- a/libavcodec/pngdsp.h
+++ b/libavcodec/pngdsp.h
@@ -25,6 +25,8 @@
 #include stdint.h
 
 typedef struct PNGDSPContext {
+/* src1 must be 16-aligned, dst and src2 must have the same alignment.
+ * If the latter are not 16-aligned, a scalar version will be used. */
 void (*add_bytes_l2)(uint8_t *dst  /* align 16 */,
  uint8_t *src1 /* align 16 */,
  uint8_t *src2 /* align 16 */, int w);
diff --git a/libavcodec/x86/pngdsp.asm b/libavcodec/x86/pngdsp.asm
index 8e23ccf..078c73f 100644
--- a/libavcodec/x86/pngdsp.asm
+++ b/libavcodec/x86/pngdsp.asm
@@ -36,9 +36,14 @@ cglobal add_bytes_l2, 4, 6, %1, dst, src1, src2, wa, w, i
 movsxd waq, wad
 %endif
 xor iq, iq
+mov wq, waq
+
+; test unaligned dst buffer
+; TODO have an optimized unaligned version
+test dstq, (mmsize-1)
+jnz .end_s
 
 ; vector loop
-mov wq, waq
 andwaq, ~(mmsize*2-1)
 jmp .end_v
 .loop_v:
-- 
2.2.0

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

Re: [FFmpeg-devel] [PATCH] libavcodec/pngdec: support 'previous' dispose operation for APNG.

[Benoit Fouet]

Le 02/12/2014 15:21, Michael Niedermayer a écrit :

On Tue, Dec 02, 2014 at 08:44:02AM +0100, Benoit Fouet wrote:

Hi,

On December 1, 2014 11:34:44 PM GMT+01:00, Michael Niedermayer 
michae...@gmx.at wrote:

On Mon, Dec 01, 2014 at 11:41:41AM +0100, Benoit Fouet wrote:

---
Tested against all the materials I have at hand.
There is an artifact showing for

https://raw.githubusercontent.com/maxcom/lorsource/master/src/test/resources/images/i_want_to_be_a_hero__apng_animated__by_tamalesyatole-d5ht8eu.png

which I don't really understand, as it seems the individual frames

are correct

for our decoder, but the disposal that's done for other decoders

(tested

firefox and chrome) is not the same for the end of the cape.
---
  libavcodec/pngdec.c | 93

-

  1 file changed, 71 insertions(+), 22 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 9e52d0b..2ca3dee 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -23,6 +23,7 @@
  
  #include libavutil/bprint.h

  #include libavutil/imgutils.h
+#include libavutil/thread.h
  #include avcodec.h
  #include bytestream.h
  #include internal.h
@@ -38,9 +39,16 @@ typedef struct PNGDecContext {
  AVCodecContext *avctx;
  
  GetByteContext gb;

+ThreadFrame previous_picture;
  ThreadFrame last_picture;
  ThreadFrame picture;
  
+#if CONFIG_APNG_DECODER

+AVMutex mutex;
+int frame_id;
+int *pframe_id;
+#endif

why do you need a mutex ?


Actually, the only thing I need is the frame index. The best place for that 
would be in the demuxer, but I didn't find a place where this information is 
accessible. Did I miss something (I hope so)? Do you think I should be using 
side data for this?
To answer the question though, the access is done is all the decoder threads, 
so I did not want the reset to happen between the reading and the writing of 
the ++.
Thinking more about this, I think it's wrong anyway. I really need the demuxer 
to handle this, it would be simpler and more correct...

iam not sure i understand
considering a single threaded decoder it cannot change anything
for a past or future iteration (because these dont exist anymore or
yet) but only its own state now with a multi threaded decoder it gets
a copy of the previous
decoders state and changes its own state only, nothing else should
change its state so there should be no need for a mutex.


The (flawed) solution I used was to have in each context a pointer to 
the frame index of the first allocated one...



the thread for the next frame would not start before the current
is done with its basic setup of stuff like the frame index

the frame index for each frame is a copy of the last + 1


I'll try this simple approach. I just didn't quite look at how the 
threads were handled, but it seems that just incrementing the frame 
index unconditionnaly would be enough for my use case.



i assume theres enough information in the bitstream for the decoder
to know when to reset the index


yes


  and seeking if ts supported would
call avcodec_flush_buffers()

but quite possibly iam missing something


Doesn't seem so. That was me, just as I suspected...

--
Ben

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

[FFmpeg-devel] [PATCH] libavcodec/pngdec: support 'previous' dispose operation for APNG.

[Benoit Fouet]

---
Tested against all the materials I have at hand.
There is an artifact showing for 
https://raw.githubusercontent.com/maxcom/lorsource/master/src/test/resources/images/i_want_to_be_a_hero__apng_animated__by_tamalesyatole-d5ht8eu.png
which I don't really understand, as it seems the individual frames are correct
for our decoder, but the disposal that's done for other decoders (tested
firefox and chrome) is not the same for the end of the cape.
---
 libavcodec/pngdec.c | 93 -
 1 file changed, 71 insertions(+), 22 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 9e52d0b..2ca3dee 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -23,6 +23,7 @@
 
 #include libavutil/bprint.h
 #include libavutil/imgutils.h
+#include libavutil/thread.h
 #include avcodec.h
 #include bytestream.h
 #include internal.h
@@ -38,9 +39,16 @@ typedef struct PNGDecContext {
 AVCodecContext *avctx;
 
 GetByteContext gb;
+ThreadFrame previous_picture;
 ThreadFrame last_picture;
 ThreadFrame picture;
 
+#if CONFIG_APNG_DECODER
+AVMutex mutex;
+int frame_id;
+int *pframe_id;
+#endif
+
 int state;
 int width, height;
 int cur_w, cur_h;
@@ -827,14 +835,18 @@ static int decode_fctl_chunk(AVCodecContext *avctx, 
PNGDecContext *s,
 return AVERROR_INVALIDDATA;
 
 /* always (re)start with a clean frame */
-if (sequence_number == 0)
+if (sequence_number == 0) {
 s-dispose_op = APNG_DISPOSE_OP_BACKGROUND;
-
-if (s-dispose_op == APNG_DISPOSE_OP_PREVIOUS) {
-av_log(avctx, AV_LOG_ERROR,
-   Dispose operation 'previous' is not yet implemented, using 
'none'.\n);
+ff_mutex_lock(s-mutex);
+*s-pframe_id = 0;
+ff_mutex_unlock(s-mutex);
+} else if (*s-pframe_id == 1  s-dispose_op == APNG_DISPOSE_OP_PREVIOUS)
+/* previous for the second frame is the first frame */
 s-dispose_op = APNG_DISPOSE_OP_NONE;
-}
+
+ff_mutex_lock(s-mutex);
+(*s-pframe_id)++;
+ff_mutex_unlock(s-mutex);
 
 return 0;
 }
@@ -864,8 +876,9 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 {
 int i, j;
 uint8_t *pd  = p-data[0];
-/* TODO make pd_last point to the one before for APNG_DISPOSE_OP_PREVIOUS 
*/
 uint8_t *pd_last = s-last_picture.f-data[0];
+uint8_t *pd_last_region = s-dispose_op == APNG_DISPOSE_OP_PREVIOUS ?
+s-previous_picture.f-data[0] : 
s-last_picture.f-data[0];
 int ls = FFMIN(av_image_get_linesize(p-format, s-width, 0), s-width * 
s-bpp);
 
 if (s-blend_op == APNG_BLEND_OP_OVER 
@@ -876,6 +889,9 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 }
 
 ff_thread_await_progress(s-last_picture, INT_MAX, 0);
+if (s-dispose_op == APNG_DISPOSE_OP_PREVIOUS)
+ff_thread_await_progress(s-previous_picture, INT_MAX, 0);
+
 for (j = 0; j  s-y_offset; j++) {
 for (i = 0; i  ls; i++)
 pd[i] = pd_last[i];
@@ -886,6 +902,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 if (s-dispose_op != APNG_DISPOSE_OP_BACKGROUND  s-blend_op == 
APNG_BLEND_OP_OVER) {
 uint8_t ri, gi, bi, ai;
 
+pd_last_region += s-y_offset * s-image_linesize;
 if (avctx-pix_fmt == AV_PIX_FMT_RGBA) {
 ri = 0;
 gi = 1;
@@ -907,17 +924,17 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 /* output = alpha * foreground + (1-alpha) * background */
 switch (alpha) {
 case 0:
-pd[i+ri] = pd_last[i+ri];
-pd[i+gi] = pd_last[i+gi];
-pd[i+bi] = pd_last[i+bi];
+pd[i+ri] = pd_last_region[i+ri];
+pd[i+gi] = pd_last_region[i+gi];
+pd[i+bi] = pd_last_region[i+bi];
 pd[i+ai] = 0xff;
 break;
 case 255:
 break;
 default:
-pd[i+ri] = FAST_DIV255(alpha * pd[i+ri] + (255 - alpha) * 
pd_last[i+ri]);
-pd[i+gi] = FAST_DIV255(alpha * pd[i+gi] + (255 - alpha) * 
pd_last[i+gi]);
-pd[i+bi] = FAST_DIV255(alpha * pd[i+bi] + (255 - alpha) * 
pd_last[i+bi]);
+pd[i+ri] = FAST_DIV255(alpha * pd[i+ri] + (255 - alpha) * 
pd_last_region[i+ri]);
+pd[i+gi] = FAST_DIV255(alpha * pd[i+gi] + (255 - alpha) * 
pd_last_region[i+gi]);
+pd[i+bi] = FAST_DIV255(alpha * pd[i+bi] + (255 - alpha) * 
pd_last_region[i+bi]);
 pd[i+ai] = 0xff;
 break;
 }
@@ -926,6 +943,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 pd[i] = pd_last[i];
 pd  += s-image_linesize;
 

Re: [FFmpeg-devel] [PATCH] libavcodec/pngdec: support 'previous' dispose operation for APNG.

[Benoit Fouet]

Hi,

On December 1, 2014 11:34:44 PM GMT+01:00, Michael Niedermayer 
michae...@gmx.at wrote:
On Mon, Dec 01, 2014 at 11:41:41AM +0100, Benoit Fouet wrote:
 ---
 Tested against all the materials I have at hand.
 There is an artifact showing for
https://raw.githubusercontent.com/maxcom/lorsource/master/src/test/resources/images/i_want_to_be_a_hero__apng_animated__by_tamalesyatole-d5ht8eu.png
 which I don't really understand, as it seems the individual frames
are correct
 for our decoder, but the disposal that's done for other decoders
(tested
 firefox and chrome) is not the same for the end of the cape.
 ---
  libavcodec/pngdec.c | 93
-
  1 file changed, 71 insertions(+), 22 deletions(-)
 
 diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
 index 9e52d0b..2ca3dee 100644
 --- a/libavcodec/pngdec.c
 +++ b/libavcodec/pngdec.c
 @@ -23,6 +23,7 @@
  
  #include libavutil/bprint.h
  #include libavutil/imgutils.h
 +#include libavutil/thread.h
  #include avcodec.h
  #include bytestream.h
  #include internal.h
 @@ -38,9 +39,16 @@ typedef struct PNGDecContext {
  AVCodecContext *avctx;
  
  GetByteContext gb;
 +ThreadFrame previous_picture;
  ThreadFrame last_picture;
  ThreadFrame picture;
  
 +#if CONFIG_APNG_DECODER
 +AVMutex mutex;
 +int frame_id;
 +int *pframe_id;
 +#endif

why do you need a mutex ?


Actually, the only thing I need is the frame index. The best place for that 
would be in the demuxer, but I didn't find a place where this information is 
accessible. Did I miss something (I hope so)? Do you think I should be using 
side data for this?
To answer the question though, the access is done is all the decoder threads, 
so I did not want the reset to happen between the reading and the writing of 
the ++.
Thinking more about this, I think it's wrong anyway. I really need the demuxer 
to handle this, it would be simpler and more correct...

-- 
Ben

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

Re: [FFmpeg-devel] [PATCH] avcodec/pcm: Increase a/mu-law encoding table sizes

[Benoit Fouet]

Hi,

Le 30/11/2014 13:11, Christophe Gisquet a écrit :

2014-11-30 13:03 GMT+01:00 Michael Niedermayer michae...@gmx.at:

not really, no,
that was also why i posted a patch for this, i wasnt sure this is
worth the extra table size

No strong opinion here, I don't think the increased memory/potential
speed impact are critical, in particular for this codec. Mostly
matters for embedded stuff I guess, but the table seems already too
big to fit in most L1 data caches anyway.


I wouldn't bother if there is no asking for it. Michael, does this patch 
come because of a request you've had?

If not, I don't think it's worth it.
If you still want to apply it, though, don't forget to fix the comment 
in libavcodec/pcm_tablegen.h to 65536 entries per table


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

[FFmpeg-devel] [PATCH] avcodec/pngdec: do not blend on transparent black

[Benoit Fouet]

There is no need to memset the zlib output buffer, as there is no
blending happening there. Instead, do not blend when the dispose
operation is set to 'background' (tranparent black).
---
 libavcodec/pngdec.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 4c9d321..da852c4 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -674,10 +674,6 @@ static int decode_idat_chunk(AVCodecContext *avctx, 
PNGDecContext *s,
 s-crow_buf  = s-buffer + 15;
 s-zstream.avail_out = s-crow_size;
 s-zstream.next_out  = s-crow_buf;
-
-if (avctx-codec_id == AV_CODEC_ID_APNG 
-s-dispose_op == APNG_DISPOSE_OP_BACKGROUND)
-memset(s-zstream.next_out, 0, s-zstream.avail_out);
 }
 s-state |= PNG_IDAT;
 if ((ret = png_decode_idat(s, length))  0)
@@ -887,7 +883,7 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 pd_last += s-image_linesize;
 }
 
-if (s-blend_op == APNG_BLEND_OP_OVER) {
+if (s-dispose_op != APNG_DISPOSE_OP_BACKGROUND  s-blend_op == 
APNG_BLEND_OP_OVER) {
 uint8_t ri, gi, bi, ai;
 
 if (avctx-pix_fmt == AV_PIX_FMT_RGBA) {
-- 
2.2.0.rc2.23.gca0107e

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

[FFmpeg-devel] [PATCH] avcodec/pngdec: allow for some code path optimizations.

[Benoit Fouet]

Use 'if (CONFIG_APNG_DECODER)' where needed, so that the compiler can
optimize out some portion of code.
---
 libavcodec/pngdec.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index da852c4..a330d36 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -949,7 +949,7 @@ static int decode_frame_common(AVCodecContext *avctx, 
PNGDecContext *s,
 for (;;) {
 length = bytestream2_get_bytes_left(s-gb);
 if (length = 0) {
-if (avctx-codec_id == AV_CODEC_ID_APNG  length == 0) {
+if (CONFIG_APNG_DECODER  avctx-codec_id == AV_CODEC_ID_APNG  
length == 0) {
 if (!(s-state  PNG_IDAT))
 return 0;
 else
@@ -984,14 +984,14 @@ static int decode_frame_common(AVCodecContext *avctx, 
PNGDecContext *s,
 goto fail;
 break;
 case MKTAG('f', 'c', 'T', 'L'):
-if (avctx-codec_id != AV_CODEC_ID_APNG)
+if (!CONFIG_APNG_DECODER || avctx-codec_id != AV_CODEC_ID_APNG)
 goto skip_tag;
 if ((ret = decode_fctl_chunk(avctx, s, length))  0)
 goto fail;
 decode_next_dat = 1;
 break;
 case MKTAG('f', 'd', 'A', 'T'):
-if (avctx-codec_id != AV_CODEC_ID_APNG)
+if (!CONFIG_APNG_DECODER || avctx-codec_id != AV_CODEC_ID_APNG)
 goto skip_tag;
 if (!decode_next_dat)
 goto fail;
@@ -999,7 +999,7 @@ static int decode_frame_common(AVCodecContext *avctx, 
PNGDecContext *s,
 length -= 4;
 /* fallthrough */
 case MKTAG('I', 'D', 'A', 'T'):
-if (avctx-codec_id == AV_CODEC_ID_APNG  !decode_next_dat)
+if (CONFIG_APNG_DECODER  avctx-codec_id == AV_CODEC_ID_APNG  
!decode_next_dat)
 goto skip_tag;
 if (decode_idat_chunk(avctx, s, length, p)  0)
 goto fail;
-- 
2.2.0.rc2.23.gca0107e

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

[FFmpeg-devel] [PATCH] avcodec/pngdec: apng: fix buffer index when no blending is needed.

[Benoit Fouet]

---
 libavcodec/pngdec.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index a96667f..0ba2749 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -933,7 +933,11 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 pd  += s-image_linesize;
 pd_last += s-image_linesize;
 }
+} else {
+pd  += s-cur_h * s-image_linesize;
+pd_last += s-cur_h * s-image_linesize;
 }
+
 for (j = s-y_offset + s-cur_h; j  s-height; j++) {
 for (i = 0; i  ls; i++)
 pd[i] = pd_last[i];
-- 
2.2.0

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

Re: [FFmpeg-devel] [PATCH] avcodec/pngdec: apng: fix buffer index when no blending is needed.

[Benoit Fouet]

Please discard, this is wrong...

- Mail original -
 ---
  libavcodec/pngdec.c | 4 
  1 file changed, 4 insertions(+)
 
 diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
 index a96667f..0ba2749 100644
 --- a/libavcodec/pngdec.c
 +++ b/libavcodec/pngdec.c
 @@ -933,7 +933,11 @@ static int handle_p_frame_apng(AVCodecContext
 *avctx, PNGDecContext *s,
  pd  += s-image_linesize;
  pd_last += s-image_linesize;
  }
 +} else {
 +pd  += s-cur_h * s-image_linesize;
 +pd_last += s-cur_h * s-image_linesize;
  }
 +
  for (j = s-y_offset + s-cur_h; j  s-height; j++) {
  for (i = 0; i  ls; i++)
  pd[i] = pd_last[i];
 --
 2.2.0
 
 ___
 ffmpeg-devel mailing list
 ffmpeg-devel@ffmpeg.org
 http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH v2] avcodec/pngdec: apng: fix output buffer filling when no blending is needed.

[Benoit Fouet]

---
 libavcodec/pngdec.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index a330d36..9e52d0b 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -927,7 +927,17 @@ static int handle_p_frame_apng(AVCodecContext *avctx, 
PNGDecContext *s,
 pd  += s-image_linesize;
 pd_last += s-image_linesize;
 }
+} else {
+for (j = s-y_offset; j  s-y_offset + s-cur_h; j++) {
+for (i = 0; i  s-x_offset * s-bpp; i++)
+pd[i] = pd_last[i];
+for (i = (s-x_offset + s-cur_w) * s-bpp; i  ls; i++)
+pd[i] = pd_last[i];
+pd  += s-image_linesize;
+pd_last += s-image_linesize;
+}
 }
+
 for (j = s-y_offset + s-cur_h; j  s-height; j++) {
 for (i = 0; i  ls; i++)
 pd[i] = pd_last[i];
-- 
2.2.0

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

Re: [FFmpeg-devel] [PATCH v2] avcodec/pngdec: apng: fix output buffer filling when no blending is needed.

[Benoit Fouet]

Hi,

Le 28/11/2014 18:33, Michael Niedermayer a écrit :

On Fri, Nov 28, 2014 at 04:54:09PM +0100, Benoit Fouet wrote:

---
  libavcodec/pngdec.c | 10 ++
  1 file changed, 10 insertions(+)

applied

maybe some of this can be factorized with the code above the
else, though iam not sure it would be better


I hesitated to do it, but chose not to. I have no real preference, to be 
honest.


--
Ben

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

[FFmpeg-devel] [PATCH 1/2] avcodec/pngdec: split P frames handling to a separate function.

[Benoit Fouet]

---
 libavcodec/pngdec.c | 29 +
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 35dcd76..8529956 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -825,6 +825,22 @@ static int decode_fctl_chunk(AVCodecContext *avctx, 
PNGDecContext *s,
 return 0;
 }
 
+static void handle_p_frame_png(PNGDecContext *s, AVFrame *p)
+{
+int i, j;
+uint8_t *pd  = p-data[0];
+uint8_t *pd_last = s-last_picture.f-data[0];
+int ls = FFMIN(av_image_get_linesize(p-format, s-width, 0), s-width * 
s-bpp);
+
+ff_thread_await_progress(s-last_picture, INT_MAX, 0);
+for (j = 0; j  s-height; j++) {
+for (i = 0; i  ls; i++)
+pd[i] += pd_last[i];
+pd  += s-image_linesize;
+pd_last += s-image_linesize;
+}
+}
+
 static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
AVFrame *p, AVPacket *avpkt)
 {
@@ -936,18 +952,7 @@ exit_loop:
  s-last_picture.f-height== p-height
  s-last_picture.f-format== p-format
  ) {
-int i, j;
-uint8_t *pd  = p-data[0];
-uint8_t *pd_last = s-last_picture.f-data[0];
-int ls = FFMIN(av_image_get_linesize(p-format, s-width, 0), 
s-width * s-bpp);
-
-ff_thread_await_progress(s-last_picture, INT_MAX, 0);
-for (j = 0; j  s-height; j++) {
-for (i = 0; i  ls; i++)
-pd[i] += pd_last[i];
-pd  += s-image_linesize;
-pd_last += s-image_linesize;
-}
+handle_p_frame_png(s, p);
 }
 }
 ff_thread_report_progress(s-picture, INT_MAX, 0);
-- 
2.2.0.rc2.23.gca0107e

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

[FFmpeg-devel] [PATCH v2] avformat/apngdec: validate frame dimensions.

[Benoit Fouet]

---
 libavformat/apngdec.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index dac71f1..1b59b82 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -295,7 +295,11 @@ static int decode_fctl_chunk(AVFormatContext *s, 
APNGDemuxContext *ctx, AVPacket
 height != s-streams[0]-codec-height ||
 x_offset != 0 ||
 y_offset != 0) {
-if (sequence_number == 0)
+if (sequence_number == 0 ||
+x_offset = s-streams[0]-codec-width ||
+width  s-streams[0]-codec-width - x_offset ||
+y_offset = s-streams[0]-codec-height ||
+height  s-streams[0]-codec-height - y_offset)
 return AVERROR_INVALIDDATA;
 ctx-is_key_frame = 0;
 } else {
-- 
2.2.0.rc2.23.gca0107e

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

Re: [FFmpeg-devel] [PATCH] lavf/apngdec: use AVStream.time_base instead of r_frame_rate

[Benoit Fouet]

Hi,

- Mail original -
 On 25/11/14 6:53 PM, Benoit Fouet wrote:
  i suggest to use set_pts_info to set a reasonable precisse
  timebase something like one millionth or billionth or similar in
  base 2. and then set pts based on these delays and the previous
  timestamp
  
  James, do you want to have a look at this approach or do you want
  me to do it?
 
 I'll leave it to you.
 

OK

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

[FFmpeg-devel] [PATCH] avformat/apngdec: use packet pts and duration instead of altering stream framerate.

[Benoit Fouet]

---
 libavformat/apngdec.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index 1b59b82..276d765 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -44,6 +44,9 @@ typedef struct APNGDemuxContext {
 int max_fps;
 int default_fps;
 
+int64_t pkt_pts;
+int pkt_duration;
+
 int is_key_frame;
 
 /*
@@ -163,6 +166,9 @@ static int apng_read_header(AVFormatContext *s)
 if (!st)
 return AVERROR(ENOMEM);
 
+/* set the timebase to something large enough (1/100,000 of second)
+ * to hopefully cope with all sane frame durations */
+avpriv_set_pts_info(st, 64, 1, 10);
 st-codec-codec_type = AVMEDIA_TYPE_VIDEO;
 st-codec-codec_id   = AV_CODEC_ID_APNG;
 st-codec-width  = avio_rb32(pb);
@@ -266,9 +272,9 @@ static int decode_fctl_chunk(AVFormatContext *s, 
APNGDemuxContext *ctx, AVPacket
 delay_num = 1;
 delay_den = ctx-default_fps;
 }
-s-streams[0]-r_frame_rate.num = delay_den;
-s-streams[0]-r_frame_rate.den = delay_num;
-pkt-duration = 1;
+ctx-pkt_duration = av_rescale_q(delay_num,
+ (AVRational){ 1, delay_den },
+ s-streams[0]-time_base);
 
 av_log(s, AV_LOG_DEBUG, %s: 
 sequence_number: %PRId32, 
@@ -383,6 +389,9 @@ static int apng_read_packet(AVFormatContext *s, AVPacket 
*pkt)
 
 if (ctx-is_key_frame)
 pkt-flags |= AV_PKT_FLAG_KEY;
+pkt-pts = ctx-pkt_pts;
+pkt-duration = ctx-pkt_duration;
+ctx-pkt_pts += ctx-pkt_duration;
 return ret;
 case MKTAG('I', 'E', 'N', 'D'):
 ctx-cur_loop++;
-- 
2.2.0.rc2.23.gca0107e

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

Re: [FFmpeg-devel] [PATCH] avformat/apngdec: transmit all the chunks between consecutive fcTL ones.

[Benoit Fouet]

Hi,

- Mail original -
 On 24/11/14 12:12 PM, Benoit Fouet wrote:
  In order to support multiple IDAT of fdAT chunks following an fcTL
  one,
  transmit all the chunks between two fcTL ones (or between fcTL and
  IEND
  one).
 
 Using one of the samples from
 https://people.mozilla.org/~dolske/apng/demo.html
 
 $ ./ffmpeg -i chompy2.png chompy2.mp4 -loglevel debug
 
 [...]
 
 Input #0, apng, from 'chompy2.png':
   Duration: N/A, bitrate: N/A
 Stream #0:0: Video: apng, rgba, 166x120, 14.58 fps, 10 tbr, 90k
 tbn, 90k tbc
 Output #0, mp4, to 'chompy2.mp4':
   Metadata:
 encoder : Lavf56.15.100
 Stream #0:0, 0, 1/10240: Video: mpeg4 ( [0][0][0] / 0x0020),
 yuv420p, 166x120, 1/10, q=2-31, 200 kb/s, 10 fps, 10240 tbn, 10
 tbc
 Metadata:
   encoder : Lavc56.13.100 mpeg4
 Stream mapping:
   Stream #0:0 - #0:0 (apng (native) - mpeg4 (native))
 Press [q] to stop, [?] for help
 *** dropping frame 8 from stream 0 at ts 5
 *** dropping frame 10 from stream 0 at ts 7
 *** dropping frame 12 from stream 0 at ts 9
 *** dropping frame 14 from stream 0 at ts 11
 [output stream 0:0 @ 00317a20] EOF on sink link output stream
 0:0:default.
 No more output streams to write to, finishing.
 frame=   16 fps=0.0 q=2.3 Lsize=  64kB time=00:00:01.60 bitrate=
 328.6kbits/s dup=0 drop=5
 video:63kB audio:0kB subtitle:0kB other streams:0kB global
 headers:0kB muxing overhead: 1.433775%
 Input file #0 (chompy2.png):
   Input stream #0:0 (video): 21 packets read (288248 bytes); 21
   frames decoded;
   Total: 21 packets (288248 bytes) demuxed
 Output file #0 (chompy2.mp4):
   Output stream #0:0 (video): 16 frames encoded; 16 packets muxed
   (64794 bytes);
   Total: 16 packets (64794 bytes) muxed
 23 frames successfully decoded, 0 decoding errors
 
 I fixed this by using time_base instead of r_frame_rate in
 decode_fctl_chunk, but i'm not sure
 if that's correct either because it's changed after reading every
 frame, and in this one apng
 file the last frame has a different delay_den and delay_num values
 than in every previous frame.
 

Please send a patch, if all works as before, with the case above being fixed, I 
can only think as it being beneficial.

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

[FFmpeg-devel] [PATCH 1/2] avformat/apngdec: account for blend and dispose operations.

[Benoit Fouet]

When the dimensions are the entire frame ones, and the dispose operation
is to reset to background, or the new frame overwrites the new one, then
consider the frame as a key one.
---
 libavformat/apng.h| 41 +
 libavformat/apngdec.c |  6 +-
 2 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 libavformat/apng.h

diff --git a/libavformat/apng.h b/libavformat/apng.h
new file mode 100644
index 000..2abf011
--- /dev/null
+++ b/libavformat/apng.h
@@ -0,0 +1,41 @@
+/*
+ * APNG common header
+ * Copyright (c) 2014 Benoit Fouet
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * @file
+ * APNG common header
+ */
+
+#ifndef AVFORMAT_APNG_H
+#define AVFORMAT_APNG_H
+
+enum {
+   APNG_DISPOSE_OP_NONE   = 0,
+   APNG_DISPOSE_OP_BACKGROUND = 1,
+   APNG_DISPOSE_OP_PREVIOUS   = 2,
+};
+
+enum {
+APNG_BLEND_OP_SOURCE = 0,
+APNG_BLEND_OP_OVER   = 1,
+};
+
+#endif /* AVFORMAT_APNG_H */
diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index d766a87..dac71f1 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -26,6 +26,7 @@
  * @see http://www.w3.org/TR/PNG
  */
 
+#include apng.h
 #include avformat.h
 #include avio_internal.h
 #include internal.h
@@ -298,7 +299,10 @@ static int decode_fctl_chunk(AVFormatContext *s, 
APNGDemuxContext *ctx, AVPacket
 return AVERROR_INVALIDDATA;
 ctx-is_key_frame = 0;
 } else {
-ctx-is_key_frame = 1;
+if (sequence_number == 0  dispose_op == APNG_DISPOSE_OP_PREVIOUS)
+dispose_op = APNG_DISPOSE_OP_BACKGROUND;
+ctx-is_key_frame = dispose_op == APNG_DISPOSE_OP_BACKGROUND ||
+blend_op   == APNG_BLEND_OP_SOURCE;
 }
 
 return 0;
-- 
2.2.0.rc2.23.gca0107e

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

[FFmpeg-devel] [PATCH 2/2] avformat/apngdec: validate frame dimensions.

[Benoit Fouet]

---
 libavformat/apngdec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index dac71f1..e9c87a1 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -295,7 +295,9 @@ static int decode_fctl_chunk(AVFormatContext *s, 
APNGDemuxContext *ctx, AVPacket
 height != s-streams[0]-codec-height ||
 x_offset != 0 ||
 y_offset != 0) {
-if (sequence_number == 0)
+if (sequence_number == 0 ||
+width  + x_offset  s-streams[0]-codec-width ||
+height + y_offset  s-streams[0]-codec-height)
 return AVERROR_INVALIDDATA;
 ctx-is_key_frame = 0;
 } else {
-- 
2.2.0.rc2.23.gca0107e

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

Re: [FFmpeg-devel] [PATCH] lavf/apngdec: use AVStream.time_base instead of r_frame_rate

[Benoit Fouet]

Hi,

On November 25, 2014 10:15:31 PM GMT+01:00, Michael Niedermayer 
michae...@gmx.at wrote:
On Tue, Nov 25, 2014 at 02:56:07PM -0300, James Almer wrote:
 Should fix framedrops on some apng files
 
 Signed-off-by: James Almer jamr...@gmail.com
 ---
 This is still not optimal because the value of time_base will be
updated on every frame, 
 and in some cases delay_num and delay_den varies between frames.
 
 Better fix welcome.
 
  libavformat/apngdec.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
 index d766a87..47d3753 100644
 --- a/libavformat/apngdec.c
 +++ b/libavformat/apngdec.c
 @@ -265,8 +265,8 @@ static int decode_fctl_chunk(AVFormatContext *s,
APNGDemuxContext *ctx, AVPacket
  delay_num = 1;
  delay_den = ctx-default_fps;
  }
 -s-streams[0]-r_frame_rate.num = delay_den;
 -s-streams[0]-r_frame_rate.den = delay_num;
 +s-streams[0]-time_base.num = delay_num;
 +s-streams[0]-time_base.den = delay_den;
  pkt-duration = 1;

this is wrong, the timebase and r_frame_rate are constant
once they have been set


OK, good to know.

i suggest to use set_pts_info to set a reasonable precisse
timebase something like one millionth or billionth or similar in
base 2. and then set pts based on these delays and the previous
timestamp

James, do you want to have a look at this approach or do you want me to do it?

That is unless apng has proper timestamps, in which case they
should be used


It doesn't. Each frame has its duration, which is represented by a fraction of 
a second, and it can very well change between images.

-- 
Ben


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

[FFmpeg-devel] [PATCH] avformat/apngdec: transmit all the chunks between consecutive fcTL ones.

[Benoit Fouet]

In order to support multiple IDAT of fdAT chunks following an fcTL one,
transmit all the chunks between two fcTL ones (or between fcTL and IEND
one).
---
 libavformat/apngdec.c | 30 --
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index 1e0f1c7..756cdd1 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -325,8 +325,6 @@ static int apng_read_packet(AVFormatContext *s, AVPacket 
*pkt)
  * and needed next:
  *  4 (length)
  *  4 (tag (must be fdAT or IDAT))
- *
- *  TODO: support multiple fdAT following an fcTL
  */
 /* if num_play is not 1, then the seekback is already guaranteed */
 if (ctx-num_play == 1  (ret = ffio_ensure_seekback(pb, 46))  0)
@@ -350,15 +348,35 @@ static int apng_read_packet(AVFormatContext *s, AVPacket 
*pkt)
 tag != MKTAG('I', 'D', 'A', 'T'))
 return AVERROR_INVALIDDATA;
 
-if ((ret = avio_seek(pb, -46, SEEK_CUR))  0)
-return ret;
-
 size = 38 /* fcTL */ + 8 /* len, tag */ + len + 4 /* crc */;
 if (size  INT_MAX)
 return AVERROR(EINVAL);
 
-if ((ret = av_get_packet(pb, pkt, size))  0)
+if ((ret = avio_seek(pb, -46, SEEK_CUR))  0 ||
+(ret = av_append_packet(pb, pkt, size))  0)
+return ret;
+
+if (ctx-num_play == 1  (ret = ffio_ensure_seekback(pb, 8))  0)
 return ret;
+
+len = avio_rb32(pb);
+tag = avio_rl32(pb);
+while (tag 
+   tag != MKTAG('f', 'c', 'T', 'L') 
+   tag != MKTAG('I', 'E', 'N', 'D')) {
+if (len  0x7fff)
+return AVERROR_INVALIDDATA;
+if ((ret = avio_seek(pb, -8, SEEK_CUR))  0 ||
+(ret = av_append_packet(pb, pkt, len + 12))  0)
+return ret;
+if (ctx-num_play == 1  (ret = ffio_ensure_seekback(pb, 8))  0)
+return ret;
+len = avio_rb32(pb);
+tag = avio_rl32(pb);
+}
+if ((ret = avio_seek(pb, -8, SEEK_CUR))  0)
+return ret;
+
 if (ctx-is_key_frame)
 pkt-flags |= AV_PKT_FLAG_KEY;
 return ret;
-- 
2.2.0.rc2.23.gca0107e

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

[FFmpeg-devel] [PATCH] avformat/apngdec: transmit all the chunks between consecutive fcTL ones.

[Benoit Fouet]

In order to support multiple IDAT of fdAT chunks following an fcTL one,
transmit all the chunks between two fcTL ones (or between fcTL and IEND
one).
---
 libavformat/apngdec.c | 34 --
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index 1e0f1c7..d766a87 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -24,10 +24,6 @@
  * APNG demuxer.
  * @see https://wiki.mozilla.org/APNG_Specification
  * @see http://www.w3.org/TR/PNG
- *
- * Not supported (yet):
- * - streams with chunks other than fcTL / fdAT / IEND after the first fcTL
- * - streams with multiple fdAT chunks after an fcTL one
  */
 
 #include avformat.h
@@ -325,8 +321,6 @@ static int apng_read_packet(AVFormatContext *s, AVPacket 
*pkt)
  * and needed next:
  *  4 (length)
  *  4 (tag (must be fdAT or IDAT))
- *
- *  TODO: support multiple fdAT following an fcTL
  */
 /* if num_play is not 1, then the seekback is already guaranteed */
 if (ctx-num_play == 1  (ret = ffio_ensure_seekback(pb, 46))  0)
@@ -350,15 +344,35 @@ static int apng_read_packet(AVFormatContext *s, AVPacket 
*pkt)
 tag != MKTAG('I', 'D', 'A', 'T'))
 return AVERROR_INVALIDDATA;
 
-if ((ret = avio_seek(pb, -46, SEEK_CUR))  0)
-return ret;
-
 size = 38 /* fcTL */ + 8 /* len, tag */ + len + 4 /* crc */;
 if (size  INT_MAX)
 return AVERROR(EINVAL);
 
-if ((ret = av_get_packet(pb, pkt, size))  0)
+if ((ret = avio_seek(pb, -46, SEEK_CUR))  0 ||
+(ret = av_append_packet(pb, pkt, size))  0)
+return ret;
+
+if (ctx-num_play == 1  (ret = ffio_ensure_seekback(pb, 8))  0)
+return ret;
+
+len = avio_rb32(pb);
+tag = avio_rl32(pb);
+while (tag 
+   tag != MKTAG('f', 'c', 'T', 'L') 
+   tag != MKTAG('I', 'E', 'N', 'D')) {
+if (len  0x7fff)
+return AVERROR_INVALIDDATA;
+if ((ret = avio_seek(pb, -8, SEEK_CUR))  0 ||
+(ret = av_append_packet(pb, pkt, len + 12))  0)
+return ret;
+if (ctx-num_play == 1  (ret = ffio_ensure_seekback(pb, 8))  0)
+return ret;
+len = avio_rb32(pb);
+tag = avio_rl32(pb);
+}
+if ((ret = avio_seek(pb, -8, SEEK_CUR))  0)
 return ret;
+
 if (ctx-is_key_frame)
 pkt-flags |= AV_PKT_FLAG_KEY;
 return ret;
-- 
2.2.0.rc2.23.gca0107e

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

Re: [FFmpeg-devel] [PATCH] avcodec/pngdec: add APNG support.

[Benoit Fouet]

Hi,

- Mail original -
 On Fri, Nov 21, 2014 at 08:09:50AM -0500, compn wrote:
  On Fri, 21 Nov 2014 12:05:47 +0100
  Benoit Fouet benoit.fo...@free.fr wrote:
  
configure   |   1 +
libavcodec/Makefile |   1 +
libavcodec/allcodecs.c  |   1 +
libavcodec/avcodec.h|   1 +
libavcodec/codec_desc.c |   8 +++
libavcodec/pngdec.c | 139
  
  missing addition of apng to docs.
 
 maybe this should be done when apng support is more complete
 ATM from
 https://people.mozilla.org/~dolske/apng/demo.html
 not all files play yet and not all play correctly
 
 Either way this is very nice work, ill apply it in a moment
 should be easier to improve and review on top of this
 

I'll take some time tomorrow for the docs and changelog entries for the 
demuxer, as my last patch makes it feature complete (to my knowledge at least).
Then I'll work on what I called non key-frames on the decoder side, trying to 
dig a bit more how the dispose and blend operations could be handled.

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

Re: [FFmpeg-devel] [PATCH] avformat/apngdec: transmit all the chunks between consecutive fcTL ones.

[Benoit Fouet]

Hi,

Le 24/11/2014 17:54, James Almer a écrit :

On 24/11/14 12:12 PM, Benoit Fouet wrote:

In order to support multiple IDAT of fdAT chunks following an fcTL one,
transmit all the chunks between two fcTL ones (or between fcTL and IEND
one).

Using one of the samples from https://people.mozilla.org/~dolske/apng/demo.html

$ ./ffmpeg -i chompy2.png chompy2.mp4 -loglevel debug


Just to be sure: the problem you have is present whther or not this 
patch is, right?



[...]

Input #0, apng, from 'chompy2.png':
   Duration: N/A, bitrate: N/A
 Stream #0:0: Video: apng, rgba, 166x120, 14.58 fps, 10 tbr, 90k tbn, 90k 
tbc
Output #0, mp4, to 'chompy2.mp4':
   Metadata:
 encoder : Lavf56.15.100
 Stream #0:0, 0, 1/10240: Video: mpeg4 ( [0][0][0] / 0x0020), yuv420p, 
166x120, 1/10, q=2-31, 200 kb/s, 10 fps, 10240 tbn, 10 tbc
 Metadata:
   encoder : Lavc56.13.100 mpeg4
Stream mapping:
   Stream #0:0 - #0:0 (apng (native) - mpeg4 (native))
Press [q] to stop, [?] for help
*** dropping frame 8 from stream 0 at ts 5
*** dropping frame 10 from stream 0 at ts 7
*** dropping frame 12 from stream 0 at ts 9
*** dropping frame 14 from stream 0 at ts 11
[output stream 0:0 @ 00317a20] EOF on sink link output stream 
0:0:default.
No more output streams to write to, finishing.
frame=   16 fps=0.0 q=2.3 Lsize=  64kB time=00:00:01.60 bitrate= 
328.6kbits/s dup=0 drop=5
video:63kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing 
overhead: 1.433775%
Input file #0 (chompy2.png):
   Input stream #0:0 (video): 21 packets read (288248 bytes); 21 frames decoded;
   Total: 21 packets (288248 bytes) demuxed
Output file #0 (chompy2.mp4):
   Output stream #0:0 (video): 16 frames encoded; 16 packets muxed (64794 
bytes);
   Total: 16 packets (64794 bytes) muxed
23 frames successfully decoded, 0 decoding errors

I fixed this by using time_base instead of r_frame_rate in decode_fctl_chunk, 
but i'm not sure
if that's correct either because it's changed after reading every frame, and in 
this one apng
file the last frame has a different delay_den and delay_num values than in 
every previous frame.


That was the part I'd have loved to get some feedback on.
I have no preference whatsoever on which solution we use for frame 
duration, but it seems rather clear with the issue you get that the one 
I've put in place is not the right one :-)


--
Ben

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

Re: [FFmpeg-devel] [PATCH] lavf/apngdec: print currently unsupported in-stream tags in a more readable form

[Benoit Fouet]

Hi,

On November 22, 2014 5:45:17 PM GMT+01:00, James Almer jamr...@gmail.com 
wrote:
Also use length and not stream position

Signed-off-by: James Almer jamr...@gmail.com
---
 libavformat/apngdec.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index db501ec..1e0f1c7 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -372,8 +372,13 @@ static int apng_read_packet(AVFormatContext *s,
AVPacket *pkt)
 return ret;
 return 0;
 default:
-avpriv_request_sample(s, In-stream tag=%#08X len=%PRId64,
tag, avio_tell(pb));
+{
+char tag_buf[5];
+
+av_get_codec_tag_string(tag_buf, sizeof(tag_buf), tag);
+avpriv_request_sample(s, In-stream tag=%s (0x%08X)
len=%PRIu32, tag_buf, tag, len);
 avio_skip(pb, len + 4);
+}
 }
 
 /* Handle the unsupported yet cases */

LGTM, thanks.

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

[FFmpeg-devel] [PATCH] ffplay: fix mem leak when opening input or parsing options fail.

[Benoit Fouet]

---
 ffplay.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ffplay.c b/ffplay.c
index f79161d..1914a66 100644
--- a/ffplay.c
+++ b/ffplay.c
@@ -3169,8 +3169,9 @@ static int read_thread(void *arg)
 stream_component_close(is, is-video_stream);
 if (is-subtitle_stream = 0)
 stream_component_close(is, is-subtitle_stream);
-if (is-ic) {
-avformat_close_input(is-ic);
+if (ic) {
+avformat_close_input(ic);
+is-ic = NULL;
 }
 
 if (ret != 0) {
-- 
2.2.0.rc2.23.gca0107e

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

Re: [FFmpeg-devel] [PATCH] dashenc: Add a segment_start_number option

[Benoit Fouet]

Hi,

- Mail original -

[...]

 ...in which I screw up and post the same patch. Here you go...
 
 From f06aa763f3e3593d12cc16d8017e796c9e5db3b3 Mon Sep 17 00:00:00
 2001
 From: Rodger Combs rodger.co...@gmail.com
 Date: Thu, 20 Nov 2014 01:47:05 -0600
 Subject: [PATCH] dashenc: Add a segment_start_number option
 
 ---
  libavformat/dashenc.c | 10 ++
  1 file changed, 6 insertions(+), 4 deletions(-)
 
 diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
 index dac217e..549c7c3 100644
 --- a/libavformat/dashenc.c
 +++ b/libavformat/dashenc.c
 @@ -80,6 +80,7 @@ typedef struct DASHContext {
  int total_duration;
  char availability_start_time[100];
  char dirname[1024];
 +int segment_start_number;
  } DASHContext;
  
  static int dash_write(void *opaque, uint8_t *buf, int buf_size)
 @@ -182,10 +183,10 @@ static void dash_free(AVFormatContext *s)
  
  static void output_segment_list(OutputStream *os, AVIOContext *out,
  DASHContext *c)
  {
 -int i, start_index = 0, start_number = 1;
 +int i, start_index = 0, start_number = c-segment_start_number;
  if (c-window_size) {
  start_index  = FFMAX(os-nb_segments   - c-window_size, 0);
 -start_number = FFMAX(os-segment_index - c-window_size, 1);
 +start_number = FFMAX(os-segment_index - c-window_size,
 c-segment_start_number);
  }
  
  if (c-use_template) {
 @@ -193,7 +194,7 @@ static void output_segment_list(OutputStream *os,
 AVIOContext *out, DASHContext
  avio_printf(out, \t\t\t\tSegmentTemplate timescale=\%d\
  , timescale);
  if (!c-use_timeline)
  avio_printf(out, duration=\%d\ , c-last_duration);
 -avio_printf(out,
 initialization=\init-stream$RepresentationID$.m4s\
 media=\chunk-stream$RepresentationID$-$Number%%05d$.m4s\
 startNumber=\%d\\n, c-use_timeline ? start_number : 1);
 +avio_printf(out,
 initialization=\init-stream$RepresentationID$.m4s\
 media=\chunk-stream$RepresentationID$-$Number%%05d$.m4s\
 startNumber=\%d\\n, start_number);
 

Shouldn't this be c-use_timeline ? start_number : c-segment_start_number 
instead?

No other remarks from me.

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

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/pngdec: add APNG support.

[Benoit Fouet]

Hi,

- Mail original -
 On Thu, Nov 20, 2014 at 03:07:17PM +0100, Benoit Fouet wrote:
  ---
   libavcodec/Makefile |   1 +
   libavcodec/allcodecs.c  |   1 +
   libavcodec/avcodec.h|   1 +
   libavcodec/codec_desc.c |   8 +++
   libavcodec/pngdec.c | 142
   +++-
   5 files changed, 150 insertions(+), 3 deletions(-)
 
 apng is missing a dependancy on zlib in configure (see png in
 configure)
 

Fixed locally. Will resend when/if there are no other points to address.

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

Re: [FFmpeg-devel] [PATCH 3/6] Add AV_PIX_FMT_NV12T.

[Benoit Fouet]

Hi,

- Mail original -

[...]

 I think it's safe to assume that if you have a SOC with a HW dec that
 outputs only this insane and uncommon format, the SOC also has a HW
 filter that can convert it.

This is a good argument IMHO.
On a side note, I don't think it would be that complicated to support both, 
through an option.

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

[FFmpeg-devel] [PATCH] avcodec/pngdec: add APNG support.

[Benoit Fouet]

---
Changes:
 - do not reset decode_next_dat when decoding IDAT of fdAT
 - add configure part
---
 configure   |   1 +
 libavcodec/Makefile |   1 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/avcodec.h|   1 +
 libavcodec/codec_desc.c |   8 +++
 libavcodec/pngdec.c | 139 ++--
 6 files changed, 148 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index c013c50..b3c3e5b 100755
--- a/configure
+++ b/configure
@@ -2068,6 +2068,7 @@ amrwb_decoder_select=lsp
 amv_decoder_select=sp5x_decoder exif
 amv_encoder_select=aandcttables mpegvideoenc
 ape_decoder_select=bswapdsp llauddsp
+apng_decoder_select=zlib
 asv1_decoder_select=blockdsp bswapdsp idctdsp
 asv1_encoder_select=bswapdsp fdctdsp pixblockdsp
 asv2_decoder_select=blockdsp bswapdsp idctdsp
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 6c625ce..fa0f53d 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -136,6 +136,7 @@ OBJS-$(CONFIG_AMV_ENCODER) += mjpegenc.o 
mjpeg.o mjpegenc_common.o \
 OBJS-$(CONFIG_ANM_DECODER) += anm.o
 OBJS-$(CONFIG_ANSI_DECODER)+= ansi.o cga_data.o
 OBJS-$(CONFIG_APE_DECODER) += apedec.o
+OBJS-$(CONFIG_APNG_DECODER)+= png.o pngdec.o pngdsp.o
 OBJS-$(CONFIG_SSA_DECODER) += assdec.o ass.o ass_split.o
 OBJS-$(CONFIG_SSA_ENCODER) += assenc.o ass.o
 OBJS-$(CONFIG_ASS_DECODER) += assdec.o ass.o ass_split.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index d08abd8..0d39d33 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -105,6 +105,7 @@ void avcodec_register_all(void)
 REGISTER_ENCDEC (AMV,   amv);
 REGISTER_DECODER(ANM,   anm);
 REGISTER_DECODER(ANSI,  ansi);
+REGISTER_DECODER(APNG,  apng);
 REGISTER_ENCDEC (ASV1,  asv1);
 REGISTER_ENCDEC (ASV2,  asv2);
 REGISTER_DECODER(AURA,  aura);
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index eac3fc7..3323284 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -319,6 +319,7 @@ enum AVCodecID {
 AV_CODEC_ID_HEVC   = MKBETAG('H','2','6','5'),
 #define AV_CODEC_ID_H265 AV_CODEC_ID_HEVC
 AV_CODEC_ID_VP7= MKBETAG('V','P','7','0'),
+AV_CODEC_ID_APNG   = MKBETAG('A','P','N','G'),
 
 /* various PCM codecs */
 AV_CODEC_ID_FIRST_AUDIO = 0x1, /// A dummy id pointing at the 
start of audio codecs
diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
index eeb4505..0af66f4 100644
--- a/libavcodec/codec_desc.c
+++ b/libavcodec/codec_desc.c
@@ -1440,6 +1440,14 @@ static const AVCodecDescriptor codec_descriptors[] = {
 .props = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS,
 .mime_types= MT(image/x-xwindowdump),
 },
+{
+.id= AV_CODEC_ID_APNG,
+.type  = AVMEDIA_TYPE_VIDEO,
+.name  = apng,
+.long_name = NULL_IF_CONFIG_SMALL(APNG (Animated Portable Network 
Graphics) image),
+.props = AV_CODEC_PROP_LOSSLESS,
+.mime_types= MT(image/png),
+},
 
 /* various PCM codecs */
 {
diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 57b73c1..ee6a2ba 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -786,15 +786,55 @@ static void handle_small_bpp(PNGDecContext *s, AVFrame *p)
 }
 }
 
+static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s,
+ uint32_t length)
+{
+uint32_t sequence_number, width, height, x_offset, y_offset;
+
+if (length != 26)
+return AVERROR_INVALIDDATA;
+
+sequence_number = bytestream2_get_be32(s-gb);
+width   = bytestream2_get_be32(s-gb);
+height  = bytestream2_get_be32(s-gb);
+x_offset= bytestream2_get_be32(s-gb);
+y_offset= bytestream2_get_be32(s-gb);
+bytestream2_skip(s-gb, 10); /* delay_num  (2)
+   * delay_den  (2)
+   * dispose_op (1)
+   * blend_op   (1)
+   * crc(4)
+   */
+
+if (width != s-width || height != s-height ||
+x_offset != 0 || y_offset != 0) {
+if (sequence_number == 0)
+return AVERROR_INVALIDDATA;
+avpriv_request_sample(avctx, non key frames);
+return AVERROR_PATCHWELCOME;
+}
+
+return 0;
+}
+
 static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
AVFrame *p, AVPacket *avpkt)
 {
 AVDictionary *metadata  = NULL;
 uint32_t tag, length;
+int decode_next_dat = 0;
+int ret = AVERROR_INVALIDDATA;
 
 for (;;) {
-if (bytestream2_get_bytes_left(s-gb) = 0) {
-av_log(avctx, AV_LOG_ERROR, %d bytes left\n, 

Re: [FFmpeg-devel] [PATCH 1/2] lavf/apngdec: properly skip currently unsupported in-stream tags

[Benoit Fouet]

Hi,

On November 21, 2014 11:09:33 PM GMT+01:00, James Almer jamr...@gmail.com 
wrote:
Signed-off-by: James Almer jamr...@gmail.com
---
 libavformat/apngdec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index 54fbd29..2af87ad 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -373,6 +373,7 @@ static int apng_read_packet(AVFormatContext *s,
AVPacket *pkt)
 return 0;
 default:
avpriv_request_sample(s, In-stream tag=%#08X len=%PRId64, tag,
avio_tell(pb));
+avio_skip(pb, len + 4);
 }
 
 /* Handle the unsupported yet cases */

OK, of course (I have this one in my tree but I forgot to send an update for 
the demuxer when I sent the decoder one...), thanks.

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

Re: [FFmpeg-devel] [PATCH] configure: Prevent icl being incorrectly detected as msvc.

[Benoit Fouet]

Hi,

- Mail original -
 Intel compiler xilink will also output the information from the
 underlying
 Microsoft linker. A result is that both the Intel info header and the
 Microsoft info header are output. This means that currently the Intel
 linker will get detected as the msvc linker incorrectly as configure
 checks
 for the presence of Microsoft first. This patch just changes the
 order of
 detection so that Intel is checked first. This allows intel to be
 detected
 correctly and also fixes an error with lto with icl.
 

LGTM

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

[FFmpeg-devel] [PATCH] avcodec/pngdec: init zlib on decoder init.

[Benoit Fouet]

---
 libavcodec/pngdec.c | 30 --
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 57b73c1..e3d61f6 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -411,11 +411,6 @@ static int decode_zbuf(AVBPrint *bp, const uint8_t *data,
 unsigned buf_size;
 int ret;
 
-zstream.zalloc = ff_png_zalloc;
-zstream.zfree  = ff_png_zfree;
-zstream.opaque = NULL;
-if (inflateInit(zstream) != Z_OK)
-return AVERROR_EXTERNAL;
 zstream.next_in  = (unsigned char *)data;
 zstream.avail_in = data_end - data;
 av_bprint_init(bp, 0, -1);
@@ -437,12 +432,10 @@ static int decode_zbuf(AVBPrint *bp, const uint8_t *data,
 if (ret == Z_STREAM_END)
 break;
 }
-inflateEnd(zstream);
 bp-str[bp-len] = 0;
 return 0;
 
 fail:
-inflateEnd(zstream);
 av_bprint_finalize(bp, NULL);
 return ret;
 }
@@ -924,16 +917,6 @@ static int decode_frame_png(AVCodecContext *avctx,
 
 s-y = s-state = 0;
 
-/* init the zlib */
-s-zstream.zalloc = ff_png_zalloc;
-s-zstream.zfree  = ff_png_zfree;
-s-zstream.opaque = NULL;
-ret = inflateInit(s-zstream);
-if (ret != Z_OK) {
-av_log(avctx, AV_LOG_ERROR, inflateInit returned error %d\n, ret);
-return AVERROR_EXTERNAL;
-}
-
 if ((ret = decode_frame_common(avctx, s, p, avpkt))  0)
 goto the_end;
 
@@ -944,7 +927,6 @@ static int decode_frame_png(AVCodecContext *avctx,
 
 ret = bytestream2_tell(s-gb);
 the_end:
-inflateEnd(s-zstream);
 s-crow_buf = NULL;
 return ret;
 }
@@ -967,6 +949,7 @@ static int update_thread_context(AVCodecContext *dst, const 
AVCodecContext *src)
 static av_cold int png_dec_init(AVCodecContext *avctx)
 {
 PNGDecContext *s = avctx-priv_data;
+int ret;
 
 s-avctx = avctx;
 s-last_picture.f = av_frame_alloc();
@@ -979,6 +962,16 @@ static av_cold int png_dec_init(AVCodecContext *avctx)
 ff_pngdsp_init(s-dsp);
 }
 
+/* init the zlib */
+s-zstream.zalloc = ff_png_zalloc;
+s-zstream.zfree  = ff_png_zfree;
+s-zstream.opaque = NULL;
+ret = inflateInit(s-zstream);
+if (ret != Z_OK) {
+av_log(avctx, AV_LOG_ERROR, inflateInit returned error %d\n, ret);
+return AVERROR_EXTERNAL;
+}
+
 return 0;
 }
 
@@ -996,6 +989,7 @@ static av_cold int png_dec_end(AVCodecContext *avctx)
 s-last_row_size = 0;
 av_freep(s-tmp_row);
 s-tmp_row_size = 0;
+inflateEnd(s-zstream);
 
 return 0;
 }
-- 
2.2.0.rc1.23.gf570943

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

Re: [FFmpeg-devel] [PATCH] avcodec/pngdec: init zlib on decoder init.

[Benoit Fouet]

Hi,

- Mail original -
 ---
  libavcodec/pngdec.c | 30 --
  1 file changed, 12 insertions(+), 18 deletions(-)
 
 diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
 index 57b73c1..e3d61f6 100644
 --- a/libavcodec/pngdec.c
 +++ b/libavcodec/pngdec.c
 @@ -411,11 +411,6 @@ static int decode_zbuf(AVBPrint *bp, const
 uint8_t *data,
  unsigned buf_size;
  int ret;
  
 -zstream.zalloc = ff_png_zalloc;
 -zstream.zfree  = ff_png_zfree;
 -zstream.opaque = NULL;
 -if (inflateInit(zstream) != Z_OK)
 -return AVERROR_EXTERNAL;
  zstream.next_in  = (unsigned char *)data;
  zstream.avail_in = data_end - data;
  av_bprint_init(bp, 0, -1);
 @@ -437,12 +432,10 @@ static int decode_zbuf(AVBPrint *bp, const
 uint8_t *data,
  if (ret == Z_STREAM_END)
  break;
  }
 -inflateEnd(zstream);
  bp-str[bp-len] = 0;
  return 0;
  
  fail:
 -inflateEnd(zstream);
  av_bprint_finalize(bp, NULL);
  return ret;
  }
 

Actually, this one shouldn't be touched, will resend shortly...

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

[FFmpeg-devel] [PATCH] avcodec/pngdec: init zlib on decoder init.

[Benoit Fouet]

---
 libavcodec/pngdec.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index 57b73c1..8467443 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -924,16 +924,6 @@ static int decode_frame_png(AVCodecContext *avctx,
 
 s-y = s-state = 0;
 
-/* init the zlib */
-s-zstream.zalloc = ff_png_zalloc;
-s-zstream.zfree  = ff_png_zfree;
-s-zstream.opaque = NULL;
-ret = inflateInit(s-zstream);
-if (ret != Z_OK) {
-av_log(avctx, AV_LOG_ERROR, inflateInit returned error %d\n, ret);
-return AVERROR_EXTERNAL;
-}
-
 if ((ret = decode_frame_common(avctx, s, p, avpkt))  0)
 goto the_end;
 
@@ -944,7 +934,6 @@ static int decode_frame_png(AVCodecContext *avctx,
 
 ret = bytestream2_tell(s-gb);
 the_end:
-inflateEnd(s-zstream);
 s-crow_buf = NULL;
 return ret;
 }
@@ -967,6 +956,7 @@ static int update_thread_context(AVCodecContext *dst, const 
AVCodecContext *src)
 static av_cold int png_dec_init(AVCodecContext *avctx)
 {
 PNGDecContext *s = avctx-priv_data;
+int ret;
 
 s-avctx = avctx;
 s-last_picture.f = av_frame_alloc();
@@ -979,6 +969,16 @@ static av_cold int png_dec_init(AVCodecContext *avctx)
 ff_pngdsp_init(s-dsp);
 }
 
+/* init the zlib */
+s-zstream.zalloc = ff_png_zalloc;
+s-zstream.zfree  = ff_png_zfree;
+s-zstream.opaque = NULL;
+ret = inflateInit(s-zstream);
+if (ret != Z_OK) {
+av_log(avctx, AV_LOG_ERROR, inflateInit returned error %d\n, ret);
+return AVERROR_EXTERNAL;
+}
+
 return 0;
 }
 
@@ -996,6 +996,7 @@ static av_cold int png_dec_end(AVCodecContext *avctx)
 s-last_row_size = 0;
 av_freep(s-tmp_row);
 s-tmp_row_size = 0;
+inflateEnd(s-zstream);
 
 return 0;
 }
-- 
2.2.0.rc1.23.gf570943

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

Re: [FFmpeg-devel] [PATCH] avcodec/pngdec: init zlib on decoder init.

[Benoit Fouet]

Hi,

- Mail original -
 Le decadi 30 brumaire, an CCXXIII, Benoit Fouet a écrit :
  ---
   libavcodec/pngdec.c | 30 --
   1 file changed, 12 insertions(+), 18 deletions(-)
  
  diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
  index 57b73c1..e3d61f6 100644
  --- a/libavcodec/pngdec.c
  +++ b/libavcodec/pngdec.c
  @@ -411,11 +411,6 @@ static int decode_zbuf(AVBPrint *bp, const
  uint8_t *data,
   unsigned buf_size;
   int ret;
   
  -zstream.zalloc = ff_png_zalloc;
  -zstream.zfree  = ff_png_zfree;
  -zstream.opaque = NULL;
  -if (inflateInit(zstream) != Z_OK)
  -return AVERROR_EXTERNAL;
 
 What happens if one frame contains a damaged zTXt and the next one a
 valid
 one? With the current code, since the zstream is inited each time,
 the first
 one gives whatever it gives and the second one works normally. With
 the
 modified code, I am afraid that the unpredictable state at the end of
 the
 damaged frame will be kept for the good one.
 

I expect the new patch should address your issue.

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

Re: [FFmpeg-devel] [PATCH] avcodec/pngdec: init zlib on decoder init.

[Benoit Fouet]

Hi,

- Mail original -
 Le decadi 30 brumaire, an CCXXIII, Benoit Fouet a écrit :
  ---
   libavcodec/pngdec.c | 23 ---
   1 file changed, 12 insertions(+), 11 deletions(-)
  
  diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
  index 57b73c1..8467443 100644
  --- a/libavcodec/pngdec.c
  +++ b/libavcodec/pngdec.c
  @@ -924,16 +924,6 @@ static int decode_frame_png(AVCodecContext
  *avctx,
   
   s-y = s-state = 0;
   
 
  -/* init the zlib */
  -s-zstream.zalloc = ff_png_zalloc;
  -s-zstream.zfree  = ff_png_zfree;
  -s-zstream.opaque = NULL;
  -ret = inflateInit(s-zstream);
  -if (ret != Z_OK) {
  -av_log(avctx, AV_LOG_ERROR, inflateInit returned error
  %d\n, ret);
  -return AVERROR_EXTERNAL;
  -}
 
  I expect the new patch should address your issue.
 
 AFAICS, the buffer is still inited once and for all and not reset
 between
 frames. Or did I miss something?
 

I did, you didn't.
Actually, I didn't test multiple PNG frames in a container, which would be the 
way to trigger this, I think.

So, I've just tested that, and you are correct. I'll drop this patch.

Thanks for your input!

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

[FFmpeg-devel] [PATCH 2/2] avformat/apngdec: add APNG demuxer.

[Benoit Fouet]

---
 libavformat/Makefile |   1 +
 libavformat/allformats.c |   1 +
 libavformat/apngdec.c| 409 +++
 3 files changed, 411 insertions(+)
 create mode 100644 libavformat/apngdec.c

diff --git a/libavformat/Makefile b/libavformat/Makefile
index 38730c5..c1b5ace 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -76,6 +76,7 @@ OBJS-$(CONFIG_AMR_MUXER) += amr.o
 OBJS-$(CONFIG_ANM_DEMUXER)   += anm.o
 OBJS-$(CONFIG_APC_DEMUXER)   += apc.o
 OBJS-$(CONFIG_APE_DEMUXER)   += ape.o apetag.o img2.o
+OBJS-$(CONFIG_APNG_DEMUXER)  += apngdec.o
 OBJS-$(CONFIG_AQTITLE_DEMUXER)   += aqtitledec.o subtitles.o
 OBJS-$(CONFIG_ASF_DEMUXER)   += asfdec.o asf.o asfcrypt.o \
 avlanguage.o
diff --git a/libavformat/allformats.c b/libavformat/allformats.c
index 3f60d7d..81aab56 100644
--- a/libavformat/allformats.c
+++ b/libavformat/allformats.c
@@ -74,6 +74,7 @@ void av_register_all(void)
 REGISTER_DEMUXER (ANM,  anm);
 REGISTER_DEMUXER (APC,  apc);
 REGISTER_DEMUXER (APE,  ape);
+REGISTER_DEMUXER (APNG, apng);
 REGISTER_DEMUXER (AQTITLE,  aqtitle);
 REGISTER_MUXDEMUX(ASF,  asf);
 REGISTER_MUXDEMUX(ASS,  ass);
diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
new file mode 100644
index 000..54fbd29
--- /dev/null
+++ b/libavformat/apngdec.c
@@ -0,0 +1,409 @@
+/*
+ * APNG demuxer
+ * Copyright (c) 2014 Benoit Fouet
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * @file
+ * APNG demuxer.
+ * @see https://wiki.mozilla.org/APNG_Specification
+ * @see http://www.w3.org/TR/PNG
+ *
+ * Not supported (yet):
+ * - streams with chunks other than fcTL / fdAT / IEND after the first fcTL
+ * - streams with multiple fdAT chunks after an fcTL one
+ */
+
+#include avformat.h
+#include avio_internal.h
+#include internal.h
+#include libavutil/imgutils.h
+#include libavutil/intreadwrite.h
+#include libavutil/opt.h
+#include libavcodec/png.h
+#include libavcodec/bytestream.h
+
+#define DEFAULT_APNG_FPS 15
+
+typedef struct APNGDemuxContext {
+const AVClass *class;
+
+int max_fps;
+int default_fps;
+
+int is_key_frame;
+
+/*
+ * loop options
+ */
+int ignore_loop;
+uint32_t num_frames;
+uint32_t num_play;
+uint32_t cur_loop;
+} APNGDemuxContext;
+
+/*
+ * To be a valid APNG file, we mandate, in this order:
+ * PNGSIG
+ * IHDR
+ * ...
+ * acTL
+ * ...
+ * IDAT
+ */
+static int apng_probe(AVProbeData *p)
+{
+GetByteContext gb;
+int state = 0;
+uint32_t len, tag;
+
+bytestream2_init(gb, p-buf, p-buf_size);
+
+if (bytestream2_get_be64(gb) != PNGSIG)
+return 0;
+
+for (;;) {
+len = bytestream2_get_be32(gb);
+if (len  0x7fff)
+return 0;
+
+tag = bytestream2_get_le32(gb);
+/* we don't check IDAT size, as this is the last tag
+ * we check, and it may be larger than the probe buffer */
+if (tag != MKTAG('I', 'D', 'A', 'T') 
+len  bytestream2_get_bytes_left(gb))
+return 0;
+
+switch (tag) {
+case MKTAG('I', 'H', 'D', 'R'):
+if (len != 13)
+return 0;
+if (av_image_check_size(bytestream2_get_be32(gb), 
bytestream2_get_be32(gb), 0, NULL))
+return 0;
+bytestream2_skip(gb, 9);
+state++;
+break;
+case MKTAG('a', 'c', 'T', 'L'):
+if (state != 1 ||
+len != 8 ||
+bytestream2_get_be32(gb) == 0) /* 0 is not a valid value for 
number of frames */
+return 0;
+bytestream2_skip(gb, 8);
+state++;
+break;
+case MKTAG('I', 'D', 'A', 'T'):
+if (state != 2)
+return 0;
+goto end;
+default:
+/* skip other tags */
+bytestream2_skip(gb, len + 4);
+break;
+}
+}
+
+end:
+return AVPROBE_SCORE_MAX;
+}
+
+static int append_extradata(AVCodecContext *s

[FFmpeg-devel] [PATCH] ffplay: fix mem leak when opening input or parsing options fail.

[Benoit Fouet]

---
 ffplay.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ffplay.c b/ffplay.c
index f79161d..3009c82 100644
--- a/ffplay.c
+++ b/ffplay.c
@@ -3169,8 +3169,8 @@ static int read_thread(void *arg)
 stream_component_close(is, is-video_stream);
 if (is-subtitle_stream = 0)
 stream_component_close(is, is-subtitle_stream);
-if (is-ic) {
-avformat_close_input(is-ic);
+if (ic) {
+avformat_close_input(ic);
 }
 
 if (ret != 0) {
-- 
2.2.0.rc2.23.gca0107e

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

[FFmpeg-devel] [PATCH 00/10] avcodec/pngdec: prepare for APNG support

[Benoit Fouet]

Some janitor work in preparation of APNG support...

Benoit Fouet (10):
  avcodec/pngdec: rename decode_frame to decode_frame_png
  avcodec/pngdec: create a function to decode IHDR chunk.
  avcodec/pngdec: create a function to decode pHYs chunk.
  avcodec/pngdec: create a function to decode IDAT chunk.
  avcodec/pngdec: create a function to decode PLTE chunk.
  avcodec/pngdec: create a function to decode tRNS chunk.
  avcodec/pngdec: fix some indentation/whitespaces
  avcodec/pngdec: use else if instead of if for small bpp handling.
  avcodec/pngdec: create a function to handle small (=4) bits per pixel
values.
  avcodec/pngdec: split frame decoding in its own function.

 libavcodec/pngdec.c | 568 +---
 1 file changed, 312 insertions(+), 256 deletions(-)

-- 
2.2.0.rc1.23.gf570943

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