Re: [FFmpeg-devel] [PATCH] Efficiently support several output pixel formats in Cinepak decoder

[Ronald S. Bultje]

Hi Rune,

On Fri, Feb 3, 2017 at 4:08 AM, <u-9...@aetey.se> wrote:

> On Thu, Feb 02, 2017 at 11:16:35AM -0500, Ronald S. Bultje wrote:
> > On Thu, Feb 2, 2017 at 10:59 AM, <u-9...@aetey.se> wrote:
> > > It is the irregular differences between them which are the reason
> > > for splitting. I would not call this "duplication". If you feel
> > > it is straightforward and important to make this more compact,
> > > with the same performance, just go ahead.
>
> > So, typically, we wouldn't duplicate the code, we'd template it. There's
> > some examples in h264 how to do it. You'd have a single
> (av_always_inline)
> > decode_codebook function, which takes "format" as an argument, and then
> > have three av_noinline callers to it (using fmt=rgb565, fmt=rgb24 or
> > fmt=rgb32).
> >
> > That way performance works as you want it, without the source code
> > duplication.
>
> (Thanks for the pointer. I'll look at how it is done in h264, but: )
>
> I thought about generating the bodies of the functions from something
> like a template but it did not feel like this would make the code more
> understandable aka maintainable. So I wonder if there is any point in
> doing this, given the same binary result.


wm4 has tried to make this point several times, it's about maintainability.
Let me explain, otherwise we'll keep going back and forth.

Let's assume you have a function that does a, b and c, where b depends on
the pix fmt. You're currently writing it as such:

function_565() {
a
b_565
c
}

function_24() {
a
b_24
c
}

function_32() {
a
b_32
c
}

It should be pretty obvious that a and c are triplicated in this example.
Now compare it with this:

av_always_inline function_generic(fmt) {
a
if (fmt == 565) {
b_565
} else if (fmt == 24) {
b_24
} else {
assert(fmt == 32);
b_32
}
c
}

function_565() {
funtion_generic(565);
}

function_24() {
function_generic(24);
}

function_32() {
function_generic(32);
}

It might look larger, but that's merely because we're not writing out a and
c. The key thing here is that we're no longer triplicating a and c. This is
a significant maintenance improvement. Also, because of the inline keywords
used, the performance will be identical.

Conclusion: better to maintain, identical performance. Only advantages, no
disadvantages. Should be easy to accept, right?

> > What we _especially_ don't do in FFmpeg is hardcoding colorspace
> > > > conversion coefficients in decoders, and doing colorspace conversion
> in
> > > > decoders.
> > >
> > > Have you got a suggestion how to do avoid this in this case,
> > > without sacrificing the speed?
>
> > Ah, yes, the question. So, the code change is quite big and it does
> various
> > things, and each of these might have a better alternative or be good
> as-is.
> > fundamentally, I don't really understand how _adding_ a colorspace
> > conversion does any good to speed. It fundamentally always makes things
> > slower. So can you explain why you need to _add_ a colorspace conversion?
>
> It moves the conversion from after the decoder, where the data to convert
> is all and whole frames, to the inside where the conversion applies
> to the codebooks, by the codec design much less than the output.
>
> > Why not just always output the native format? (And then do conversion in
>
> The "native" Cinepak format is actually unknown to swscaler, and I
> seriously doubt it would make sense to add it there, which would
> just largely cut down the decoding efficiency.


I see, so the codebook contains indexed entities that are re-used to
reconstruct the actual frames. That makes some sense. So, what I don't
understand then, is why below you're claiming that get_format() doesn't do
this. this is exactly what get_format() does. Why do you believe
get_format() isn't capable of helping you accomplish this?

> > > +char *out_fmt_override = getenv("CINEPAK_OUTPUT_FORMAT_
> OVERRIDE");
> > >
> > > > Absolutely not acceptable.
> > >
> > > 1. Why?
> > >
> >
> > Libavcodec is a library. Being sensitive to environment in a library, or
> > worse yet, affecting the environment, is typically not what is expected.
> > There are almost always better ways to do the same thing.
>
> I did my best to look for a better way but it does not seem to be existing.


Look into private options, for one. But really, get_format() solves this. I
can elaborate more if you really want me to, as above, but I feel you
haven't really looked into it. I know this feeling, sometimes you've got
something that works for you and you just want to commit it and be done
with it.

But that's not going to happen. The env va

Re: [FFmpeg-devel] [PATCH] Efficiently support several output pixel formats in Cinepak decoder

[Ronald S. Bultje]

Hi Rune,

On Thu, Feb 2, 2017 at 10:59 AM,  wrote:

> On Thu, Feb 02, 2017 at 04:25:05PM +0100, wm4 wrote:
> > > +static void cinepak_decode_codebook_rgb32 (CinepakContext *s,
> > > +cvid_codebook *codebook, int chunk_id, int size, const uint8_t
> *data)
> >
> > > +static void cinepak_decode_codebook_rgb24 (CinepakContext *s,
> > > +cvid_codebook *codebook, int chunk_id, int size, const uint8_t
> *data)
> >
> > > +static void cinepak_decode_codebook_rgb565 (CinepakContext *s,
> > > +cvid_codebook *codebook, int chunk_id, int size, const uint8_t
> *data)
>
> > So a part of the decoder is essentially duplicated for each format?
>
> It is the irregular differences between them which are the reason
> for splitting. I would not call this "duplication". If you feel
> it is straightforward and important to make this more compact,
> with the same performance, just go ahead.
>

So, typically, we wouldn't duplicate the code, we'd template it. There's
some examples in h264 how to do it. You'd have a single (av_always_inline)
decode_codebook function, which takes "format" as an argument, and then
have three av_noinline callers to it (using fmt=rgb565, fmt=rgb24 or
fmt=rgb32).

That way performance works as you want it, without the source code
duplication.

> What we _especially_ don't do in FFmpeg is hardcoding colorspace
> > conversion coefficients in decoders, and doing colorspace conversion in
> > decoders.
>
> Have you got a suggestion how to do avoid this in this case,
> without sacrificing the speed?


Ah, yes, the question. So, the code change is quite big and it does various
things, and each of these might have a better alternative or be good as-is.
fundamentally, I don't really understand how _adding_ a colorspace
conversion does any good to speed. It fundamentally always makes things
slower. So can you explain why you need to _add_ a colorspace conversion?
Why not just always output the native format? (And then do conversion in
GPU if really needed, that is always near-free.)

> > +char *out_fmt_override = getenv("CINEPAK_OUTPUT_FORMAT_OVERRIDE");
>
> > Absolutely not acceptable.
>
> 1. Why?
>

Libavcodec is a library. Being sensitive to environment in a library, or
worse yet, affecting the environment, is typically not what is expected.
There are almost always better ways to do the same thing.

For example, in this case:


> 2. I am open to a better solution of how to choose a format at run time
> when the application lacks the knowledge for choosing the most suitable
> format or does not even try to.
>

wm4 suggested earlier to implement a get_format() function. He meant this:

https://www.ffmpeg.org/doxygen/trunk/structAVCodecContext.html#ae85c5a0e81e9f97c063881148edc28b7

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

Re: [FFmpeg-devel] [PATCH 9/9] boadec: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Mon, Jan 30, 2017 at 8:52 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> Marton proposed a way to mitigate this [1], but you haven't commented
> on it so far.


It doesn't mitigate it. Source code is still an issue, as is binary size if
CONFIG_SMALL is off, which is the default for release builds.

Even if you change CONFIG_SMALL to NDEBUG, the source code issue is still
there.

I don't want this patch. I also don't want to discuss it further. Please
remove the log message. Thank you.

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

Re: [FFmpeg-devel] [PATCH 9/9] boadec: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Sun, Jan 29, 2017 at 7:37 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 29.01.2017 04:46, Ronald S. Bultje wrote:
> > Hm ... So I guess I wasn't clear about this, but the reason I didn't
> reply
> > to other patches with log messages was not because I'm OK with, but
> simply
> > to keep the discussion contained in a single thread and not spam the
> list.
> > I'd prefer if the log msg disappeared from all fuzz-only checks...
>
> Being a "fuzz-only check" is not a well-defined concept. Anything a fuzzer
> does could in principle also happen due to file corruption.


Please stop already. This is ridiculous.

Error messages are supposed to help solve a problem. A corrupt file isn't
solvable. No error message can help. So why bother informing the user?
"Channel count too large" is the same as "fluffybuzz whackabit
mackahooloo". There is no action associated with the message that can help
solve it. This is different from "encryption key missing, please use the
option -key value to specify" for encrypted content.

All we're doing is growing source and binary size and code complexity, and
we do so without any apparent benefit. I just don't think that's a great
approach.

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

Re: [FFmpeg-devel] [PATCH 9/9] boadec: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Sat, Jan 28, 2017 at 7:25 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 27.01.2017 03:27, Michael Niedermayer wrote:
> > On Thu, Jan 26, 2017 at 02:14:09AM +0100, Andreas Cadhalpun wrote:
> >> Signed-off-by: Andreas Cadhalpun 
> >> ---
> >>  libavformat/boadec.c | 14 +-
> >>  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > LGTM
>
> Pushed.


Hm ... So I guess I wasn't clear about this, but the reason I didn't reply
to other patches with log messages was not because I'm OK with, but simply
to keep the discussion contained in a single thread and not spam the list.
I'd prefer if the log msg disappeared from all fuzz-only checks...

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

Re: [FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space

[Ronald S. Bultje]

Hi,

On Wed, Jan 25, 2017 at 8:56 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 26.01.2017 02:26, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Wed, Jan 25, 2017 at 8:20 PM, Andreas Cadhalpun <
> > andreas.cadhal...@googlemail.com> wrote:
> >
> >> On 07.01.2017 13:44, Ronald S. Bultje wrote:
> >>> Doesn't this destroy exporting of newly added colorspaces that have no
> >>> explicit mention yet in libavutil? I'm not 100% sure this is a good
> >> thing.
> >>
> >> Yes. That's how it is already done in libavcodec/h264_ps.c, so it just
> >> makes handling unknown values consistent.
> >
> >
> > Changing h264_ps.c would also make things consistent.
>
> No, because also libavcodec/options_table.h limits the colorspace to known
> values.


That is input only, this is output.

> The question is not whether changing A or B towards the other makes the
> > combination consistent. The question is which form of consistency is
> > better...
>
> As new values don't get added that often, I don't see a problem with
> requiring
> to explicitly add them to libavutil before being able to use them.


That is because your use case for libavcodec is constrained, you use it
only for decoding videos and fuzzing. There are others in this community
that do a lot more than that with libavcodec.

Look, Andreas, I appreciate the work you're doing, I really do, but you
always pick a fight with every review I put out on your code. I don't
understand why. My reviews are not difficult to address, they really are
not. My reviews are actionable, and if the action is taken, I'm happy and
the code can be committed. Why pick a fight? What is the point? Do you
think I'm an idiot that has no clue what he's doing and should be shot down
because of that? Please appreciate that I do have some clue of what I'm
doing, and I am looking out for the health of the project, just like you,
but with a slightly different perspective to some things. If I'm wrong,
please point it out, I make mistakes also, but in cases like these, it can
also help to just drop it and move on. Resolving an issue is not losing, it
is winning.

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

Re: [FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space

[Ronald S. Bultje]

Hi,

On Wed, Jan 25, 2017 at 8:20 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 07.01.2017 13:44, Ronald S. Bultje wrote:
> > On Fri, Jan 6, 2017 at 9:35 PM, Michael Niedermayer
> <mich...@niedermayer.cc>
> > wrote:
> >
> >> On Fri, Jan 06, 2017 at 09:43:24PM +0100, Andreas Cadhalpun wrote:
> >>> On 23.12.2016 00:57, Andreas Cadhalpun wrote:
> >>>> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
> >>>> ---
> >>>>  libavcodec/mpeg12dec.c | 4 
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> >>>> index 63979079c8..d3dc67ad6a 100644
> >>>> --- a/libavcodec/mpeg12dec.c
> >>>> +++ b/libavcodec/mpeg12dec.c
> >>>> @@ -1470,6 +1470,10 @@ static void mpeg_decode_sequence_display_
> extension(Mpeg1Context
> >> *s1)
> >>>>  s->avctx->color_primaries = get_bits(>gb, 8);
> >>>>  s->avctx->color_trc   = get_bits(>gb, 8);
> >>>>  s->avctx->colorspace  = get_bits(>gb, 8);
> >>>> +if (!av_color_space_name(s->avctx->colorspace)) {
> >>>> +av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d,
> >> setting to unspecified\n", s->avctx->colorspace);
> >>>> +s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
> >>>> +}
> >>>>  }
> >>>>  w = get_bits(>gb, 14);
> >>>>  skip_bits(>gb, 1); // marker
> >>>>
> >>>
> >>> Ping for the series.
> >>
> >> i have no real objection to it.
> >> iam used to see these being exported unchanged though so it feels a
> >> bit odd
> >
> >
> > Doesn't this destroy exporting of newly added colorspaces that have no
> > explicit mention yet in libavutil? I'm not 100% sure this is a good
> thing.
>
> Yes. That's how it is already done in libavcodec/h264_ps.c, so it just
> makes handling unknown values consistent.


Changing h264_ps.c would also make things consistent.

The question is not whether changing A or B towards the other makes the
combination consistent. The question is which form of consistency is
better...

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

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavformat/nistspheredec.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
> index 782d1dfbfb..3386497682 100644
> --- a/libavformat/nistspheredec.c
> +++ b/libavformat/nistspheredec.c
> @@ -21,6 +21,7 @@
>
>  #include "libavutil/avstring.h"
>  #include "libavutil/intreadwrite.h"
> +#include "libavcodec/internal.h"
>  #include "avformat.h"
>  #include "internal.h"
>  #include "pcm.h"
> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s)
>  return 0;
>  } else if (!memcmp(buffer, "channel_count", 13)) {
>  sscanf(buffer, "%*s %*s %"SCNd32, >codecpar->channels);
> +if (st->codecpar->channels > FF_SANE_NB_CHANNELS) {
> +av_log(s, AV_LOG_ERROR, "Too many channels %d > %d\n",
> +   st->codecpar->channels, FF_SANE_NB_CHANNELS);
> +return AVERROR(ENOSYS);
> +}


I've said this before, but again - please don't add useless log messages.
These don't help end users at all, since these samples are exceedingly
unlikely to be real. Most likely, they derive from fuzzing, and stderr
cramming is one of the things that makes fuzzing slower.

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

Re: [FFmpeg-devel] GSoC 2017

[Ronald S. Bultje]

Hi,

On Mon, Jan 23, 2017 at 9:39 AM, Michael Niedermayer 
wrote:

> IIRC google suggests that ideas pages are writen and maintained
> year round. Now we have a list but some develoeprs seem to oppose some
> of these old ideas, I would expect that for each objection against an
> old idea a new has to be provided and the one objecting should
> volunteer as mentor for it. Otherwise we might be short on ideas and
> mentors


I don't think this is a good idea. But I'm open to being a mentor this
year. I'll come up with a good idea.

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

Re: [FFmpeg-devel] [PATCH 1/3] lavc/pthread_frame: protect read state access in setup finish function

[Ronald S. Bultje]

Hi,

On Fri, Jan 13, 2017 at 9:39 AM, wm4 <nfx...@googlemail.com> wrote:

> On Fri, 13 Jan 2017 09:07:48 -0500
> "Ronald S. Bultje" <rsbul...@gmail.com> wrote:
>
> > Hi,
> >
> > On Jan 13, 2017 6:40 AM, "Clément Bœsch" <u...@pkh.me> wrote:
> >
> > From: Clément Bœsch <cboe...@gopro.com>
> >
> > ---
> >  libavcodec/pthread_frame.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> > index 7ef5e9f6be..cb6d76284e 100644
> > --- a/libavcodec/pthread_frame.c
> > +++ b/libavcodec/pthread_frame.c
> > @@ -509,11 +509,11 @@ void ff_thread_finish_setup(AVCodecContext
> *avctx) {
> >
> >  if (!(avctx->active_thread_type_THREAD_FRAME)) return;
> >
> > +pthread_mutex_lock(>progress_mutex);
> >  if(p->state == STATE_SETUP_FINISHED){
> >  av_log(avctx, AV_LOG_WARNING, "Multiple ff_thread_finish_setup()
> > calls\n");
> >  }
> >
> > -pthread_mutex_lock(>progress_mutex);
> >
> >
> > This has a problem in that we're potentially calling an I/O function that
> > potentially directly enters application space with a lock held.
>
> I think the user should be taking care of this (or just don't spam
> av_log in a performance critical function). The log call here seems
> more like an internal warning though, and I've never seen it happening.
>

But it's easy to resolve right?

lock();
int state = p->state;
..
unlock();
if (state == ..)
  av_log(..);

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

Re: [FFmpeg-devel] [PATCH 1/3] lavc/pthread_frame: protect read state access in setup finish function

[Ronald S. Bultje]

Hi,

On Jan 13, 2017 6:40 AM, "Clément Bœsch"  wrote:

From: Clément Bœsch 

---
 libavcodec/pthread_frame.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
index 7ef5e9f6be..cb6d76284e 100644
--- a/libavcodec/pthread_frame.c
+++ b/libavcodec/pthread_frame.c
@@ -509,11 +509,11 @@ void ff_thread_finish_setup(AVCodecContext *avctx) {

 if (!(avctx->active_thread_type_THREAD_FRAME)) return;

+pthread_mutex_lock(>progress_mutex);
 if(p->state == STATE_SETUP_FINISHED){
 av_log(avctx, AV_LOG_WARNING, "Multiple ff_thread_finish_setup()
calls\n");
 }

-pthread_mutex_lock(>progress_mutex);


This has a problem in that we're potentially calling an I/O function that
potentially directly enters application space with a lock held.

Can the av_log call be moved under a local variable for the condition to
after the lock is released?

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

Re: [FFmpeg-devel] [PATCH] HTTP: optimize forward seek performance

[Ronald S. Bultje]

Hi Joel,

On Wed, Jan 11, 2017 at 6:01 PM, Joel Cunningham 
wrote:

> Hi,
>
> I’ve been working on optimizing HTTP forward seek performance for ffmpeg
> and would like to contribute this patch into mainline ffmpeg.  Please see
> the below patch for an explanation of the issue and proposed fix.  I have
> provided evidence of the current performance issue and my sample MP4 so
> others can reproduce and observe the behavior.
>
> Files are located in Dropbox here: https://www.dropbox.com/sh/
> 4q4ru8isdv22joj/AABU3XyXmgLMiEFqucf1LdZ3a?dl=0
>
> GRMT0003.MP4 - test video file
> mac-ffplay-baseline.pcapng - wireshark capture of ffplay (49abd) playing
> the above test file on MacOS 10.12.2 from a remote NGINX server
> mac-ffplay-optimize-patch.pcapng - same ffplay setup but with patch
> applied
> ffplay_output.log - console output of ffplay with patch (loglevel debug)
>
> I’m happy to discuss this issue further if the below description doesn’t
> fully communicate the issue.
>
> Thanks,
>
> Joel
>
> From 89a3ed8aab9168313b4f7e83c00857f9b715ba4e Mon Sep 17 00:00:00 2001
> From: Joel Cunningham 
> Date: Wed, 11 Jan 2017 13:55:02 -0600
> Subject: [PATCH] HTTP: optimize forward seek performance
>
> This commit optimizes HTTP forward seeks by advancing the stream on
> the current connection when the seek amount is within the current
> TCP window rather than closing the connection and opening a new one.
> This improves performance because with TCP flow control, a window's
> worth of data is always either in the local socket buffer already or
> in-flight from the sender.
>
> The previous behavior of closing the connection, then opening a new
> with a new HTTP range value results in a massive amounts of discarded
> and re-sent data when large TCP windows are used.  This has been observed
> on MacOS/iOS which starts with an inital window of 256KB and grows up to
> 1MB depending on the bandwidth-product delay.
>
> When seeking within a window's worth of data and we close the connection,
> then open a new one within the same window's worth of data, we discard
> from the current offset till the end of the window.  Then on the new
> connection the server ends up re-sending the previous data from new
> offset till the end of old window.
>
> Example:
>
> TCP window size: 64KB
> Position: 32KB
> Forward seek position: 40KB
>
>   *  (Next window)
> 32KB |--| 96KB |---| 160KB
> *
>   40KB |---| 104KB
>
> Re-sent amount: 96KB - 40KB = 56KB
>
> For a real world test example, I have MP4 file of ~25MB, which ffplay
> only reads ~16MB and performs 177 seeks. With current ffmpeg, this results
> in 177 HTTP GETs and ~73MB worth of TCP data communication.  With this
> patch, ffmpeg issues 4 HTTP GETs for a total of ~20MB of TCP data
> communication.
>
> To support this feature, a new URL function has been added to get the
> stream buffer size from the TCP protocol.  The stream advancement logic
> has been implemented in the HTTP layer since this the layer in charge of
> the seek and creating/destroying the TCP connections.
>
> This feature has been tested on Windows 7 and MacOS/iOS.  Windows support
> is slightly complicated by the fact that when TCP window auto-tuning is
> enabled, SO_RCVBUF doesn't report the real window size, but it does if
> SO_RCVBUF was manually set (disabling auto-tuning). So we can only use
> this optimization on Windows in the later case
> ---
>  libavformat/avio.c |  7 ++
>  libavformat/http.c | 69 ++
> 
>  libavformat/tcp.c  | 21 +
>  libavformat/url.h  |  8 +++
>  4 files changed, 105 insertions(+)


Very interesting. There's some minor stylistic nits (double brackets where
there shouldn't be, I didn't check super-closely for that), but overall
this is a pretty thoughtful patch in what it's trying to do.

As for Windows, I'm surprised that there wouldn't be any API to get the
real current window size. I mean, I understand that they would decrease
window size on-demand, but I would expect them to do that w/o throwing out
already-downloaded data - so essentially I expect delayed auto-tuning. In
that case, getting the current window size should be trivial and valid
until the next read. Very strange. I suppose no workarounds (perhaps with
windows-specific API) exist? Also, what is the practical implication? Does
it work as before? Or better but not optimal? Or worse?

I'm wondering if there's some way to make the interaction between tcp and
http less awkward. Similar problems exist between rtp/udp, where we want
deeper protocol integration and the generic API basically inhibits us.
Probably out of scope for this patch and not a terribly big deal because
the URL API is currently not exposed, but we eventually want to expose this
API (IMO) so at some point this will need some more thoughts.

If nobody else has comments, I'm 

Re: [FFmpeg-devel] [PATCH] avutil/frame: Remove requirements to use accessors for AVFrame

[Ronald S. Bultje]

Hi,

On Mon, Jan 9, 2017 at 8:48 AM, wm4 <nfx...@googlemail.com> wrote:

> On Mon, 9 Jan 2017 08:17:08 -0500
> "Ronald S. Bultje" <rsbul...@gmail.com> wrote:
>
> > Hi,
> >
> > On Mon, Jan 9, 2017 at 7:15 AM, wm4 <nfx...@googlemail.com> wrote:
> >
> > > /**
> > >  * Not to be accessed directly from outside libavutil
> > >  */
> > > AVBufferRef *qp_table_buf;
> > >
> > > The comment should be adjusted and allow direct access. Also,
> > > it should be moved out of the FF_API_FRAME_QP ifdef (why was
> > > it under it anyway).
> >
> >
> > I disagree. FF_API_FRAME_QP is not something that exists or is useful
> > outside a handful of codecs (basically mpeg-1/2/4). If we want this to be
> > public API, it should exist in a way that makes it useful to the majority
> > of codecs that use quantization. I'm speaking codecs not developed by
> > people that implemented qp_table_buf, for example vp3/5/6/8/9 etc.
> >
> > This also means we need a useful way to normalization the quantizer to a
> > universal scale. Exporting it as SCALE_MPEG, SCALE_H264 etc. is not
> useful.
> > We need a universal Q so -sameq has some kind of meaning when re-encoding
> > mpeg-4 to vp9. Also, aq visualization in libavfilter would suddenly be
> > meaningful across codecs. Etc.
>
> Ah, I thought the QP API survived, but now I see that the accessors are
> within FF_API_FRAME_QP as well. So this is not a bug like I thought,
> but intentional, and all QP retrieval will be removed after the API
> bump. Just strange that the accessors have no attribute_deprecated.


I agree that's strange. We should probably add them...

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

Re: [FFmpeg-devel] [PATCH] avutil/frame: Remove requirements to use accessors for AVFrame

[Ronald S. Bultje]

Hi,

On Mon, Jan 9, 2017 at 7:15 AM, wm4  wrote:

> /**
>  * Not to be accessed directly from outside libavutil
>  */
> AVBufferRef *qp_table_buf;
>
> The comment should be adjusted and allow direct access. Also,
> it should be moved out of the FF_API_FRAME_QP ifdef (why was
> it under it anyway).


I disagree. FF_API_FRAME_QP is not something that exists or is useful
outside a handful of codecs (basically mpeg-1/2/4). If we want this to be
public API, it should exist in a way that makes it useful to the majority
of codecs that use quantization. I'm speaking codecs not developed by
people that implemented qp_table_buf, for example vp3/5/6/8/9 etc.

This also means we need a useful way to normalization the quantizer to a
universal scale. Exporting it as SCALE_MPEG, SCALE_H264 etc. is not useful.
We need a universal Q so -sameq has some kind of meaning when re-encoding
mpeg-4 to vp9. Also, aq visualization in libavfilter would suddenly be
meaningful across codecs. Etc.

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

Re: [FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space

[Ronald S. Bultje]

Hi,

On Fri, Jan 6, 2017 at 9:35 PM, Michael Niedermayer 
wrote:

> On Fri, Jan 06, 2017 at 09:43:24PM +0100, Andreas Cadhalpun wrote:
> > On 23.12.2016 00:57, Andreas Cadhalpun wrote:
> > > Signed-off-by: Andreas Cadhalpun 
> > > ---
> > >  libavcodec/mpeg12dec.c | 4 
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> > > index 63979079c8..d3dc67ad6a 100644
> > > --- a/libavcodec/mpeg12dec.c
> > > +++ b/libavcodec/mpeg12dec.c
> > > @@ -1470,6 +1470,10 @@ static void 
> > > mpeg_decode_sequence_display_extension(Mpeg1Context
> *s1)
> > >  s->avctx->color_primaries = get_bits(>gb, 8);
> > >  s->avctx->color_trc   = get_bits(>gb, 8);
> > >  s->avctx->colorspace  = get_bits(>gb, 8);
> > > +if (!av_color_space_name(s->avctx->colorspace)) {
> > > +av_log(s->avctx, AV_LOG_WARNING, "Invalid color space %d,
> setting to unspecified\n", s->avctx->colorspace);
> > > +s->avctx->colorspace = AVCOL_SPC_UNSPECIFIED;
> > > +}
> > >  }
> > >  w = get_bits(>gb, 14);
> > >  skip_bits(>gb, 1); // marker
> > >
> >
> > Ping for the series.
>
> i have no real objection to it.
> iam used to see these being exported unchanged though so it feels a
> bit odd


Doesn't this destroy exporting of newly added colorspaces that have no
explicit mention yet in libavutil? I'm not 100% sure this is a good thing.

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

Re: [FFmpeg-devel] [PATCH 4/9] genh: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Fri, Jan 6, 2017 at 8:43 PM, Michael Niedermayer 
wrote:

> On Fri, Jan 06, 2017 at 08:48:02PM +0100, Andreas Cadhalpun wrote:
> > Signed-off-by: Andreas Cadhalpun 
> > ---
> >  libavformat/genh.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/genh.c b/libavformat/genh.c
> > index b683e026d1..6ce2588ed3 100644
> > --- a/libavformat/genh.c
> > +++ b/libavformat/genh.c
> > @@ -74,6 +74,7 @@ static int genh_read_header(AVFormatContext *s)
> >  case  0: st->codecpar->codec_id = AV_CODEC_ID_ADPCM_PSX;
> break;
> >  case  1:
> >  case 11: st->codecpar->bits_per_coded_sample = 4;
> > + FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels > INT_MAX
> / 36)
> >   st->codecpar->block_align = 36 * st->codecpar->channels;
> >   st->codecpar->codec_id = AV_CODEC_ID_ADPCM_IMA_WAV;
> break;
> >  case  2: st->codecpar->codec_id = AV_CODEC_ID_ADPCM_DTK;
> break;
>
> i see a channels * 1024 in genh_read_packet()
> is the added check sufficient ?
>
> also i think we should ask for a sample for a file that has a
> channel count beyond normal bounds


Not in this code. Such generic channel sanity checks belong in utils.c, not
here, and should not be invoked by the demuxer explicitly, but always run
as integral part of read_header or add_stream or so.

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

Re: [FFmpeg-devel] [PATCH 6/9] nistspheredec: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Fri, Jan 6, 2017 at 5:30 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 06.01.2017 22:30, Ronald S. Bultje wrote:
> > On Fri, Jan 6, 2017 at 2:48 PM, Andreas Cadhalpun <
> > andreas.cadhal...@googlemail.com> wrote:
> >
> >> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
> >> ---
> >>  libavformat/nistspheredec.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
> >> index 782d1dfbfb..9023ed7fc7 100644
> >> --- a/libavformat/nistspheredec.c
> >> +++ b/libavformat/nistspheredec.c
> >> @@ -80,6 +80,7 @@ static int nist_read_header(AVFormatContext *s)
> >>
> >>  avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
> >>
> >> +FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels &&
> >> st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
> >
> >
> > Same comment as the other one, the channels == 0 isn't a valid case that
> we
> > want to special case, probably check that it's not zero separately.
>
> Here I disagree: This is only the demuxer, that doesn't need to know
> the number of channels, which can be set later by the decoder.
> (For example the shorten decoder does this.)


Hm ... I don't like how we're adding special-case code for hypothetical
cases that can only come from entirely broken muxers or fuzzers. I'll leave
it to others to break the tie.

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

Re: [FFmpeg-devel] [PATCH 5/9] ircamdec: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Fri, Jan 6, 2017 at 2:48 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavformat/ircamdec.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/ircamdec.c b/libavformat/ircamdec.c
> index 59f3a49411..f3cf4d0dc9 100644
> --- a/libavformat/ircamdec.c
> +++ b/libavformat/ircamdec.c
> @@ -96,6 +96,7 @@ static int ircam_read_header(AVFormatContext *s)
>  }
>
>  st->codecpar->bits_per_coded_sample = av_get_bits_per_sample(st->
> codecpar->codec_id);
> +FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels &&
> st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
>  st->codecpar->block_align = st->codecpar->bits_per_coded_sample *
> st->codecpar->channels / 8;
>  avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
>  avio_skip(s->pb, 1008);


I see this code a few lines up:

if (!channels || !sample_rate)
return AVERROR_INVALIDDATA;

So channels == 0 seems impossible to me.

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

Re: [FFmpeg-devel] [PATCH 6/9] nistspheredec: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Fri, Jan 6, 2017 at 2:48 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavformat/nistspheredec.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/nistspheredec.c b/libavformat/nistspheredec.c
> index 782d1dfbfb..9023ed7fc7 100644
> --- a/libavformat/nistspheredec.c
> +++ b/libavformat/nistspheredec.c
> @@ -80,6 +80,7 @@ static int nist_read_header(AVFormatContext *s)
>
>  avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
>
> +FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels &&
> st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)


Same comment as the other one, the channels == 0 isn't a valid case that we
want to special case, probably check that it's not zero separately.

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

Re: [FFmpeg-devel] [PATCH 2/9] 4xm: prevent overflow during block alignment calculation

[Ronald S. Bultje]

Hi,

On Fri, Jan 6, 2017 at 2:47 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavformat/4xm.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
> index 2758b69d29..45949c4e97 100644
> --- a/libavformat/4xm.c
> +++ b/libavformat/4xm.c
> @@ -187,6 +187,7 @@ static int parse_strk(AVFormatContext *s,
>  st->codecpar->bit_rate  = (int64_t)st->codecpar->channels
> *
>st->codecpar->sample_rate *
>st->codecpar->bits_per_coded_
> sample;
> +FF_RETURN_ON_OVERFLOW(s, st->codecpar->channels &&
> st->codecpar->bits_per_coded_sample > INT_MAX / st->codecpar->channels)
>  st->codecpar->block_align   = st->codecpar->channels *
>st->codecpar->bits_per_coded_
> sample;
>
> --
> 2.11.0


To an innocent reader (who doesn't know/care about SIGFPE), this might look
like channels = 0 is an actual valid decoder condition that is explicitly
handled here.

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

Re: [FFmpeg-devel] libavcodec memory usage

[Ronald S. Bultje]

Hi,

On Tue, Jan 3, 2017 at 4:35 PM, Matthieu Beghin <
matthieu.beg...@garagecube.com> wrote:

> Hi,
>
> > On 03 Jan 2017, at 18:26, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, Jan 3, 2017 at 12:16 PM, Matthieu Beghin <
> > matthieu.beg...@garagecube.com> wrote:
> >
> >>
> >>> On 03 Jan 2017, at 17:55, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On Tue, Jan 3, 2017 at 11:44 AM, Matthieu Beghin <
> >>> matthieu.beg...@garagecube.com> wrote:
> >>>
> >>>>> I'm assuming that you're talking about 8K H264 with pixfmt=yuv420p10?
> >> 8K
> >>>> yuv420p10 frames are 100MB
> >>>>
> >>>> Correct
> >>>>
> >>>>> 32 plus delayed output and current_pic gives H264_MAX_PICTURE_COUNT =
> >> 36
> >>>> without threading: 36*100=3.6GB.
> >>>>
> >>>> What do you mean by "delayed output” ? Can you link me to a document ?
> >>>
> >>>
> >>> Out-of-order coded P-frames, they are buffered internally but not
> counted
> >>> (and thus extra) on top of the max_refs value. I believe the spec calls
> >>> this dpb (delayed pic buffer).
> >>>
> >>>> Or maybe consider ditching 32bit support?
> >>>>
> >>>> Unfortunately, that’s 10 years old app using OSX Carbon library (not
> 64
> >>>> bits), and rewriting that part would be too much work.
> >>>>
> >>>
> >>> I'm frowning at you. Really badly.
> >>>
> >>> As an alternative, is there a way to know how much bytes are buffered ?
> >>>> Iterating buffered frames ?
> >>>
> >>>
> >>> What are you hoping to accomplish by knowing how many bytes are
> buffered?
> >>> It doesn't solve the problem? But anyway, check
> >>> H264Context->short/long_ref[]; delayed_pic[] gives you the delayed
> >> output.
> >>>
> >>
> >> I’m trying to let people use 8k movies if it will fit, but in case it
> >> would use more than 1 GB, refuse to load the file. My application is a
> live
> >> application, it cannot crash, and actually, it can...
> >>
> >> So to know how much bytes livavcodec is using, I have to know the number
> >> of frames / field, if it is interlaced and the number of delayed frames,
> >> that's correct ?
> >>
> >> How can I know the number of frames / field ?
> >> To know if it’s interlaced: AVFrame::interlaced_frame
> >> Number of delayed frames ?
> >>
> >> Another solution is to start playing the movie and to check the buffered
> >> data amount each time I decode a frame, then stop the movie if it uses
> more
> >> than 1 GB...
> >> In this case, just iterating H264Context->short/long_ref[];
> delayed_pic[]
> >> would be enough ?
> >> How can I get the H264Context ?
> >
> >
> > You can't access H264Context outside libavcodec without hacks...
> >
> > Does something in libavcodec crash if you use too much mem? (If so: what?
> > We want to fix this...) Or something else?
>
> I’m generally blocked by crashes in Apple nVidia GL driver. When I’m not
> blocked by my own assertions (I should use a macro…) but anyway if the GL
> driver calls crash I can’t reach memory limit.


Hm, right... OK, yeah, I understand the issue, but I somehow doubt there's
an easy way to resolve that that isn't disgustingly hacky on our side.
Really, if they crash, they need to provide a solution. If it is critical
that it doesn't crash, you most likely want to run that portion as a
separate process with shmem/whatever so if it crashes, your app stays
alive, but I'm obviously not in a position to make design recommendations
to you. You know that code better than me ;)

> The idea is that on OOM, libavcodec mallocs fail and we return gracefully.
> > If that doesn't happen, we want to fix it.
>
> If I have any crash in FFMPEG when reaching memory limit, I’ll document it
> and send you the info.


Thanks :)

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

Re: [FFmpeg-devel] libavcodec memory usage

[Ronald S. Bultje]

Hi,

On Tue, Jan 3, 2017 at 12:16 PM, Matthieu Beghin <
matthieu.beg...@garagecube.com> wrote:

>
> > On 03 Jan 2017, at 17:55, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, Jan 3, 2017 at 11:44 AM, Matthieu Beghin <
> > matthieu.beg...@garagecube.com> wrote:
> >
> >>> I'm assuming that you're talking about 8K H264 with pixfmt=yuv420p10?
> 8K
> >> yuv420p10 frames are 100MB
> >>
> >> Correct
> >>
> >>> 32 plus delayed output and current_pic gives H264_MAX_PICTURE_COUNT =
> 36
> >> without threading: 36*100=3.6GB.
> >>
> >> What do you mean by "delayed output” ? Can you link me to a document ?
> >
> >
> > Out-of-order coded P-frames, they are buffered internally but not counted
> > (and thus extra) on top of the max_refs value. I believe the spec calls
> > this dpb (delayed pic buffer).
> >
> >> Or maybe consider ditching 32bit support?
> >>
> >> Unfortunately, that’s 10 years old app using OSX Carbon library (not 64
> >> bits), and rewriting that part would be too much work.
> >>
> >
> > I'm frowning at you. Really badly.
> >
> > As an alternative, is there a way to know how much bytes are buffered ?
> >> Iterating buffered frames ?
> >
> >
> > What are you hoping to accomplish by knowing how many bytes are buffered?
> > It doesn't solve the problem? But anyway, check
> > H264Context->short/long_ref[]; delayed_pic[] gives you the delayed
> output.
> >
>
> I’m trying to let people use 8k movies if it will fit, but in case it
> would use more than 1 GB, refuse to load the file. My application is a live
> application, it cannot crash, and actually, it can...
>
> So to know how much bytes livavcodec is using, I have to know the number
> of frames / field, if it is interlaced and the number of delayed frames,
> that's correct ?
>
> How can I know the number of frames / field ?
> To know if it’s interlaced: AVFrame::interlaced_frame
> Number of delayed frames ?
>
> Another solution is to start playing the movie and to check the buffered
> data amount each time I decode a frame, then stop the movie if it uses more
> than 1 GB...
> In this case, just iterating H264Context->short/long_ref[]; delayed_pic[]
> would be enough ?
> How can I get the H264Context ?


You can't access H264Context outside libavcodec without hacks...

Does something in libavcodec crash if you use too much mem? (If so: what?
We want to fix this...) Or something else?

The idea is that on OOM, libavcodec mallocs fail and we return gracefully.
If that doesn't happen, we want to fix it.

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

Re: [FFmpeg-devel] libavcodec memory usage

[Ronald S. Bultje]

Hi,

On Tue, Jan 3, 2017 at 11:44 AM, Matthieu Beghin <
matthieu.beg...@garagecube.com> wrote:

> > I'm assuming that you're talking about 8K H264 with pixfmt=yuv420p10? 8K
> yuv420p10 frames are 100MB
>
> Correct
>
> > 32 plus delayed output and current_pic gives H264_MAX_PICTURE_COUNT = 36
> without threading: 36*100=3.6GB.
>
> What do you mean by "delayed output” ? Can you link me to a document ?


Out-of-order coded P-frames, they are buffered internally but not counted
(and thus extra) on top of the max_refs value. I believe the spec calls
this dpb (delayed pic buffer).

> Or maybe consider ditching 32bit support?
>
> Unfortunately, that’s 10 years old app using OSX Carbon library (not 64
> bits), and rewriting that part would be too much work.
>

I'm frowning at you. Really badly.

As an alternative, is there a way to know how much bytes are buffered ?
> Iterating buffered frames ?


What are you hoping to accomplish by knowing how many bytes are buffered?
It doesn't solve the problem? But anyway, check
H264Context->short/long_ref[]; delayed_pic[] gives you the delayed output.

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

Re: [FFmpeg-devel] libavcodec memory usage

[Ronald S. Bultje]

Hi,

On Tue, Jan 3, 2017 at 10:58 AM, Matthieu Beghin <
matthieu.beg...@garagecube.com> wrote:

> Hi,
>
> My application is using libavcodec etc. to playback movies. Everything is
> ok with our 64 bits app. But I have a 32 bits app and there when playing a
> 8k we reach 3 GB memory usage, so when adding another 4k or a few HD
> movies, the app crashes.
>
> I haven’t found a way to reduce memory usage. The library stores lots of
> frames or data. I tried reducing thread_count to 1 to test, but it does not
> help and performances are not acceptable.
>
> Is there a way to limit the size of buffered data / number of buffered
> frames ?
> I’m using FFMPEG 2.8.7 (libavcodec.56.60.100 / libavformat.56.40.101).


I'm assuming that you're talking about 8K H264 with pixfmt=yuv420p10? 8K
yuv420p10 frames are 100MB, H264 can have up to 16 refs/field, for
interlaced content this means 32 plus delayed output and current_pic gives
H264_MAX_PICTURE_COUNT = 36 without threading: 36*100=3.6GB.

There's not much you can do about this in the decoder side. You can tell
your encoder to use less references and discard references earlier. How to
do this depends on which encoder you're using.

Or maybe consider ditching 32bit support?

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

Re: [FFmpeg-devel] [PATCH] avcodec/x86/vc1dsp_mc: Fix build with NASM 2.09.10

[Ronald S. Bultje]

Hi,

On Mon, Jan 2, 2017 at 3:35 PM, Michael Niedermayer 
wrote:

> make fate passes
>
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/x86/vc1dsp_mc.asm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/x86/vc1dsp_mc.asm b/libavcodec/x86/vc1dsp_mc.asm
> index 7eaf043964..175c397ae9 100644
> --- a/libavcodec/x86/vc1dsp_mc.asm
> +++ b/libavcodec/x86/vc1dsp_mc.asm
> @@ -155,7 +155,7 @@ cglobal vc1_%2_hor_16b_shift2, 4, 5, 0, dst, stride,
> src, rnd, h
>  movhq, 8
>  sub  srcq, 2
>  sub  rndd, (-1+9+9-1) * 1024 ; add -1024 bias
> -LOAD_ROUNDER_MMX rndq
> +LOAD_ROUNDER_MMX rndd
>  mova   m5, [pw_9]
>  mova   m6, [pw_128]
>  pxor   m0, m0
> --
> 2.11.0
>

LGTM.

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

Re: [FFmpeg-devel] PATCH for building with nasm

[Ronald S. Bultje]

Hi,

On Sun, Jan 1, 2017 at 8:36 PM, Michael Niedermayer 
wrote:

> On Sun, Jan 01, 2017 at 08:16:00AM +, Rostislav Pehlivanov wrote:
> > On 1 January 2017 at 07:52, John Comeau  wrote:
> >
> > > fixes `operation size not specified` errors as described here:
> > > http://stackoverflow.com/questions/36854583/compiling-
> > > ffmpeg-for-kali-linux-2
> > >
> > > I rebuilt again with yasm and made sure it didn't break that.
> > > --
> > > John Comeau KE5TFZ j...@unternet.net http://jc.unternet.net/
> > > "A place for everything, and everything all over the place"
> > >
> > > ___
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > >
> > Odd, I've been using nasm to build ffmpeg for years now.
> > What version do you use? It sure works on nasm 2.12.01 here.
>
> do you see a disadvantage in the change by the patch ?
> if not, then i think it should be applied as it seems improving
> support for some nasm version be that the only issue or one of
> several


I think it's fine to merge.

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

Re: [FFmpeg-devel] [PATCH 1/2] wmavoice: truncate spillover_nbits if too large

[Ronald S. Bultje]

Hi,

On Sun, Jan 1, 2017 at 5:55 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 01.01.2017 23:27, Ronald S. Bultje wrote:
> > On Sun, Jan 1, 2017 at 5:18 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com <mailto:andreas.cadhal...@googlemail.com>>
> wrote:
> >
> > This fixes triggering the av_assert0(ret <= tmp.size).
> >
> > The problem was reintroduced by commit
> > 7b27dd5c16de785297ce4de4b88afa0b6685f61d and originally fixed by
> > 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e.
> >
> > Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com
> <mailto:andreas.cadhal...@googlemail.com>>
> > ---
> >  libavcodec/wmavoice.c | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> > index cd5958c7bc..1bfad46b2e 100644
> > --- a/libavcodec/wmavoice.c
> > +++ b/libavcodec/wmavoice.c
> > @@ -1923,6 +1923,11 @@ static int wmavoice_decode_packet(AVCodecContext
> *ctx, void *data,
> >   * continuing to parse new superframes in the current
> packet. */
> >  if (s->sframe_cache_size > 0) {
> >  int cnt = get_bits_count(gb);
> > +if (cnt + s->spillover_nbits > avpkt->size * 8) {
> > +av_log(ctx, AV_LOG_WARNING, "Number of spillover
> bits %d larger than remaining packet size %d, truncating.\n",
> > +   s->spillover_nbits, avpkt->size * 8 - cnt);
> > +s->spillover_nbits = avpkt->size * 8 - cnt;
> > +}
> >
> >
> > This litters stderr uselessly. It doesn't help a user or developer
> accomplish
> > anything. Let me explain, because this is probably confusing.
> >
> > If a library integrator (e.g., the developer of gst-ffmpeg) is playing
> with
> > wmavoice and forgets to set block_align, the codec should error out.
> Ideally,
> > it does this saying block_align is zero, so the developer knows what to
> do.
> > But this is not strictly required and we typically don't do this.
> >
> > However, the above can only happen during corrupt streams (or fuzzing).
> > The user cannot fix this. So any detailed error is useless.
>
> I don't think it's useless, as it tells the user that there is a problem
> with
> the file. He might just test if ffmpeg can decode it to see if it is
> corrupt.
>
> > Therefore, we should not display any detailed error (or warning) message.
>
> So what would you do instead?


I'd just remove the message, but otherwise what you're doing (truncate
spillover_nbits) seems fine. You could also return AVERROR_INVALIDDATA, the
player will resume playback with a new packet. Make sure (in this
particular case) that you reset the internal cache before erroring out
(i.e. assign s->sframe_cache_size = 0).

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

Re: [FFmpeg-devel] [PATCH 2/2] wmavoice: prevent division by zero crash

[Ronald S. Bultje]

Hi,

On Sun, Jan 1, 2017 at 5:51 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 01.01.2017 23:23, Ronald S. Bultje wrote:
> > On Sun, Jan 1, 2017 at 5:19 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com <mailto:andreas.cadhal...@googlemail.com>>
> wrote:
> >
> > The problem was introduced by commit
> > 3deb4b54a24f8cddce463d9f5751b01efeb976af.
> >
> > Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com
> <mailto:andreas.cadhal...@googlemail.com>>
> > ---
> >  libavcodec/wmavoice.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> > index 1bfad46b2e..279b44dc12 100644
> > --- a/libavcodec/wmavoice.c
> > +++ b/libavcodec/wmavoice.c
> > @@ -1908,7 +1908,7 @@ static int wmavoice_decode_packet(AVCodecContext
> *ctx, void *data,
> >  /* size == ctx->block_align is used to indicate whether we are
> dealing with
> >   * a new packet or a packet of which we already read the packet
> header
> >   * previously. */
> > -if (!(size % ctx->block_align)) { // new packet header
> > +if (ctx->block_align && !(size % ctx->block_align)) { // new
> packet header
> >  if (!size) {
> >  s->spillover_nbits = 0;
> >  s->nb_superframes = 0;
> > --
> > 2.11.0
> >
> >
> > nak.
> >
> > The init routine should error out if block_align is zero.
> > The codec can not operate without block_align set.
>
> Fine for me. Patch doing that is attached.


LGTM.

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

Re: [FFmpeg-devel] [PATCH 1/2] wmavoice: truncate spillover_nbits if too large

[Ronald S. Bultje]

Hi,

On Sun, Jan 1, 2017 at 5:18 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> This fixes triggering the av_assert0(ret <= tmp.size).
>
> The problem was reintroduced by commit
> 7b27dd5c16de785297ce4de4b88afa0b6685f61d and originally fixed by
> 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e.
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/wmavoice.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> index cd5958c7bc..1bfad46b2e 100644
> --- a/libavcodec/wmavoice.c
> +++ b/libavcodec/wmavoice.c
> @@ -1923,6 +1923,11 @@ static int wmavoice_decode_packet(AVCodecContext
> *ctx, void *data,
>   * continuing to parse new superframes in the current packet. */
>  if (s->sframe_cache_size > 0) {
>  int cnt = get_bits_count(gb);
> +if (cnt + s->spillover_nbits > avpkt->size * 8) {
> +av_log(ctx, AV_LOG_WARNING, "Number of spillover bits %d
> larger than remaining packet size %d, truncating.\n",
> +   s->spillover_nbits, avpkt->size * 8 - cnt);
> +s->spillover_nbits = avpkt->size * 8 - cnt;
> +}


This litters stderr uselessly. It doesn't help a user or developer
accomplish anything. Let me explain, because this is probably confusing.

If a library integrator (e.g., the developer of gst-ffmpeg) is playing with
wmavoice and forgets to set block_align, the codec should error out.
Ideally, it does this saying block_align is zero, so the developer knows
what to do. But this is not strictly required and we typically don't do
this.

However, the above can only happen during corrupt streams (or fuzzing). The
user cannot fix this. So any detailed error is useless. Therefore, we
should not display any detailed error (or warning) message.

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

Re: [FFmpeg-devel] [PATCH 2/2] wmavoice: prevent division by zero crash

[Ronald S. Bultje]

Hi,

On Sun, Jan 1, 2017 at 5:19 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> The problem was introduced by commit
> 3deb4b54a24f8cddce463d9f5751b01efeb976af.
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavcodec/wmavoice.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> index 1bfad46b2e..279b44dc12 100644
> --- a/libavcodec/wmavoice.c
> +++ b/libavcodec/wmavoice.c
> @@ -1908,7 +1908,7 @@ static int wmavoice_decode_packet(AVCodecContext
> *ctx, void *data,
>  /* size == ctx->block_align is used to indicate whether we are
> dealing with
>   * a new packet or a packet of which we already read the packet header
>   * previously. */
> -if (!(size % ctx->block_align)) { // new packet header
> +if (ctx->block_align && !(size % ctx->block_align)) { // new packet
> header
>  if (!size) {
>  s->spillover_nbits = 0;
>  s->nb_superframes = 0;
> --
> 2.11.0


nak.

The init routine should error out if block_align is zero. The codec can not
operate without block_align set.

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

[FFmpeg-devel] [PATCH] wmavoice: remove unused or write-only variables.

[Ronald S. Bultje]

---
 libavcodec/wmavoice.c | 44 +---
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index cd5958c..4b283e3 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -104,26 +104,24 @@ static const struct frame_type_desc {
 uint8_t dbl_pulses;   ///< how many pulse vectors have pulse pairs
   ///< (rather than just one single pulse)
   ///< only if #fcb_type == #FCB_TYPE_EXC_PULSES
-uint16_t frame_size;  ///< the amount of bits that make up the block
-  ///< data (per frame)
 } frame_descs[17] = {
-{ 1, 0, ACB_TYPE_NONE,   FCB_TYPE_SILENCE,0,   0 },
-{ 2, 1, ACB_TYPE_NONE,   FCB_TYPE_HARDCODED,  0,  28 },
-{ 2, 1, ACB_TYPE_ASYMMETRIC, FCB_TYPE_AW_PULSES,  0,  46 },
-{ 2, 1, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 2,  80 },
-{ 2, 1, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 5, 104 },
-{ 4, 2, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 0, 108 },
-{ 4, 2, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 2, 132 },
-{ 4, 2, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 5, 168 },
-{ 2, 1, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 0,  64 },
-{ 2, 1, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 2,  80 },
-{ 2, 1, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 5, 104 },
-{ 4, 2, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 0, 108 },
-{ 4, 2, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 2, 132 },
-{ 4, 2, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 5, 168 },
-{ 8, 3, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 0, 176 },
-{ 8, 3, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 2, 208 },
-{ 8, 3, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 5, 256 }
+{ 1, 0, ACB_TYPE_NONE,   FCB_TYPE_SILENCE,0 },
+{ 2, 1, ACB_TYPE_NONE,   FCB_TYPE_HARDCODED,  0 },
+{ 2, 1, ACB_TYPE_ASYMMETRIC, FCB_TYPE_AW_PULSES,  0 },
+{ 2, 1, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 2 },
+{ 2, 1, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 5 },
+{ 4, 2, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 0 },
+{ 4, 2, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 2 },
+{ 4, 2, ACB_TYPE_ASYMMETRIC, FCB_TYPE_EXC_PULSES, 5 },
+{ 2, 1, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 0 },
+{ 2, 1, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 2 },
+{ 2, 1, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 5 },
+{ 4, 2, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 0 },
+{ 4, 2, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 2 },
+{ 4, 2, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 5 },
+{ 8, 3, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 0 },
+{ 8, 3, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 2 },
+{ 8, 3, ACB_TYPE_HAMMING,FCB_TYPE_EXC_PULSES, 5 }
 };
 
 /**
@@ -160,10 +158,6 @@ typedef struct WMAVoiceContext {
 int lsp_q_mode;   ///< defines quantizer defaults [0, 1]
 int lsp_def_mode; ///< defines different sets of LSP defaults
   ///< [0, 1]
-int frame_lsp_bitsize;///< size (in bits) of LSPs, when encoded
-  ///< per-frame (independent coding)
-int sframe_lsp_bitsize;   ///< size (in bits) of LSPs, when encoded
-  ///< per superframe (residual coding)
 
 int min_pitch_val;///< base value for pitch parsing code
 int max_pitch_val;///< max value + 1 for pitch parsing
@@ -418,12 +412,8 @@ static av_cold int wmavoice_decode_init(AVCodecContext 
*ctx)
 lsp16_flag   =flags & 0x1000;
 if (lsp16_flag) {
 s->lsps   = 16;
-s->frame_lsp_bitsize  = 34;
-s->sframe_lsp_bitsize = 60;
 } else {
 s->lsps   = 10;
-s->frame_lsp_bitsize  = 24;
-s->sframe_lsp_bitsize = 48;
 }
 for (n = 0; n < s->lsps; n++)
 s->prev_lsps[n] = M_PI * (n + 1.0) / (s->lsps + 1.0);
-- 
2.8.1

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

Re: [FFmpeg-devel] [PATCH] checkasm/vp9: benchmark all sub-IDCTs (but not WHT or ADST).

[Ronald S. Bultje]

Hi,

On Tue, Dec 20, 2016 at 7:10 PM, Ronald S. Bultje <rsbul...@gmail.com>
wrote:

> Hi,
>
> On Mon, Dec 5, 2016 at 4:15 PM, Ronald S. Bultje <rsbul...@gmail.com>
> wrote:
>
>> ---
>>  tests/checkasm/vp9dsp.c | 22 ++
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/checkasm/vp9dsp.c b/tests/checkasm/vp9dsp.c
>> index 441041c..f32b97c 100644
>> --- a/tests/checkasm/vp9dsp.c
>> +++ b/tests/checkasm/vp9dsp.c
>> @@ -331,15 +331,20 @@ static void check_itxfm(void)
>>  int n_txtps = tx < TX_32X32 ? N_TXFM_TYPES : 1;
>>
>>  for (txtp = 0; txtp < n_txtps; txtp++) {
>> -if (check_func(dsp.itxfm_add[tx][txtp],
>> "vp9_inv_%s_%dx%d_add_%d",
>> -   tx == 4 ? "wht_wht" : txtp_types[txtp],
>> sz, sz,
>> -   bit_depth)) {
>> -randomize_buffers();
>> -ftx(coef, tx, txtp, sz, bit_depth);
>> -
>> -for (sub = (txtp == 0) ? 1 : 2; sub <= sz; sub <<=
>> 1) {
>> +// skip testing sub-IDCTs for WHT or ADST since they
>> don't
>> +// implement it in any of the SIMD functions. If they do,
>> +// consider changing this to ensure we have complete test
>> +// coverage
>> +for (sub = (txtp == 0 && tx < 4) ? 1 : sz; sub <= sz;
>> sub <<= 1) {
>> +if (check_func(dsp.itxfm_add[tx][txtp],
>> +   "vp9_inv_%s_%dx%d_sub%d_add_%d",
>> +   tx == 4 ? "wht_wht" :
>> txtp_types[txtp],
>> +   sz, sz, sub, bit_depth)) {
>>  int eob;
>>
>> +randomize_buffers();
>> +ftx(coef, tx, txtp, sz, bit_depth);
>> +
>>  if (sub < sz) {
>>  eob = copy_subcoefs(subcoef0, coef, tx, txtp,
>>  sz, sub, bit_depth);
>> @@ -357,8 +362,9 @@ static void check_itxfm(void)
>>  !iszero(subcoef0, sz * sz * SIZEOF_COEF) ||
>>  !iszero(subcoef1, sz * sz * SIZEOF_COEF))
>>  fail();
>> +
>> +bench_new(dst, sz * SIZEOF_PIXEL, coef, eob);
>>  }
>> -bench_new(dst, sz * SIZEOF_PIXEL, coef, sz * sz);
>>  }
>>  }
>>  }
>> --
>> 2.8.1
>
>
> Ping. I will apply this with some other outstanding patches if there's no
> comments.
>

Pushed.

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

Re: [FFmpeg-devel] [PATCH] wmavoice: protect against zero-energy in adaptive gain control.

[Ronald S. Bultje]

Hi,

On Wed, Dec 21, 2016 at 2:50 AM, Paul B Mahol <one...@gmail.com> wrote:

> On 12/21/16, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> > Otherwise the scale factor becomes NaN, resulting in corrupt output.
> > Fixes #5426.
> > ---
> >  libavcodec/wmavoice.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
> > index 90dfe20..cd5958c 100644
> > --- a/libavcodec/wmavoice.c
> > +++ b/libavcodec/wmavoice.c
> > @@ -512,7 +512,8 @@ static void adaptive_gain_control(float *out, const
> > float *in,
> >  speech_energy += fabsf(speech_synth[i]);
> >  postfilter_energy += fabsf(in[i]);
> >  }
> > -gain_scale_factor = (1.0 - alpha) * speech_energy /
> postfilter_energy;
> > +gain_scale_factor = postfilter_energy == 0.0 ? 0.0 :
> > +(1.0 - alpha) * speech_energy /
> postfilter_energy;
> >
> >  for (i = 0; i < size; i++) {
> >  mem = alpha * mem + gain_scale_factor;
> > --
> > 2.8.1
> >
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
>
> lgtm
>

Whole set pushed.

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

Re: [FFmpeg-devel] [libav-devel] [PATCH] x86inc: Avoid using eax/rax for storing the stack pointer

[Ronald S. Bultje]

Hi,

On Mon, Dec 26, 2016 at 4:53 AM, Henrik Gramner <hen...@gramner.com> wrote:

> On Mon, Dec 26, 2016 at 2:32 AM, Ronald S. Bultje <rsbul...@gmail.com>
> wrote:
> > I know I'm terribly nitpicking here for the limited scope of the comment,
> > but this only matters for functions that have a return value. Do you
> think
> > it makes sense to allow functions to opt out of this requirement if they
> > explicitly state to not have a return value?
>
> An opt-out would only be relevant on 64-bit Windows when the following
> criteria are true for a function:
>
> * Reserves exactly 6 registers
> * Reserves stack space with the original stack pointer stored in a
> register (as opposed to the stack)
> * Requires >16 byte stack alignment (e.g. spilling ymm registers to the
> stack)
> * Does not have a return value
>
> If and only if all of those are true this would result in one register
> being unnecessarily saved (the cost of which would likely be hidden by
> OoE). On other systems than WIN64 or if any of the conditions above is
> false an opt-out doesn't make any sense.
>
> Considering how rare that corner case is in combination with how
> fairly insignificant the downside is I'm not sure it makes that much
> sense to complicate the x86inc API further with an opt-out just for
> that specific scenario.
>

 Hm, OK, I think it affects unix64/x86-32 also when using 32-byte
alignment. We do use the stack pointer then. But let's ignore that for a
second, I think it's besides the point.

I think my hesitation comes from how I view x86inc.asm. There's two ways to
see it:
- it's a universal tool, like a compiler, to assist writing assembly
(combined with yasm/nasm as actual assembler);
or
- it's a local tool for ffmpeg/libav/x26[5], like libavutil/attributes.h,
to assist writing assembly.

If x86inc.asm were like a compiler, every micro-optimization, no matter the
benefit, would be important. If it were a local tool, we indeed wouldn't
care because ffmpeg spends most runtime for important use cases in other
areas. (There's obviously a grayscale in this black/white range that I'm
drawing out.) So having said that, patch is OK. If someone would later come
in to add something to take return value type (void vs. non-void) into
account, I would still find that helpful. :)

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

Re: [FFmpeg-devel] [libav-devel] [PATCH] x86inc: Avoid using eax/rax for storing the stack pointer

[Ronald S. Bultje]

Hi,

On Sun, Dec 25, 2016 at 2:24 PM, Henrik Gramner  wrote:

> When allocating stack space with an alignment requirement that is larger
> than the current stack alignment we need to store a copy of the original
> stack pointer in order to be able to restore it later.
>
> If we chose to use another register for this purpose we should not pick
> eax/rax since it can be overwritten as a return value.
> ---
>  libavutil/x86/x86inc.asm | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/libavutil/x86/x86inc.asm b/libavutil/x86/x86inc.asm
> index b2e9c60..128ddc1 100644
> --- a/libavutil/x86/x86inc.asm
> +++ b/libavutil/x86/x86inc.asm
> @@ -385,7 +385,14 @@ DECLARE_REG_TMP_SIZE 0,1,2,3,4,5,6,7,8,9,10,11,12,
> 13,14
>  %ifnum %1
>  %if %1 != 0 && required_stack_alignment > STACK_ALIGNMENT
>  %if %1 > 0
> +; Reserve an additional register for storing the original
> stack pointer, but avoid using
> +; eax/rax for this purpose since it can potentially get
> overwritten as a return value.
>  %assign regs_used (regs_used + 1)
> +%if ARCH_X86_64 && regs_used == 7
> +%assign regs_used 8
> +%elif ARCH_X86_64 == 0 && regs_used == 1
> +%assign regs_used 2
> +%endif
>  %endif
>  %if ARCH_X86_64 && regs_used < 5 + UNIX64 * 3
>  ; Ensure that we don't clobber any registers containing
> arguments. For UNIX64 we also preserve r6 (rax)
> --
> 2.7.4


I know I'm terribly nitpicking here for the limited scope of the comment,
but this only matters for functions that have a return value. Do you think
it makes sense to allow functions to opt out of this requirement if they
explicitly state to not have a return value?

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

Re: [FFmpeg-devel] [PATCH] avcodec/magicyuv: add SIMD for median of 10bits

[Ronald S. Bultje]

Hi,

On Sat, Dec 24, 2016 at 9:29 AM, Paul B Mahol <one...@gmail.com> wrote:

> On 12/24/16, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> > Hi,
> >
> > On Sat, Dec 24, 2016 at 6:09 AM, Paul B Mahol <one...@gmail.com> wrote:
> >
> >> On 12/24/16, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> >> > Hi,
> >> >
> >> > On Fri, Dec 23, 2016 at 6:18 PM, James Almer <jamr...@gmail.com>
> wrote:
> >> >
> >> >> On 12/23/2016 8:00 PM, Ronald S. Bultje wrote:
> >> >> > Hi,
> >> >> >
> >> >> > On Fri, Dec 23, 2016 at 12:32 PM, Paul B Mahol <one...@gmail.com>
> >> wrote:
> >> >> >
> >> >> >> diff --git a/libavcodec/lossless_videodsp.h
> b/libavcodec/lossless_
> >> >> >> videodsp.h
> >> >> >>
> >> >> > [..]
> >> >> >
> >> >> >> @@ -32,6 +32,7 @@ typedef struct LLVidDSPContext {
> >> >> >>
> >> >> > [..]
> >> >> >
> >> >> >> +void (*add_magy_median_pred_int16)(uint16_t *dst, const
> >> uint16_t
> >> >> >> *top, const uint16_t *diff, unsigned mask, int w, int *left, int
> >> >> *left_top);
> >> >> >>
> >> >> >
> >> >> > That seems wrong. Why would you add a magicuv-specific function to
> >> >> > losslessdsp-context which is intended for functions shared between
> >> many
> >> >> > (not just one) lossless codecs? You probably want a new dsp for
> >> magicyuv
> >> >> > specifically.
> >> >> >
> >> >> > I know this is tedious, but we're very specifically trying to
> prevent
> >> >> > dsputil from ever happening again.
> >> >> >
> >> >> > Ronald
> >> >>
> >> >> Some functions in this dsp are used only by huffyuv. Only one is used
> >> >> by
> >> >> both huffyuv and magicyuv.
> >> >> To properly apply what you mention, it would need to be split in two,
> >> >> huffyuvdsp and lldsp, then this new function added to a new dsp
> called
> >> >> magicyuvdsp.
> >> >
> >> >
> >> > That would be even better, yes.
> >>
> >> What about yasm code?
> >>
> >> I wanted that to be commented.
> >
> >
> > It's like dithering, it uses the immediately adjacent pixel in the next
> > loop iteration, can you really simd this effectively?
>
> Apparently, and someone is making money from it.


The parallelizable portion of it is the top-topleft, and you seem to do
that already. Other than that, I don't see much to be done. You can
probably use some mmxext instructions like pshufw to make life easier, but
I think you'll always be limited by the inherent limitation.

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

Re: [FFmpeg-devel] [PATCH] avcodec/magicyuv: add SIMD for median of 10bits

[Ronald S. Bultje]

Hi,

On Sat, Dec 24, 2016 at 6:09 AM, Paul B Mahol <one...@gmail.com> wrote:

> On 12/24/16, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> > Hi,
> >
> > On Fri, Dec 23, 2016 at 6:18 PM, James Almer <jamr...@gmail.com> wrote:
> >
> >> On 12/23/2016 8:00 PM, Ronald S. Bultje wrote:
> >> > Hi,
> >> >
> >> > On Fri, Dec 23, 2016 at 12:32 PM, Paul B Mahol <one...@gmail.com>
> wrote:
> >> >
> >> >> diff --git a/libavcodec/lossless_videodsp.h b/libavcodec/lossless_
> >> >> videodsp.h
> >> >>
> >> > [..]
> >> >
> >> >> @@ -32,6 +32,7 @@ typedef struct LLVidDSPContext {
> >> >>
> >> > [..]
> >> >
> >> >> +void (*add_magy_median_pred_int16)(uint16_t *dst, const
> uint16_t
> >> >> *top, const uint16_t *diff, unsigned mask, int w, int *left, int
> >> *left_top);
> >> >>
> >> >
> >> > That seems wrong. Why would you add a magicuv-specific function to
> >> > losslessdsp-context which is intended for functions shared between
> many
> >> > (not just one) lossless codecs? You probably want a new dsp for
> magicyuv
> >> > specifically.
> >> >
> >> > I know this is tedious, but we're very specifically trying to prevent
> >> > dsputil from ever happening again.
> >> >
> >> > Ronald
> >>
> >> Some functions in this dsp are used only by huffyuv. Only one is used by
> >> both huffyuv and magicyuv.
> >> To properly apply what you mention, it would need to be split in two,
> >> huffyuvdsp and lldsp, then this new function added to a new dsp called
> >> magicyuvdsp.
> >
> >
> > That would be even better, yes.
>
> What about yasm code?
>
> I wanted that to be commented.


It's like dithering, it uses the immediately adjacent pixel in the next
loop iteration, can you really simd this effectively?

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

Re: [FFmpeg-devel] [PATCH] avcodec/magicyuv: add SIMD for median of 10bits

[Ronald S. Bultje]

Hi,

On Fri, Dec 23, 2016 at 6:18 PM, James Almer <jamr...@gmail.com> wrote:

> On 12/23/2016 8:00 PM, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Fri, Dec 23, 2016 at 12:32 PM, Paul B Mahol <one...@gmail.com> wrote:
> >
> >> diff --git a/libavcodec/lossless_videodsp.h b/libavcodec/lossless_
> >> videodsp.h
> >>
> > [..]
> >
> >> @@ -32,6 +32,7 @@ typedef struct LLVidDSPContext {
> >>
> > [..]
> >
> >> +void (*add_magy_median_pred_int16)(uint16_t *dst, const uint16_t
> >> *top, const uint16_t *diff, unsigned mask, int w, int *left, int
> *left_top);
> >>
> >
> > That seems wrong. Why would you add a magicuv-specific function to
> > losslessdsp-context which is intended for functions shared between many
> > (not just one) lossless codecs? You probably want a new dsp for magicyuv
> > specifically.
> >
> > I know this is tedious, but we're very specifically trying to prevent
> > dsputil from ever happening again.
> >
> > Ronald
>
> Some functions in this dsp are used only by huffyuv. Only one is used by
> both huffyuv and magicyuv.
> To properly apply what you mention, it would need to be split in two,
> huffyuvdsp and lldsp, then this new function added to a new dsp called
> magicyuvdsp.


That would be even better, yes.

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

Re: [FFmpeg-devel] [PATCH] avcodec/magicyuv: add SIMD for median of 10bits

[Ronald S. Bultje]

Hi,

On Fri, Dec 23, 2016 at 12:32 PM, Paul B Mahol  wrote:

> diff --git a/libavcodec/lossless_videodsp.h b/libavcodec/lossless_
> videodsp.h
>
[..]

> @@ -32,6 +32,7 @@ typedef struct LLVidDSPContext {
>
[..]

> +void (*add_magy_median_pred_int16)(uint16_t *dst, const uint16_t
> *top, const uint16_t *diff, unsigned mask, int w, int *left, int *left_top);
>

That seems wrong. Why would you add a magicuv-specific function to
losslessdsp-context which is intended for functions shared between many
(not just one) lossless codecs? You probably want a new dsp for magicyuv
specifically.

I know this is tedious, but we're very specifically trying to prevent
dsputil from ever happening again.

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

Re: [FFmpeg-devel] [PATCH 2/4] wmavoice: disable bitstream checking.

[Ronald S. Bultje]

Hi,

On Tue, Dec 20, 2016 at 7:59 PM, Michael Niedermayer <mich...@niedermayer.cc
> wrote:

> On Tue, Dec 20, 2016 at 07:51:05PM -0500, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Tue, Dec 20, 2016 at 7:44 PM, Michael Niedermayer
> <mich...@niedermayer.cc
> > > wrote:
> >
> > > On Tue, Dec 20, 2016 at 05:23:06PM -0500, Ronald S. Bultje wrote:
> > > > The checked bitstream reader does that already. To allow parsing of
> > > > superframes split over a packet boundary, we always decode the last
> > > > superframe in each packet at the start of the next packet, even if
> > > > theoretically we could have decoded it. The last superframe in the
> > > > last packet is decoded using AV_CODEC_CAP_DELAY.
> > > > ---
> > > >  libavcodec/wmavoice.c | 144 ++
> > > 
> > > >  1 file changed, 29 insertions(+), 115 deletions(-)
> > >
> > > this causes
> > >
> > > tickets/1708/mss2_speech.wmv
> > >
> > > to show
> > > [wmavoice @ 0x7f7ec40726c0] WMAPro-in-WMAVoice is not implemented.
> Update
> > > your FFmpeg version to the newest one from Git. If the problem still
> > > occurs, it means that your file has a feature which has not been
> > > implemented.
> > > [wmavoice @ 0x7f7ec40726c0] If you want to help, upload a sample of
> this
> > > file to ftp://upload.ffmpeg.org/incoming/ and contact the ffmpeg-devel
> > > mailing list. (ffmpeg-devel@ffmpeg.org)
> > > [wmavoice @ 0x7f7ec40726c0] WMAPro-in-WMAVoice is not implemented.
> Update
> > > your FFmpeg version to the newest one from Git. If the problem still
> > > occurs, it means that your file has a feature which has not been
> > > implemented.
> > > [wmavoice @ 0x7f7ec40726c0] If you want to help, upload a sample of
> this
> > > file to ftp://upload.ffmpeg.org/incoming/ and contact the ffmpeg-devel
> > > mailing list. (ffmpeg-devel@ffmpeg.org)
> >
> >
> > See http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2016-
> December/204656.html
> >
> > To confirm that's the issue, did the md5 change? If not, then it's a
> great
> > sample file and you should file a ticket so I finally have a valid sample
> > to implement wmapro-in-voice decoding (I still don't have any).
>
> The framecrc from before and after your patchset changes
> file should be here:
>
> http://samples.mplayerhq.hu/V-codecs/MSS2/mss2_speech.wmv


Aha, tested. Before the patch, we ignore the nb_superframes value. This can
lead to us reading more superframes per packet than are actually written in
the packet data. Even if the packet only has N bytes of useful data, it is
still padded to block_align bytes because ASF is stupid that way. So, for
the affected packets, after decoding N superframes correctly (as indicated
in the header value) and exhausting the valid data in some of these packets
(not necessarily just the final ones), we start reading the padding garbage
(and usually fail to do so), leading to some of the wmapro-in-wmavoice
warnings in some cases, and garbage decoding + timestamp warnings (because
last_ts + last_duration > this_ts after correct decoding resumes) in others.

After the patch, we correctly skip this padding garbage. This causes the
timestamp warnings to go away, and 4 out of 6 wmapro-in-wmavoice warnings
to go away also. Audibly, the result sounds identical; what's different is
the length of some of the silence sections. E.g. the start is 2/100th sec
silence shorter after compared to before the patch. Given my explanation
above, this is probably an improvement.

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

Re: [FFmpeg-devel] [PATCH 2/4] wmavoice: disable bitstream checking.

[Ronald S. Bultje]

Hi,

On Tue, Dec 20, 2016 at 7:44 PM, Michael Niedermayer <mich...@niedermayer.cc
> wrote:

> On Tue, Dec 20, 2016 at 05:23:06PM -0500, Ronald S. Bultje wrote:
> > The checked bitstream reader does that already. To allow parsing of
> > superframes split over a packet boundary, we always decode the last
> > superframe in each packet at the start of the next packet, even if
> > theoretically we could have decoded it. The last superframe in the
> > last packet is decoded using AV_CODEC_CAP_DELAY.
> > ---
> >  libavcodec/wmavoice.c | 144 ++
> 
> >  1 file changed, 29 insertions(+), 115 deletions(-)
>
> this causes
>
> tickets/1708/mss2_speech.wmv
>
> to show
> [wmavoice @ 0x7f7ec40726c0] WMAPro-in-WMAVoice is not implemented. Update
> your FFmpeg version to the newest one from Git. If the problem still
> occurs, it means that your file has a feature which has not been
> implemented.
> [wmavoice @ 0x7f7ec40726c0] If you want to help, upload a sample of this
> file to ftp://upload.ffmpeg.org/incoming/ and contact the ffmpeg-devel
> mailing list. (ffmpeg-devel@ffmpeg.org)
> [wmavoice @ 0x7f7ec40726c0] WMAPro-in-WMAVoice is not implemented. Update
> your FFmpeg version to the newest one from Git. If the problem still
> occurs, it means that your file has a feature which has not been
> implemented.
> [wmavoice @ 0x7f7ec40726c0] If you want to help, upload a sample of this
> file to ftp://upload.ffmpeg.org/incoming/ and contact the ffmpeg-devel
> mailing list. (ffmpeg-devel@ffmpeg.org)


See http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2016-December/204656.html

To confirm that's the issue, did the md5 change? If not, then it's a great
sample file and you should file a ticket so I finally have a valid sample
to implement wmapro-in-voice decoding (I still don't have any).

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

Re: [FFmpeg-devel] [PATCH] checkasm/vp9: benchmark all sub-IDCTs (but not WHT or ADST).

[Ronald S. Bultje]

Hi,

On Mon, Dec 5, 2016 at 4:15 PM, Ronald S. Bultje <rsbul...@gmail.com> wrote:

> ---
>  tests/checkasm/vp9dsp.c | 22 ++
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/tests/checkasm/vp9dsp.c b/tests/checkasm/vp9dsp.c
> index 441041c..f32b97c 100644
> --- a/tests/checkasm/vp9dsp.c
> +++ b/tests/checkasm/vp9dsp.c
> @@ -331,15 +331,20 @@ static void check_itxfm(void)
>  int n_txtps = tx < TX_32X32 ? N_TXFM_TYPES : 1;
>
>  for (txtp = 0; txtp < n_txtps; txtp++) {
> -if (check_func(dsp.itxfm_add[tx][txtp],
> "vp9_inv_%s_%dx%d_add_%d",
> -   tx == 4 ? "wht_wht" : txtp_types[txtp],
> sz, sz,
> -   bit_depth)) {
> -randomize_buffers();
> -ftx(coef, tx, txtp, sz, bit_depth);
> -
> -for (sub = (txtp == 0) ? 1 : 2; sub <= sz; sub <<= 1)
> {
> +// skip testing sub-IDCTs for WHT or ADST since they don't
> +// implement it in any of the SIMD functions. If they do,
> +// consider changing this to ensure we have complete test
> +// coverage
> +for (sub = (txtp == 0 && tx < 4) ? 1 : sz; sub <= sz; sub
> <<= 1) {
> +if (check_func(dsp.itxfm_add[tx][txtp],
> +   "vp9_inv_%s_%dx%d_sub%d_add_%d",
> +   tx == 4 ? "wht_wht" : txtp_types[txtp],
> +   sz, sz, sub, bit_depth)) {
>  int eob;
>
> +randomize_buffers();
> +ftx(coef, tx, txtp, sz, bit_depth);
> +
>  if (sub < sz) {
>  eob = copy_subcoefs(subcoef0, coef, tx, txtp,
>  sz, sub, bit_depth);
> @@ -357,8 +362,9 @@ static void check_itxfm(void)
>  !iszero(subcoef0, sz * sz * SIZEOF_COEF) ||
>  !iszero(subcoef1, sz * sz * SIZEOF_COEF))
>  fail();
> +
> +bench_new(dst, sz * SIZEOF_PIXEL, coef, eob);
>  }
> -bench_new(dst, sz * SIZEOF_PIXEL, coef, sz * sz);
>  }
>  }
>  }
> --
> 2.8.1


Ping. I will apply this with some other outstanding patches if there's no
comments.

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

[FFmpeg-devel] [PATCH] wmavoice: protect against zero-energy in adaptive gain control.

[Ronald S. Bultje]

Otherwise the scale factor becomes NaN, resulting in corrupt output.
Fixes #5426.
---
 libavcodec/wmavoice.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index 90dfe20..cd5958c 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -512,7 +512,8 @@ static void adaptive_gain_control(float *out, const float 
*in,
 speech_energy += fabsf(speech_synth[i]);
 postfilter_energy += fabsf(in[i]);
 }
-gain_scale_factor = (1.0 - alpha) * speech_energy / postfilter_energy;
+gain_scale_factor = postfilter_energy == 0.0 ? 0.0 :
+(1.0 - alpha) * speech_energy / postfilter_energy;
 
 for (i = 0; i < size; i++) {
 mem = alpha * mem + gain_scale_factor;
-- 
2.8.1

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

[FFmpeg-devel] [PATCH 4/4] wmavoice: move overflow handling to common code.

[Ronald S. Bultje]

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

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index f31c9d2..90dfe20 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1800,6 +1800,11 @@ static int synth_superframe(AVCodecContext *ctx, AVFrame 
*frame,
 skip_bits(gb, 10 * (res + 1));
 }
 
+if (get_bits_left(gb) < 0) {
+wmavoice_flush(ctx);
+return AVERROR_INVALIDDATA;
+}
+
 *got_frame_ptr = 1;
 
 /* Update history */
@@ -1925,12 +1930,6 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 cnt += s->spillover_nbits;
 s->skip_bits_next = cnt & 7;
 res = cnt >> 3;
-if (res > avpkt->size) {
-av_log(ctx, AV_LOG_ERROR,
-   "Trying to skip %d bytes in packet of size %d\n",
-   res, avpkt->size);
-return AVERROR_INVALIDDATA;
-}
 return res;
 } else
 skip_bits_long (gb, s->spillover_nbits - cnt +
@@ -1955,12 +1954,6 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 int cnt = get_bits_count(gb);
 s->skip_bits_next = cnt & 7;
 res = cnt >> 3;
-if (res > avpkt->size) {
-av_log(ctx, AV_LOG_ERROR,
-   "Trying to skip %d bytes in packet of size %d\n",
-   res, avpkt->size);
-return AVERROR_INVALIDDATA;
-}
 return res;
 }
 } else if ((s->sframe_cache_size = pos) > 0) {
-- 
2.8.1

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

[FFmpeg-devel] [PATCH 2/4] wmavoice: disable bitstream checking.

[Ronald S. Bultje]

The checked bitstream reader does that already. To allow parsing of
superframes split over a packet boundary, we always decode the last
superframe in each packet at the start of the next packet, even if
theoretically we could have decoded it. The last superframe in the
last packet is decoded using AV_CODEC_CAP_DELAY.
---
 libavcodec/wmavoice.c | 144 ++
 1 file changed, 29 insertions(+), 115 deletions(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index 4b3ab43..ae100fb 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -251,6 +251,7 @@ typedef struct WMAVoiceContext {
 
 int frame_cntr;   ///< current frame index [0 - 0xFFFE]; is
   ///< only used for comfort noise in #pRNG()
+int nb_superframes;   ///< number of superframes in current packet
 float gain_pred_err[6];   ///< cache for gain prediction
 float excitation_history[MAX_SIGNAL_HISTORY];
   ///< cache of the signal of previous
@@ -875,7 +876,6 @@ static void dequant_lsps(double *lsps, int num,
 /**
  * @name LSP dequantization routines
  * LSP dequantization routines, for 10/16LSPs and independent/residual coding.
- * @note we assume enough bits are available, caller should check.
  * lsp10i() consumes 24 bits; lsp10r() consumes an additional 24 bits;
  * lsp16i() consumes 34 bits; lsp16r() consumes an additional 26 bits.
  * @{
@@ -1419,7 +1419,6 @@ static void synth_block_fcb_acb(WMAVoiceContext *s, 
GetBitContext *gb,
 
 /**
  * Parse data in a single block.
- * @note we assume enough bits are available, caller should check.
  *
  * @param s WMA Voice decoding context private data
  * @param gb bit I/O context
@@ -1463,7 +1462,6 @@ static void synth_block(WMAVoiceContext *s, GetBitContext 
*gb,
 
 /**
  * Synthesize output samples for a single frame.
- * @note we assume enough bits are available, caller should check.
  *
  * @param ctx WMA Voice decoder context
  * @param gb bit I/O context (s->gb or one for cross-packet superframes)
@@ -1682,83 +1680,6 @@ static void stabilize_lsps(double *lsps, int num)
 }
 
 /**
- * Test if there's enough bits to read 1 superframe.
- *
- * @param orig_gb bit I/O context used for reading. This function
- *does not modify the state of the bitreader; it
- *only uses it to copy the current stream position
- * @param s WMA Voice decoding context private data
- * @return < 0 on error, 1 on not enough bits or 0 if OK.
- */
-static int check_bits_for_superframe(GetBitContext *orig_gb,
- WMAVoiceContext *s)
-{
-GetBitContext s_gb, *gb = _gb;
-int n, need_bits, bd_idx;
-const struct frame_type_desc *frame_desc;
-
-/* initialize a copy */
-init_get_bits(gb, orig_gb->buffer, orig_gb->size_in_bits);
-skip_bits_long(gb, get_bits_count(orig_gb));
-av_assert1(get_bits_left(gb) == get_bits_left(orig_gb));
-
-/* superframe header */
-if (get_bits_left(gb) < 14)
-return 1;
-if (!get_bits1(gb))
-return AVERROR(ENOSYS);   // WMAPro-in-WMAVoice superframe
-if (get_bits1(gb)) skip_bits(gb, 12); // number of  samples in superframe
-if (s->has_residual_lsps) {   // residual LSPs (for all frames)
-if (get_bits_left(gb) < s->sframe_lsp_bitsize)
-return 1;
-skip_bits_long(gb, s->sframe_lsp_bitsize);
-}
-
-/* frames */
-for (n = 0; n < MAX_FRAMES; n++) {
-int aw_idx_is_ext = 0;
-
-if (!s->has_residual_lsps) { // independent LSPs (per-frame)
-   if (get_bits_left(gb) < s->frame_lsp_bitsize) return 1;
-   skip_bits_long(gb, s->frame_lsp_bitsize);
-}
-bd_idx = s->vbm_tree[get_vlc2(gb, frame_type_vlc.table, 6, 3)];
-if (bd_idx < 0)
-return AVERROR_INVALIDDATA; // invalid frame type VLC code
-frame_desc = _descs[bd_idx];
-if (frame_desc->acb_type == ACB_TYPE_ASYMMETRIC) {
-if (get_bits_left(gb) < s->pitch_nbits)
-return 1;
-skip_bits_long(gb, s->pitch_nbits);
-}
-if (frame_desc->fcb_type == FCB_TYPE_SILENCE) {
-skip_bits(gb, 8);
-} else if (frame_desc->fcb_type == FCB_TYPE_AW_PULSES) {
-int tmp = get_bits(gb, 6);
-if (tmp >= 0x36) {
-skip_bits(gb, 2);
-aw_idx_is_ext = 1;
-}
-}
-
-/* blocks */
-if (frame_desc->acb_type == ACB_TYPE_HAMMING) {
-need_bits = s->block_pitch_nbits +
-(frame_desc->n_blocks - 1) * s->block_delta_pitch_nbits;
-} else if (frame_desc->fcb_type == FCB_TYPE_AW_PULSES) {
-need_bits = 2 * !aw_idx_is_ext;
-} else
-need_bits = 0;
-need_bits += frame_desc->frame_size;
-if (get_bits_left(gb) < need_bits)
-return 1;
-   

[FFmpeg-devel] [PATCH 1/4] wmavoice: move wmavoice_flush() up.

[Ronald S. Bultje]

---
 libavcodec/wmavoice.c | 56 +--
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index ceac61f..4b3ab43 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -337,6 +337,34 @@ static av_cold void wmavoice_init_static_data(AVCodec 
*codec)
 bits, 1, 1, codes, 2, 2, 132);
 }
 
+static av_cold void wmavoice_flush(AVCodecContext *ctx)
+{
+WMAVoiceContext *s = ctx->priv_data;
+int n;
+
+s->postfilter_agc= 0;
+s->sframe_cache_size = 0;
+s->skip_bits_next= 0;
+for (n = 0; n < s->lsps; n++)
+s->prev_lsps[n] = M_PI * (n + 1.0) / (s->lsps + 1.0);
+memset(s->excitation_history, 0,
+   sizeof(*s->excitation_history) * MAX_SIGNAL_HISTORY);
+memset(s->synth_history,  0,
+   sizeof(*s->synth_history)  * MAX_LSPS);
+memset(s->gain_pred_err,  0,
+   sizeof(s->gain_pred_err));
+
+if (s->do_apf) {
+memset(>synth_filter_out_buf[MAX_LSPS_ALIGN16 - s->lsps], 0,
+   sizeof(*s->synth_filter_out_buf) * s->lsps);
+memset(s->dcf_mem,  0,
+   sizeof(*s->dcf_mem)  * 2);
+memset(s->zero_exc_pf,  0,
+   sizeof(*s->zero_exc_pf)  * s->history_nsamples);
+memset(s->denoise_filter_cache, 0, sizeof(s->denoise_filter_cache));
+}
+}
+
 /**
  * Set up decoder with parameters from demuxer (extradata etc.).
  */
@@ -2046,34 +2074,6 @@ static av_cold int wmavoice_decode_end(AVCodecContext 
*ctx)
 return 0;
 }
 
-static av_cold void wmavoice_flush(AVCodecContext *ctx)
-{
-WMAVoiceContext *s = ctx->priv_data;
-int n;
-
-s->postfilter_agc= 0;
-s->sframe_cache_size = 0;
-s->skip_bits_next= 0;
-for (n = 0; n < s->lsps; n++)
-s->prev_lsps[n] = M_PI * (n + 1.0) / (s->lsps + 1.0);
-memset(s->excitation_history, 0,
-   sizeof(*s->excitation_history) * MAX_SIGNAL_HISTORY);
-memset(s->synth_history,  0,
-   sizeof(*s->synth_history)  * MAX_LSPS);
-memset(s->gain_pred_err,  0,
-   sizeof(s->gain_pred_err));
-
-if (s->do_apf) {
-memset(>synth_filter_out_buf[MAX_LSPS_ALIGN16 - s->lsps], 0,
-   sizeof(*s->synth_filter_out_buf) * s->lsps);
-memset(s->dcf_mem,  0,
-   sizeof(*s->dcf_mem)  * 2);
-memset(s->zero_exc_pf,  0,
-   sizeof(*s->zero_exc_pf)  * s->history_nsamples);
-memset(s->denoise_filter_cache, 0, sizeof(s->denoise_filter_cache));
-}
-}
-
 AVCodec ff_wmavoice_decoder = {
 .name = "wmavoice",
 .long_name= NULL_IF_CONFIG_SMALL("Windows Media Audio Voice"),
-- 
2.8.1

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

[FFmpeg-devel] [PATCH 3/4] wmavoice: reindent.

[Ronald S. Bultje]

---
 libavcodec/wmavoice.c | 72 +--
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index ae100fb..f31c9d2 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1915,29 +1915,29 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 /* If the packet header specifies a s->spillover_nbits, then we want
  * to push out all data of the previous packet (+ spillover) before
  * continuing to parse new superframes in the current packet. */
-if (s->sframe_cache_size > 0) {
-int cnt = get_bits_count(gb);
-copy_bits(>pb, avpkt->data, size, gb, s->spillover_nbits);
-flush_put_bits(>pb);
-s->sframe_cache_size += s->spillover_nbits;
-if ((res = synth_superframe(ctx, data, got_frame_ptr)) == 0 &&
-*got_frame_ptr) {
-cnt += s->spillover_nbits;
-s->skip_bits_next = cnt & 7;
-res = cnt >> 3;
-if (res > avpkt->size) {
-av_log(ctx, AV_LOG_ERROR,
-   "Trying to skip %d bytes in packet of size 
%d\n",
-   res, avpkt->size);
-return AVERROR_INVALIDDATA;
-}
-return res;
-} else
-skip_bits_long (gb, s->spillover_nbits - cnt +
-get_bits_count(gb)); // resync
-} else if (s->spillover_nbits) {
-skip_bits_long(gb, s->spillover_nbits);  // resync
-}
+if (s->sframe_cache_size > 0) {
+int cnt = get_bits_count(gb);
+copy_bits(>pb, avpkt->data, size, gb, s->spillover_nbits);
+flush_put_bits(>pb);
+s->sframe_cache_size += s->spillover_nbits;
+if ((res = synth_superframe(ctx, data, got_frame_ptr)) == 0 &&
+*got_frame_ptr) {
+cnt += s->spillover_nbits;
+s->skip_bits_next = cnt & 7;
+res = cnt >> 3;
+if (res > avpkt->size) {
+av_log(ctx, AV_LOG_ERROR,
+   "Trying to skip %d bytes in packet of size %d\n",
+   res, avpkt->size);
+return AVERROR_INVALIDDATA;
+}
+return res;
+} else
+skip_bits_long (gb, s->spillover_nbits - cnt +
+get_bits_count(gb)); // resync
+} else if (s->spillover_nbits) {
+skip_bits_long(gb, s->spillover_nbits);  // resync
+}
 } else if (s->skip_bits_next)
 skip_bits(gb, s->skip_bits_next);
 
@@ -1949,20 +1949,20 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 *got_frame_ptr = 0;
 return size;
 } else if (s->nb_superframes > 0) {
-if ((res = synth_superframe(ctx, data, got_frame_ptr)) < 0) {
-return res;
-} else if (*got_frame_ptr) {
-int cnt = get_bits_count(gb);
-s->skip_bits_next = cnt & 7;
-res = cnt >> 3;
-if (res > avpkt->size) {
-av_log(ctx, AV_LOG_ERROR,
-   "Trying to skip %d bytes in packet of size %d\n",
-   res, avpkt->size);
-return AVERROR_INVALIDDATA;
+if ((res = synth_superframe(ctx, data, got_frame_ptr)) < 0) {
+return res;
+} else if (*got_frame_ptr) {
+int cnt = get_bits_count(gb);
+s->skip_bits_next = cnt & 7;
+res = cnt >> 3;
+if (res > avpkt->size) {
+av_log(ctx, AV_LOG_ERROR,
+   "Trying to skip %d bytes in packet of size %d\n",
+   res, avpkt->size);
+return AVERROR_INVALIDDATA;
+}
+return res;
 }
-return res;
-}
 } else if ((s->sframe_cache_size = pos) > 0) {
 /* ... cache it for spillover in next packet */
 init_put_bits(>pb, s->sframe_cache, SFRAME_CACHE_MAXSIZE);
-- 
2.8.1

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

[FFmpeg-devel] [PATCH 1/2] wmavoice: disable bitstream checking.

[Ronald S. Bultje]

The checked bitstream reader does that already. To allow parsing of
superframes split over a packet boundary, we always decode the last
superframe in each packet at the start of the next packet, even if
theoretically we could have decoded it. The last superframe in the
last packet is decoded using AV_CODEC_CAP_DELAY.
---
 libavcodec/wmavoice.c | 160 --
 1 file changed, 38 insertions(+), 122 deletions(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index ba02c7d..5001b0b 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -251,6 +251,7 @@ typedef struct WMAVoiceContext {
 
 int frame_cntr;   ///< current frame index [0 - 0xFFFE]; is
   ///< only used for comfort noise in #pRNG()
+int nb_superframes;   ///< number of superframes in current packet
 float gain_pred_err[6];   ///< cache for gain prediction
 float excitation_history[MAX_SIGNAL_HISTORY];
   ///< cache of the signal of previous
@@ -847,7 +848,6 @@ static void dequant_lsps(double *lsps, int num,
 /**
  * @name LSP dequantization routines
  * LSP dequantization routines, for 10/16LSPs and independent/residual coding.
- * @note we assume enough bits are available, caller should check.
  * lsp10i() consumes 24 bits; lsp10r() consumes an additional 24 bits;
  * lsp16i() consumes 34 bits; lsp16r() consumes an additional 26 bits.
  * @{
@@ -1391,7 +1391,6 @@ static void synth_block_fcb_acb(WMAVoiceContext *s, 
GetBitContext *gb,
 
 /**
  * Parse data in a single block.
- * @note we assume enough bits are available, caller should check.
  *
  * @param s WMA Voice decoding context private data
  * @param gb bit I/O context
@@ -1435,7 +1434,6 @@ static void synth_block(WMAVoiceContext *s, GetBitContext 
*gb,
 
 /**
  * Synthesize output samples for a single frame.
- * @note we assume enough bits are available, caller should check.
  *
  * @param ctx WMA Voice decoder context
  * @param gb bit I/O context (s->gb or one for cross-packet superframes)
@@ -1654,83 +1652,6 @@ static void stabilize_lsps(double *lsps, int num)
 }
 
 /**
- * Test if there's enough bits to read 1 superframe.
- *
- * @param orig_gb bit I/O context used for reading. This function
- *does not modify the state of the bitreader; it
- *only uses it to copy the current stream position
- * @param s WMA Voice decoding context private data
- * @return < 0 on error, 1 on not enough bits or 0 if OK.
- */
-static int check_bits_for_superframe(GetBitContext *orig_gb,
- WMAVoiceContext *s)
-{
-GetBitContext s_gb, *gb = _gb;
-int n, need_bits, bd_idx;
-const struct frame_type_desc *frame_desc;
-
-/* initialize a copy */
-init_get_bits(gb, orig_gb->buffer, orig_gb->size_in_bits);
-skip_bits_long(gb, get_bits_count(orig_gb));
-av_assert1(get_bits_left(gb) == get_bits_left(orig_gb));
-
-/* superframe header */
-if (get_bits_left(gb) < 14)
-return 1;
-if (!get_bits1(gb))
-return AVERROR(ENOSYS);   // WMAPro-in-WMAVoice superframe
-if (get_bits1(gb)) skip_bits(gb, 12); // number of  samples in superframe
-if (s->has_residual_lsps) {   // residual LSPs (for all frames)
-if (get_bits_left(gb) < s->sframe_lsp_bitsize)
-return 1;
-skip_bits_long(gb, s->sframe_lsp_bitsize);
-}
-
-/* frames */
-for (n = 0; n < MAX_FRAMES; n++) {
-int aw_idx_is_ext = 0;
-
-if (!s->has_residual_lsps) { // independent LSPs (per-frame)
-   if (get_bits_left(gb) < s->frame_lsp_bitsize) return 1;
-   skip_bits_long(gb, s->frame_lsp_bitsize);
-}
-bd_idx = s->vbm_tree[get_vlc2(gb, frame_type_vlc.table, 6, 3)];
-if (bd_idx < 0)
-return AVERROR_INVALIDDATA; // invalid frame type VLC code
-frame_desc = _descs[bd_idx];
-if (frame_desc->acb_type == ACB_TYPE_ASYMMETRIC) {
-if (get_bits_left(gb) < s->pitch_nbits)
-return 1;
-skip_bits_long(gb, s->pitch_nbits);
-}
-if (frame_desc->fcb_type == FCB_TYPE_SILENCE) {
-skip_bits(gb, 8);
-} else if (frame_desc->fcb_type == FCB_TYPE_AW_PULSES) {
-int tmp = get_bits(gb, 6);
-if (tmp >= 0x36) {
-skip_bits(gb, 2);
-aw_idx_is_ext = 1;
-}
-}
-
-/* blocks */
-if (frame_desc->acb_type == ACB_TYPE_HAMMING) {
-need_bits = s->block_pitch_nbits +
-(frame_desc->n_blocks - 1) * s->block_delta_pitch_nbits;
-} else if (frame_desc->fcb_type == FCB_TYPE_AW_PULSES) {
-need_bits = 2 * !aw_idx_is_ext;
-} else
-need_bits = 0;
-need_bits += frame_desc->frame_size;
-if (get_bits_left(gb) < need_bits)
-return 1;
-   

[FFmpeg-devel] [PATCH 2/2] wmavoice: reindent.

[Ronald S. Bultje]

---
 libavcodec/wmavoice.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index 5001b0b..5393003 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1887,23 +1887,23 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 /* If the packet header specifies a s->spillover_nbits, then we want
  * to push out all data of the previous packet (+ spillover) before
  * continuing to parse new superframes in the current packet. */
-if (s->sframe_cache_size > 0) {
-int cnt = get_bits_count(gb);
-copy_bits(>pb, avpkt->data, size, gb, s->spillover_nbits);
-flush_put_bits(>pb);
-s->sframe_cache_size += s->spillover_nbits;
-if ((res = synth_superframe(ctx, data, got_frame_ptr)) == 0 &&
-*got_frame_ptr) {
-cnt += s->spillover_nbits;
-s->skip_bits_next = cnt & 7;
-res = cnt >> 3;
-return FFMIN(avpkt->size, res);
-} else
-skip_bits_long(gb, s->spillover_nbits - cnt +
-   get_bits_count(gb)); // resync
-} else if (s->spillover_nbits) {
-skip_bits_long(gb, s->spillover_nbits);  // resync
-}
+if (s->sframe_cache_size > 0) {
+int cnt = get_bits_count(gb);
+copy_bits(>pb, avpkt->data, size, gb, s->spillover_nbits);
+flush_put_bits(>pb);
+s->sframe_cache_size += s->spillover_nbits;
+if ((res = synth_superframe(ctx, data, got_frame_ptr)) == 0 &&
+*got_frame_ptr) {
+cnt += s->spillover_nbits;
+s->skip_bits_next = cnt & 7;
+res = cnt >> 3;
+return FFMIN(avpkt->size, res);
+} else
+skip_bits_long(gb, s->spillover_nbits - cnt +
+   get_bits_count(gb)); // resync
+} else if (s->spillover_nbits) {
+skip_bits_long(gb, s->spillover_nbits);  // resync
+}
 } else if (s->skip_bits_next)
 skip_bits(gb, s->skip_bits_next);
 
-- 
2.8.1

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

Re: [FFmpeg-devel] [PATCH 1/2] wmavoice: don't check if we have enough bits to read a superframe.

[Ronald S. Bultje]

Hi,

On Fri, Dec 16, 2016 at 9:49 AM, Michael Niedermayer <mich...@niedermayer.cc
> wrote:

> On Fri, Dec 16, 2016 at 08:19:44AM -0500, Ronald S. Bultje wrote:
> > The checked bitstream reader makes this unnecessary.
> > ---
> >  libavcodec/wmavoice.c | 83 --
> -
> >  1 file changed, 83 deletions(-)
>
> this breaks fate
>
> TESTwmavoice-19k


I noticed, I'm looking, consider this a proof of concept rather than
something meant for committing for now...

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

[FFmpeg-devel] [PATCH] wmavoice: don't check if we have enough bits to read a superframe.

[Ronald S. Bultje]

The checked bitstream reader makes this unnecessary.
---
 libavcodec/wmavoice.c | 86 ++-
 1 file changed, 2 insertions(+), 84 deletions(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index ba02c7d..e4ea128 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1654,83 +1654,6 @@ static void stabilize_lsps(double *lsps, int num)
 }
 
 /**
- * Test if there's enough bits to read 1 superframe.
- *
- * @param orig_gb bit I/O context used for reading. This function
- *does not modify the state of the bitreader; it
- *only uses it to copy the current stream position
- * @param s WMA Voice decoding context private data
- * @return < 0 on error, 1 on not enough bits or 0 if OK.
- */
-static int check_bits_for_superframe(GetBitContext *orig_gb,
- WMAVoiceContext *s)
-{
-GetBitContext s_gb, *gb = _gb;
-int n, need_bits, bd_idx;
-const struct frame_type_desc *frame_desc;
-
-/* initialize a copy */
-init_get_bits(gb, orig_gb->buffer, orig_gb->size_in_bits);
-skip_bits_long(gb, get_bits_count(orig_gb));
-av_assert1(get_bits_left(gb) == get_bits_left(orig_gb));
-
-/* superframe header */
-if (get_bits_left(gb) < 14)
-return 1;
-if (!get_bits1(gb))
-return AVERROR(ENOSYS);   // WMAPro-in-WMAVoice superframe
-if (get_bits1(gb)) skip_bits(gb, 12); // number of  samples in superframe
-if (s->has_residual_lsps) {   // residual LSPs (for all frames)
-if (get_bits_left(gb) < s->sframe_lsp_bitsize)
-return 1;
-skip_bits_long(gb, s->sframe_lsp_bitsize);
-}
-
-/* frames */
-for (n = 0; n < MAX_FRAMES; n++) {
-int aw_idx_is_ext = 0;
-
-if (!s->has_residual_lsps) { // independent LSPs (per-frame)
-   if (get_bits_left(gb) < s->frame_lsp_bitsize) return 1;
-   skip_bits_long(gb, s->frame_lsp_bitsize);
-}
-bd_idx = s->vbm_tree[get_vlc2(gb, frame_type_vlc.table, 6, 3)];
-if (bd_idx < 0)
-return AVERROR_INVALIDDATA; // invalid frame type VLC code
-frame_desc = _descs[bd_idx];
-if (frame_desc->acb_type == ACB_TYPE_ASYMMETRIC) {
-if (get_bits_left(gb) < s->pitch_nbits)
-return 1;
-skip_bits_long(gb, s->pitch_nbits);
-}
-if (frame_desc->fcb_type == FCB_TYPE_SILENCE) {
-skip_bits(gb, 8);
-} else if (frame_desc->fcb_type == FCB_TYPE_AW_PULSES) {
-int tmp = get_bits(gb, 6);
-if (tmp >= 0x36) {
-skip_bits(gb, 2);
-aw_idx_is_ext = 1;
-}
-}
-
-/* blocks */
-if (frame_desc->acb_type == ACB_TYPE_HAMMING) {
-need_bits = s->block_pitch_nbits +
-(frame_desc->n_blocks - 1) * s->block_delta_pitch_nbits;
-} else if (frame_desc->fcb_type == FCB_TYPE_AW_PULSES) {
-need_bits = 2 * !aw_idx_is_ext;
-} else
-need_bits = 0;
-need_bits += frame_desc->frame_size;
-if (get_bits_left(gb) < need_bits)
-return 1;
-skip_bits_long(gb, need_bits);
-}
-
-return 0;
-}
-
-/**
  * Synthesize output samples for a single superframe. If we have any data
  * cached in s->sframe_cache, that will be used instead of whatever is loaded
  * in s->gb.
@@ -1771,12 +1694,6 @@ static int synth_superframe(AVCodecContext *ctx, AVFrame 
*frame,
 s->sframe_cache_size = 0;
 }
 
-if ((res = check_bits_for_superframe(gb, s)) == 1) {
-*got_frame_ptr = 0;
-return 1;
-} else if (res < 0)
-return res;
-
 /* First bit is speech/music bit, it differentiates between WMAVoice
  * speech samples (the actual codec) and WMAVoice music samples, which
  * are really WMAPro-in-WMAVoice-superframes. I've never seen those in
@@ -1999,7 +1916,7 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 pos = get_bits_left(gb);
 if ((res = synth_superframe(ctx, data, got_frame_ptr)) < 0) {
 return res;
-} else if (*got_frame_ptr) {
+} else if (*got_frame_ptr && get_bits_left(>gb) >= 0) {
 int cnt = get_bits_count(gb);
 s->skip_bits_next = cnt & 7;
 res = cnt >> 3;
@@ -2015,6 +1932,7 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 copy_bits(>pb, avpkt->data, size, gb, s->sframe_cache_size);
 // FIXME bad - just copy bytes as whole and add use the
 // skip_bits_next field
+*got_frame_ptr = 0;
 }
 
 return size;
-- 
2.8.1

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

[FFmpeg-devel] [PATCH 2/2] wmavoice: don't error out if we're skipping more bits than available.

[Ronald S. Bultje]

This reverts 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e and implements it
correctly so streams actually decode the way the encoder intended them
to.
---
 libavcodec/wmavoice.c | 20 
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index 0f29bdd..f1b5369 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1900,16 +1900,10 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 cnt += s->spillover_nbits;
 s->skip_bits_next = cnt & 7;
 res = cnt >> 3;
-if (res > avpkt->size) {
-av_log(ctx, AV_LOG_ERROR,
-   "Trying to skip %d bytes in packet of size 
%d\n",
-   res, avpkt->size);
-return AVERROR_INVALIDDATA;
-}
-return res;
+return FFMIN(avpkt->size, res);
 } else
-skip_bits_long (gb, s->spillover_nbits - cnt +
-get_bits_count(gb)); // resync
+skip_bits_long(gb, s->spillover_nbits - cnt +
+   get_bits_count(gb)); // resync
 } else
 skip_bits_long(gb, s->spillover_nbits);  // resync
 }
@@ -1926,13 +1920,7 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, 
void *data,
 int cnt = get_bits_count(gb);
 s->skip_bits_next = cnt & 7;
 res = cnt >> 3;
-if (res > avpkt->size) {
-av_log(ctx, AV_LOG_ERROR,
-   "Trying to skip %d bytes in packet of size %d\n",
-   res, avpkt->size);
-return AVERROR_INVALIDDATA;
-}
-return res;
+return FFMIN(res, avpkt->size);
 } else if ((s->sframe_cache_size = pos) > 0) {
 /* rewind bit reader to start of last (incomplete) superframe... */
 init_get_bits(gb, avpkt->data, size << 3);
-- 
2.8.1

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

[FFmpeg-devel] [PATCH 1/2] wmavoice: don't check if we have enough bits to read a superframe.

[Ronald S. Bultje]

The checked bitstream reader makes this unnecessary.
---
 libavcodec/wmavoice.c | 83 ---
 1 file changed, 83 deletions(-)

diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c
index ceac61f..0f29bdd 100644
--- a/libavcodec/wmavoice.c
+++ b/libavcodec/wmavoice.c
@@ -1654,83 +1654,6 @@ static void stabilize_lsps(double *lsps, int num)
 }
 
 /**
- * Test if there's enough bits to read 1 superframe.
- *
- * @param orig_gb bit I/O context used for reading. This function
- *does not modify the state of the bitreader; it
- *only uses it to copy the current stream position
- * @param s WMA Voice decoding context private data
- * @return < 0 on error, 1 on not enough bits or 0 if OK.
- */
-static int check_bits_for_superframe(GetBitContext *orig_gb,
- WMAVoiceContext *s)
-{
-GetBitContext s_gb, *gb = _gb;
-int n, need_bits, bd_idx;
-const struct frame_type_desc *frame_desc;
-
-/* initialize a copy */
-init_get_bits(gb, orig_gb->buffer, orig_gb->size_in_bits);
-skip_bits_long(gb, get_bits_count(orig_gb));
-av_assert1(get_bits_left(gb) == get_bits_left(orig_gb));
-
-/* superframe header */
-if (get_bits_left(gb) < 14)
-return 1;
-if (!get_bits1(gb))
-return AVERROR(ENOSYS);   // WMAPro-in-WMAVoice superframe
-if (get_bits1(gb)) skip_bits(gb, 12); // number of  samples in superframe
-if (s->has_residual_lsps) {   // residual LSPs (for all frames)
-if (get_bits_left(gb) < s->sframe_lsp_bitsize)
-return 1;
-skip_bits_long(gb, s->sframe_lsp_bitsize);
-}
-
-/* frames */
-for (n = 0; n < MAX_FRAMES; n++) {
-int aw_idx_is_ext = 0;
-
-if (!s->has_residual_lsps) { // independent LSPs (per-frame)
-   if (get_bits_left(gb) < s->frame_lsp_bitsize) return 1;
-   skip_bits_long(gb, s->frame_lsp_bitsize);
-}
-bd_idx = s->vbm_tree[get_vlc2(gb, frame_type_vlc.table, 6, 3)];
-if (bd_idx < 0)
-return AVERROR_INVALIDDATA; // invalid frame type VLC code
-frame_desc = _descs[bd_idx];
-if (frame_desc->acb_type == ACB_TYPE_ASYMMETRIC) {
-if (get_bits_left(gb) < s->pitch_nbits)
-return 1;
-skip_bits_long(gb, s->pitch_nbits);
-}
-if (frame_desc->fcb_type == FCB_TYPE_SILENCE) {
-skip_bits(gb, 8);
-} else if (frame_desc->fcb_type == FCB_TYPE_AW_PULSES) {
-int tmp = get_bits(gb, 6);
-if (tmp >= 0x36) {
-skip_bits(gb, 2);
-aw_idx_is_ext = 1;
-}
-}
-
-/* blocks */
-if (frame_desc->acb_type == ACB_TYPE_HAMMING) {
-need_bits = s->block_pitch_nbits +
-(frame_desc->n_blocks - 1) * s->block_delta_pitch_nbits;
-} else if (frame_desc->fcb_type == FCB_TYPE_AW_PULSES) {
-need_bits = 2 * !aw_idx_is_ext;
-} else
-need_bits = 0;
-need_bits += frame_desc->frame_size;
-if (get_bits_left(gb) < need_bits)
-return 1;
-skip_bits_long(gb, need_bits);
-}
-
-return 0;
-}
-
-/**
  * Synthesize output samples for a single superframe. If we have any data
  * cached in s->sframe_cache, that will be used instead of whatever is loaded
  * in s->gb.
@@ -1771,12 +1694,6 @@ static int synth_superframe(AVCodecContext *ctx, AVFrame 
*frame,
 s->sframe_cache_size = 0;
 }
 
-if ((res = check_bits_for_superframe(gb, s)) == 1) {
-*got_frame_ptr = 0;
-return 1;
-} else if (res < 0)
-return res;
-
 /* First bit is speech/music bit, it differentiates between WMAVoice
  * speech samples (the actual codec) and WMAVoice music samples, which
  * are really WMAPro-in-WMAVoice-superframes. I've never seen those in
-- 
2.8.1

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

Re: [FFmpeg-devel] [PATCH 1/3] 4xm: prevent overflow during bit rate calculation

[Ronald S. Bultje]

Hi,

On Thu, Dec 15, 2016 at 9:28 AM, Michael Niedermayer <mich...@niedermayer.cc
> wrote:

> On Thu, Dec 15, 2016 at 08:02:52AM -0500, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
> > andreas.cadhal...@googlemail.com> wrote:
> >
> > > On 14.12.2016 02:46, Ronald S. Bultje wrote:
> > > > Not wanting to discourage you, but I wonder if there's really a
> point to
> > > > this...?
> > >
> > > These patches are prerequisites for enforcing the validity of codec
> > > parameters [1].
> > >
> > > > I don't see how the user experience changes.
> > >
> > > Users won't see ffmpeg claiming nonsense bit rates like
> -1184293205235990
> > > kb/s
> > > anymore.
> >
> >
> > I don't think you understand my question.
> >
> > - does this belong in 4xm.c? (Or in generic code? Or in the app?)
> > - should it return an error? (Or just clip parameters? Or ignore the
> > invalid value?)
> >
> > > This isn't specifically intended at this patch, but rather at the sort
> of
> > > > rabbit hole this change might lead to,
> > >
> > > I have a pretty good map of this rabbit hole (i.e. lots of samples
> > > triggering
> > > UBSan errors) and one day I might try to dig it up, but for now I'm
> > > limiting
> > > myself to the codec parameters.
> >
> >
> > I'm not saying mplayer was great, but one of the principles I believe we
> > always copied from them was to try to play files to the best of our
> > abilities and not error out at the first convenience. This isn't just a
> > theoretical thing, a lot of people credited mplayer with playing utterly
> > broken AVI files that probably even ffmpeg rejects. What ffmpeg added on
> > top of that is to make a project maintainable by not being an utter
> > crapshoot.
> >
> > This isn't 4xm.c-specific, this is a general philosophical question:
> > - should we error out?
> > - should this be in generic code if we're likely to repeat such checks
> all
> > over the place?
> >
> > > which would cause the code to be uber-full of such checks, none of
> which
> > > > really have any significance. But maybe others disagree...
> > >
> > > Not relying on undefined behavior is a significant improvement. And
> doing
> > > these checks consequently where the values are set makes it possible
> > > for other code to rely on their validity without further checks.
> >
> >
> > Unless "UB" leads to actual bad behaviour, I personally don't necessarily
> > care. I'm very scared that when you go beyond codec parameters, and you
> > want to check for overflows all over the place, we'll never see the end
> of
> > it...
> >
>
> > I'd appreciate if others could chime in here, I don't want to carry this
> > argument all by myself if nobody cares.
>
> as you are asking for others oppinion
> valid C code must not trigger undefined behavior


So, I asked on IRC, here's 3 suggestions from Roger Combs:

- in case we want to be pedantic (I personally don't care, but I understand
other people are), it might make sense to just make these members
(channels, block_align, bitrate) unsigned. That removes the UB of the
overflow, and it fixes the negative number reporting in client apps for
bitrate, but you can still have positive crazy numbers and it doesn't
return an error.
- if for whatever reason some things cannot be done in generic code or by
changing the type (this should really cover most cases), and we want
specific overflow checks, then maybe we want to have some generic helper
macros that make them one-liners in decoders. This would return an error
along with fixing the UB.
- have overflow-safe multiplication functions instead of checking each
argument before doing a multiply, like __builtin_mul_overflow, and then
return INT64_MAX if it would overflow inside that function. This fixes
display of numbers in client applications and the UB, but without returning
an error.

What I want most - and this comment applies to all patches of this sort -
is to have something that we can all agree is OK for pretty much any
decoder out there without significant overhead in code (source - not
binary) size. This way, you have a template to work from and fix specific
instances without us having to argue over every single time you do a next
run with ubsan. I personally like suggestion (1), unsigned is a pretty good
type for things that shouldn't have negative values. WDYT?

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

Re: [FFmpeg-devel] [PATCH 6/6] pvfdec: prevent overflow during block alignment, calculation

[Ronald S. Bultje]

Hi,

On Wed, Dec 14, 2016 at 8:19 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavformat/pvfdec.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/libavformat/pvfdec.c b/libavformat/pvfdec.c
> index b9f6d4f..5eecc22 100644
> --- a/libavformat/pvfdec.c
> +++ b/libavformat/pvfdec.c
> @@ -56,6 +56,11 @@ static int pvf_read_header(AVFormatContext *s)
>  st->codecpar->sample_rate = sample_rate;
>  st->codecpar->codec_id= ff_get_pcm_codec_id(bps, 0, 1, 0x);
>  st->codecpar->bits_per_coded_sample = bps;
> +if (bps > INT_MAX / st->codecpar->channels) {
> +av_log(s, AV_LOG_ERROR, "Overflow during block alignment
> calculation %d * %d\n",
> +   bps, st->codecpar->channels);
> +return AVERROR_INVALIDDATA;
> +}


And this is what I meant.

Please stop. No. No. No. No. No. Not in codec code. Add these checks in
generic code if you care about the outcome, but please don't make each
codec a crapshoot like this.

Please. From a maintenance point of view, that's a much better approach.
Please stop for a second and think about my point of view here. I beg you.
Please.

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

Re: [FFmpeg-devel] [PATCH 1/3] 4xm: prevent overflow during bit rate calculation

[Ronald S. Bultje]

Hi,

On Wed, Dec 14, 2016 at 7:11 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 14.12.2016 02:46, Ronald S. Bultje wrote:
> > Not wanting to discourage you, but I wonder if there's really a point to
> > this...?
>
> These patches are prerequisites for enforcing the validity of codec
> parameters [1].
>
> > I don't see how the user experience changes.
>
> Users won't see ffmpeg claiming nonsense bit rates like -1184293205235990
> kb/s
> anymore.


I don't think you understand my question.

- does this belong in 4xm.c? (Or in generic code? Or in the app?)
- should it return an error? (Or just clip parameters? Or ignore the
invalid value?)

> This isn't specifically intended at this patch, but rather at the sort of
> > rabbit hole this change might lead to,
>
> I have a pretty good map of this rabbit hole (i.e. lots of samples
> triggering
> UBSan errors) and one day I might try to dig it up, but for now I'm
> limiting
> myself to the codec parameters.


I'm not saying mplayer was great, but one of the principles I believe we
always copied from them was to try to play files to the best of our
abilities and not error out at the first convenience. This isn't just a
theoretical thing, a lot of people credited mplayer with playing utterly
broken AVI files that probably even ffmpeg rejects. What ffmpeg added on
top of that is to make a project maintainable by not being an utter
crapshoot.

This isn't 4xm.c-specific, this is a general philosophical question:
- should we error out?
- should this be in generic code if we're likely to repeat such checks all
over the place?

> which would cause the code to be uber-full of such checks, none of which
> > really have any significance. But maybe others disagree...
>
> Not relying on undefined behavior is a significant improvement. And doing
> these checks consequently where the values are set makes it possible
> for other code to rely on their validity without further checks.


Unless "UB" leads to actual bad behaviour, I personally don't necessarily
care. I'm very scared that when you go beyond codec parameters, and you
want to check for overflows all over the place, we'll never see the end of
it...

I'd appreciate if others could chime in here, I don't want to carry this
argument all by myself if nobody cares.

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

Re: [FFmpeg-devel] [PATCH 1/3] 4xm: prevent overflow during bit rate calculation

[Ronald S. Bultje]

Hi,

On Tue, Dec 13, 2016 at 8:21 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 14.12.2016 02:01, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Tue, Dec 13, 2016 at 7:57 PM, Andreas Cadhalpun <
> > andreas.cadhal...@googlemail.com> wrote:
> >
> >> Signed-off-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
> >> ---
> >>  libavformat/4xm.c | 8 +++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
> >> index 8a50778..2758b69 100644
> >> --- a/libavformat/4xm.c
> >> +++ b/libavformat/4xm.c
> >> @@ -163,6 +163,12 @@ static int parse_strk(AVFormatContext *s,
> >>  return AVERROR_INVALIDDATA;
> >>  }
> >>
> >> +if (fourxm->tracks[track].sample_rate > INT64_MAX /
> >> fourxm->tracks[track].bits / fourxm->tracks[track].channels) {
> >> +av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation
> %d
> >> * %d * %d\n",
> >> +   fourxm->tracks[track].sample_rate,
> >> fourxm->tracks[track].bits, fourxm->tracks[track].channels);
> >> +return AVERROR_INVALIDDATA;
> >> +}
> >
> >
> > What is the functional effect of the overflow?
>
> It is undefined behavior.
>
> > Does it crash? Or is there some other security issue?
>
> The most likely behavior is that bit_rate will contain a negative value,
> which might cause problems later on, but I'm not aware of specific
> security issues caused by this.


Not wanting to discourage you, but I wonder if there's really a point to
this...? I don't see how the user experience changes.

This isn't specifically intended at this patch, but rather at the sort of
rabbit hole this change might lead to, which would cause the code to be
uber-full of such checks, none of which really have any significance. But
maybe others disagree...

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

Re: [FFmpeg-devel] [PATCH 1/3] 4xm: prevent overflow during bit rate calculation

[Ronald S. Bultje]

Hi,

On Tue, Dec 13, 2016 at 7:57 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> Signed-off-by: Andreas Cadhalpun 
> ---
>  libavformat/4xm.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
> index 8a50778..2758b69 100644
> --- a/libavformat/4xm.c
> +++ b/libavformat/4xm.c
> @@ -163,6 +163,12 @@ static int parse_strk(AVFormatContext *s,
>  return AVERROR_INVALIDDATA;
>  }
>
> +if (fourxm->tracks[track].sample_rate > INT64_MAX /
> fourxm->tracks[track].bits / fourxm->tracks[track].channels) {
> +av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation %d
> * %d * %d\n",
> +   fourxm->tracks[track].sample_rate,
> fourxm->tracks[track].bits, fourxm->tracks[track].channels);
> +return AVERROR_INVALIDDATA;
> +}


What is the functional effect of the overflow? Does it crash? Or is there
some other security issue?

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

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

[Ronald S. Bultje]

Hi,

On Sun, Dec 11, 2016 at 1:41 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 10.12.2016 23:22, Carl Eugen Hoyos wrote:
> > 2016-12-10 19:47 GMT+01:00 Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com>:
> >> Suggested-by: Carl Eugen Hoyos 
> >> Signed-off-by: Andreas Cadhalpun 
> >> ---
> >>  src/index | 7 +++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/src/index b/src/index
> >> index c203676..8f533f5 100644
> >> --- a/src/index
> >> +++ b/src/index
> >> @@ -37,6 +37,13 @@
> >>  News
> >>
> >>
> >> +  December 10th, 2016, stricter configure
> behavior.
> >> +  
> >> +The configure script now fails if autodetect-libraries are
> requested but not found.
> >> +  
> >> +  
> >> +If for example SDL headers are not available, '--enable-sdl2' will
> cause a configure failure.
> >
> > Please push, esp. since the relevant change is already in git.
>
> This trivial news patch already got way more comments than the original
> patch series, which gives me the impression that people care more about
> the website than the actual code. :-/


That's unfair and somewhat offensive to those of us that commented in this
thread but do more than enough to keep our tree healthy and fresh. We might
not care specifically about configure patches, but we may certainly well
care what goes on our website, since that reflects very specifically and
very focused on all of us.

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

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

[Ronald S. Bultje]

Hi,

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

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


... according to you...

Right?

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

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

[Ronald S. Bultje]

Hi,

On Sat, Dec 10, 2016 at 5:24 PM, Carl Eugen Hoyos <ceffm...@gmail.com>
wrote:

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


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

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

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

[Ronald S. Bultje]

Hi,

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

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


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

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

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

[Ronald S. Bultje]

Hi,

On Sat, Dec 10, 2016 at 7:11 AM, Carl Eugen Hoyos <ceffm...@gmail.com>
wrote:

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


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

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

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

[Ronald S. Bultje]

Hi,

On Thu, Dec 8, 2016 at 7:03 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 08.12.2016 22:59, Carl Eugen Hoyos wrote:
> > 2016-12-08 18:37 GMT+01:00 Michael Niedermayer :
> >
> >> -{"max_streams", "maximum number of streams", OFFSET(max_streams),
> AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D },
> >> +{"max_streams", "maximum number of streams", OFFSET(max_streams),
> AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D },
> >
> > I wanted to suggest 1000 which is still a magnitude less than the
> provided
> > crashing sample but 255 also sounds ok to me.
>
> Either value is OK. The important thing is that it's several orders of
> magnitude lower than INT_MAX.


On IRC, we discussed at what values OOM start occurring, which seems to be
around 30k-60k, so from there I suggested a value like 10k or 5k. 1000
seems a little low but I think I can live with it (I doubt ATM I can come
up with legit use cases that use 1000 streams).

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

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

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

[Ronald S. Bultje]

Hi,

On Thu, Dec 8, 2016 at 1:25 PM, James Almer <jamr...@gmail.com> wrote:

> On 12/8/2016 2:44 PM, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Thu, Dec 8, 2016 at 12:37 PM, Michael Niedermayer
> <mich...@niedermayer.cc
> >> wrote:
> >
> >> Suggested-by: Andreas Cadhalpun <andreas.cadhal...@googlemail.com>
> >> Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc>
> >> ---
> >>  libavformat/options_table.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/options_table.h b/libavformat/options_table.h
> >> index d5448e503f..19cb87ae4e 100644
> >> --- a/libavformat/options_table.h
> >> +++ b/libavformat/options_table.h
> >> @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = {
> >>  {"format_whitelist", "List of demuxers that are allowed to be used",
> >> OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },
> CHAR_MIN,
> >> CHAR_MAX, D },
> >>  {"protocol_whitelist", "List of protocols that are allowed to be used",
> >> OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },
> CHAR_MIN,
> >> CHAR_MAX, D },
> >>  {"protocol_blacklist", "List of protocols that are not allowed to be
> >> used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL },
> >> CHAR_MIN, CHAR_MAX, D },
> >> -{"max_streams", "maximum number of streams", OFFSET(max_streams),
> >> AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D },
> >> +{"max_streams", "maximum number of streams", OFFSET(max_streams),
> >> AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D },
> >>  {NULL},
> >>  };
> >
> >
> > Isn't this - in some way - an ABI break?
> >
> > Ronald
>
> The option in question was added an hour ago, so no.


I meant that legal input with 101 streams stop working. That's not crazy
IMO. IOW, 100 is kinda low.

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

Re: [FFmpeg-devel] [PATCH] aarch64: h264idct: Use the offset parameter to movrel

[Ronald S. Bultje]

Hi,

On Thu, Dec 8, 2016 at 12:14 PM, Michael Niedermayer <mich...@niedermayer.cc
> wrote:

> On Thu, Dec 08, 2016 at 12:01:33PM -0500, Ronald S. Bultje wrote:
> > From: Martin Storsjö <mar...@martin.st>
> >
> > Signed-off-by: Martin Storsjö <mar...@martin.st>
> > Signed-off-by: Ronald S. Bultje <rsbul...@gmail.com>
> > ---
> >  libavcodec/aarch64/h264idct_neon.S | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> didnt see this
> martin pinged me privatly on IRC and i already pushed this


That's fine also, tnx.

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

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

[Ronald S. Bultje]

Hi,

On Thu, Dec 8, 2016 at 12:37 PM, Michael Niedermayer  wrote:

> Suggested-by: Andreas Cadhalpun 
> Signed-off-by: Michael Niedermayer 
> ---
>  libavformat/options_table.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/options_table.h b/libavformat/options_table.h
> index d5448e503f..19cb87ae4e 100644
> --- a/libavformat/options_table.h
> +++ b/libavformat/options_table.h
> @@ -105,7 +105,7 @@ static const AVOption avformat_options[] = {
>  {"format_whitelist", "List of demuxers that are allowed to be used",
> OFFSET(format_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN,
> CHAR_MAX, D },
>  {"protocol_whitelist", "List of protocols that are allowed to be used",
> OFFSET(protocol_whitelist), AV_OPT_TYPE_STRING, { .str = NULL },  CHAR_MIN,
> CHAR_MAX, D },
>  {"protocol_blacklist", "List of protocols that are not allowed to be
> used", OFFSET(protocol_blacklist), AV_OPT_TYPE_STRING, { .str = NULL },
> CHAR_MIN, CHAR_MAX, D },
> -{"max_streams", "maximum number of streams", OFFSET(max_streams),
> AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 0, INT_MAX, D },
> +{"max_streams", "maximum number of streams", OFFSET(max_streams),
> AV_OPT_TYPE_INT, { .i64 = 100 }, 0, INT_MAX, D },
>  {NULL},
>  };


Isn't this - in some way - an ABI break?

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

[FFmpeg-devel] [PATCH] aarch64: h264idct: Use the offset parameter to movrel

[Ronald S. Bultje]

From: Martin Storsjö <mar...@martin.st>

Signed-off-by: Martin Storsjö <mar...@martin.st>
Signed-off-by: Ronald S. Bultje <rsbul...@gmail.com>
---
 libavcodec/aarch64/h264idct_neon.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/aarch64/h264idct_neon.S 
b/libavcodec/aarch64/h264idct_neon.S
index fa414f7..e9f5b18 100644
--- a/libavcodec/aarch64/h264idct_neon.S
+++ b/libavcodec/aarch64/h264idct_neon.S
@@ -162,7 +162,7 @@ function ff_h264_idct_add8_neon, export=1
 mov w19, w3 // stride
 movrel  x13, X(ff_h264_idct_dc_add_neon)
 movrel  x14, X(ff_h264_idct_add_neon)
-movrel  x7,  scan8+16
+movrel  x7,  scan8, 16
 mov x10, #0
 mov x11, #16
 1:  mov w2,  w19
-- 
2.8.1

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

Re: [FFmpeg-devel] [PATCH] remove news entry about ffserver

[Ronald S. Bultje]

Hi,

On Thu, Dec 8, 2016 at 10:04 AM, Carl Eugen Hoyos <ceffm...@gmail.com>
wrote:

> 2016-12-08 16:02 GMT+01:00 Ronald S. Bultje <rsbul...@gmail.com>:
> > Hi,
> >
> > On Thu, Dec 8, 2016 at 8:42 AM, Carl Eugen Hoyos <ceffm...@gmail.com>
> wrote:
> >
> >> 2016-12-08 14:36 GMT+01:00 compn <te...@mi.rr.com>:
> >> > yeah.
> >>
> >> Instead I would suggest to clarify / correct the entry or write a new
> one.
> >
> >
> > I agree with CE here, let's not rewrite history.
> >
> > My reading of the vote is that we, as a project, are now promising to
> > update, maintain and improve ffserver
>
> No.
>
> We will try to do so.


Uh, right, that's probably better, yes.

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

Re: [FFmpeg-devel] [PATCH] remove news entry about ffserver

[Ronald S. Bultje]

Hi,

On Thu, Dec 8, 2016 at 8:42 AM, Carl Eugen Hoyos  wrote:

> 2016-12-08 14:36 GMT+01:00 compn :
> > yeah.
>
> Instead I would suggest to clarify / correct the entry or write a new one.


I agree with CE here, let's not rewrite history.

My reading of the vote is that we, as a project, are now promising to
update, maintain and improve ffserver and that if this succeeds (see
Nicolas' original post for official definition of this), ffserver can stay.

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

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

[Ronald S. Bultje]

Hi,

On Wed, Dec 7, 2016 at 1:29 PM, Nicolas George  wrote:

> keepAndreas Cadhalpun
> keepMarton Balint
> keepMichael Niedermayer (slightly invalid)
> keepNicolas George
> keepReynaldo H. Verdejo Pinochet (slightly invalid)


I believe you missed Carl Eugen's vote (or at least I read it as a vote):
http://ffmpeg.org/pipermail/ffmpeg-devel/2016-December/203915.html

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

Re: [FFmpeg-devel] [PATCH 0/4] More H.264 assembly (the sequel) [version 2]

[Ronald S. Bultje]

Hi,

On Tue, Dec 6, 2016 at 7:04 AM, James Darnley  wrote:

> On 2016-12-05 19:32, James Darnley wrote:
> > Fixed the problem Michael highlighted.  Dropped the intra functions
> until it
> > becomes clear why their performance is unexpected. Updated the
> benchmarks with
> > results from a Nehalem and used (slightly) more accurate data.
> >
> > Regarding the age of MMX:  I have written it so unless someone tells me
> to
> > remove it I will keep the code.  However, I will probably not write any
> more
> > going forward.


The "age" of mmx has been brought up for a while now, but latest intel CPUs
still support it, so I agree it's fine for now.

If nobody raises objections or makes further comments I will push these,
> including the AVX, later today.


OK.

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

[FFmpeg-devel] [PATCH] checkasm/vp9: benchmark all sub-IDCTs (but not WHT or ADST).

[Ronald S. Bultje]

---
 tests/checkasm/vp9dsp.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/tests/checkasm/vp9dsp.c b/tests/checkasm/vp9dsp.c
index 441041c..f32b97c 100644
--- a/tests/checkasm/vp9dsp.c
+++ b/tests/checkasm/vp9dsp.c
@@ -331,15 +331,20 @@ static void check_itxfm(void)
 int n_txtps = tx < TX_32X32 ? N_TXFM_TYPES : 1;
 
 for (txtp = 0; txtp < n_txtps; txtp++) {
-if (check_func(dsp.itxfm_add[tx][txtp], 
"vp9_inv_%s_%dx%d_add_%d",
-   tx == 4 ? "wht_wht" : txtp_types[txtp], sz, sz,
-   bit_depth)) {
-randomize_buffers();
-ftx(coef, tx, txtp, sz, bit_depth);
-
-for (sub = (txtp == 0) ? 1 : 2; sub <= sz; sub <<= 1) {
+// skip testing sub-IDCTs for WHT or ADST since they don't
+// implement it in any of the SIMD functions. If they do,
+// consider changing this to ensure we have complete test
+// coverage
+for (sub = (txtp == 0 && tx < 4) ? 1 : sz; sub <= sz; sub <<= 
1) {
+if (check_func(dsp.itxfm_add[tx][txtp],
+   "vp9_inv_%s_%dx%d_sub%d_add_%d",
+   tx == 4 ? "wht_wht" : txtp_types[txtp],
+   sz, sz, sub, bit_depth)) {
 int eob;
 
+randomize_buffers();
+ftx(coef, tx, txtp, sz, bit_depth);
+
 if (sub < sz) {
 eob = copy_subcoefs(subcoef0, coef, tx, txtp,
 sz, sub, bit_depth);
@@ -357,8 +362,9 @@ static void check_itxfm(void)
 !iszero(subcoef0, sz * sz * SIZEOF_COEF) ||
 !iszero(subcoef1, sz * sz * SIZEOF_COEF))
 fail();
+
+bench_new(dst, sz * SIZEOF_PIXEL, coef, eob);
 }
-bench_new(dst, sz * SIZEOF_PIXEL, coef, sz * sz);
 }
 }
 }
-- 
2.8.1

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

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

[Ronald S. Bultje]

Hi,

On Mon, Dec 5, 2016 at 9:45 AM, Carl Eugen Hoyos  wrote:

> 2016-12-05 15:23 GMT+01:00 James Almer :
> > The technical reasons are there, described in the news entry you seem to
> > not want to read, or at least properly parse.
> > These past week however saw one developer working against the clock doing
> > what the actual people interested in ffserver should have done for the
> past
> > few months and even years. That is, he addressed most, but not all, of
> those
> > and other reasons.
>
> Which reasons were not adressed?


ffm is a pretty important one?

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

Re: [FFmpeg-devel] [PATCH] Save FFmpeg colorspace info in openh264 video files.

[Ronald S. Bultje]

Hi,

On Thu, Dec 1, 2016 at 6:35 PM, Carl Eugen Hoyos  wrote:

> 2016-12-01 22:26 GMT+01:00 Gregory J. Wolfe  >:
>
> > +switch (avctx->color_primaries) {
> > +case AVCOL_PRI_BT709:param.sSpatialLayers[0].uiColorPrimaries
> = CP_BT709; break;
> > +case AVCOL_PRI_UNSPECIFIED:  param.sSpatialLayers[0].uiColorPrimaries
> = CP_UNDEF; break;
>
> [ignore]
> Please align vertically.
> [/ignore]
> Is it possible to convince gmail to always use a fixed-width font?
> The patch looks ugly here on gmail but I suspect it was aligned nicely...


In settings, change "Default text style:" to "Fixed Width".

I don't believe there is a way to do this only for inline patches. I agree
it would be useful.

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

Re: [FFmpeg-devel] [PATCH 1/2] lavu: Add JEDEC P22 color primaries

[Ronald S. Bultje]

Hi,

On Wed, Nov 30, 2016 at 5:26 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> On 30.11.2016 22:55, Ronald S. Bultje wrote:
> > On Wed, Nov 30, 2016 at 4:51 PM, Andreas Cadhalpun <
> > andreas.cadhal...@googlemail.com> wrote:
> >
> >> On 30.11.2016 19:16, Vittorio Giovara wrote:
> >> You can't just add a gap like that.
> >> The current code assumes that the numbers are consecutive, like e.g. the
> >> naming of AVCOL_PRI_NB suggests.
> >
> >
> > No, we've had gaps in these before.
>
> In AVColorPrimaries?


I seemed to remember, looked it up and appeared to be wrong, so scrap that
:)

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

Re: [FFmpeg-devel] [PATCH 3/3] avcodec/h264: sse2 and avx 4:2:2 idct add8 10-bit functions

[Ronald S. Bultje]

Hi,

On Wed, Nov 30, 2016 at 7:10 AM, James Darnley  wrote:

> On 2016-11-29 21:09, Carl Eugen Hoyos wrote:
> > 2016-11-29 17:14 GMT+01:00 James Darnley :
> >> On 2016-11-29 15:30, Carl Eugen Hoyos wrote:
> >>> 2016-11-29 12:52 GMT+01:00 James Darnley :
>  sse2:
>  complex: 4.13x faster (1514 vs. 367 cycles)
>  simple:  4.38x faster (1836 vs. 419 cycles)
> 
>  avx:
>  complex: 1.07x faster (260 vs. 244 cycles)
>  simple:  1.03x faster (284 vs. 274 cycles)
> >>>
> >>> What are you comparing?
> >
> >> The AVX comparison is it versus SSE2.
> >
> > This wasn't obvious to me.
>
> I've made it more verbose but I'm not sure whether it is any better.
> Care to give your opinion Carl?
>
> > Nehalem:
> >  - sse2:
> >- complex: 4.13x faster (1514 vs. 367 cycles)
> >- simple:  4.38x faster (1836 vs. 419 cycles)
> >
> > Haswell:
> >  - sse2:
> >- complex: 3.61x faster ( 936 vs. 260 cycles)
> >- simple:  3.97x faster (1126 vs. 284 cycles)
> >  - avx (versus sse2):
> >- complex: 1.07x faster (260 vs. 244 cycles)
> >- simple:  1.03x faster (284 vs. 274 cycles)
>
> I included the sse2 results for the Haswell to show that the avx is
> (slightly) better.


Ah! Now it makes sense. I had no idea why your SSE2 results changed from
367 (SSE2 vs. C) to 260 cycles (AVX vs. SSE2).

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

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

[Ronald S. Bultje]

Hi Stefano,

On Tue, Nov 29, 2016 at 1:46 PM, Stefano Sabatini 
wrote:

> On date Monday 2016-11-28 19:15:28 +0100, Nicolas George encoded:
> > Deadline: 2016-12-06 00:00 UTC.
> >
> > I propose, and put to the discussion, that the decision to drop ffserver
> > is revoked, conditioned to the fixing of the technical issues that lead
> > to it.
> >
> > In other words, if the technical problems that require dropping ffserver
> > are resolved at the time it is about to be dropped, then it must not be
> > and the patch is not applied.
>
> Someone please specify who is entitled to vote (such decisions are not
> so common, and the voting criteria were defined a long time ago IIRC).


https://ffmpeg.org/pipermail/ffmpeg-devel/2015-November/183803.html

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

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

[Ronald S. Bultje]

Hi,

On Mon, Nov 28, 2016 at 1:15 PM, Nicolas George  wrote:

> Deadline: 2016-12-06 00:00 UTC.
>
> I propose, and put to the discussion, that the decision to drop ffserver
> is revoked, conditioned to the fixing of the technical issues that lead
> to it.
>
> In other words, if the technical problems that require dropping ffserver
> are resolved at the time it is about to be dropped, then it must not be
> and the patch is not applied.
>
> I support the decision.


I vote to continue to drop ffserver.

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

Re: [FFmpeg-devel] [PATCH] lavc/vaapi-vp9: add support for profile 2 (bpp > 8)

[Ronald S. Bultje]

Hi,

On Mon, Nov 28, 2016 at 7:26 PM, Mark Thompson  wrote:

> On 28/11/16 21:22, Mathieu Velten wrote:
> > ---
> >  libavcodec/vaapi_vp9.c |  1 +
> >  libavcodec/vp9.c   | 32 +---
> >  libavcodec/vp9.h   |  1 +
> >  3 files changed, 19 insertions(+), 15 deletions(-)
>
> Nice :)
>
> Tested on Kaby Lake, works for me (woo 180fps 4K 10-bit decode).
>
> This should probably be split into two patches, though - one for the
> generic vp9 hwaccel support, a second then enabling it for VAAPI.
>
> > diff --git a/libavcodec/vaapi_vp9.c b/libavcodec/vaapi_vp9.c
> > index b360dcb..9b3e81a 100644
> > --- a/libavcodec/vaapi_vp9.c
> > +++ b/libavcodec/vaapi_vp9.c
> > @@ -38,6 +38,7 @@ static void fill_picture_parameters(AVCodecContext
>  *avctx,
> >  pp->first_partition_size = h->h.compressed_header_size;
> >
> >  pp->profile = h->h.profile;
> > +pp->bit_depth = h->h.bpp;
> >
> >  pp->filter_level = h->h.filter.level;
> >  pp->sharpness_level = h->h.filter.sharpness;
> > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> > index 0ec895a..ff526da 100644
> > --- a/libavcodec/vp9.c
> > +++ b/libavcodec/vp9.c
> > @@ -68,7 +68,7 @@ typedef struct VP9Context {
> >  ptrdiff_t y_stride, uv_stride;
> >
> >  uint8_t ss_h, ss_v;
> > -uint8_t last_bpp, bpp, bpp_index, bytesperpixel;
> > +uint8_t last_bpp, bpp_index, bytesperpixel;
> >  uint8_t last_keyframe;
> >  // sb_cols/rows, rows/cols and last_fmt are used for allocating all
> internal
> >  // arrays, and are thus per-thread. w/h and gf_fmt are synced
> between threads
> > @@ -258,7 +258,9 @@ static int update_size(AVCodecContext *ctx, int w,
> int h)
> >  if ((res = ff_set_dimensions(ctx, w, h)) < 0)
> >  return res;
> >
> > -if (s->pix_fmt == AV_PIX_FMT_YUV420P) {
> > +if (s->pix_fmt == AV_PIX_FMT_YUV420P ||
> > +s->pix_fmt == AV_PIX_FMT_YUV420P10 ||
> > +s->pix_fmt == AV_PIX_FMT_YUV420P12) {
> >  #if CONFIG_VP9_DXVA2_HWACCEL
> >  *fmtp++ = AV_PIX_FMT_DXVA2_VLD;
> >  #endif
>
> This is enabling it for DXVA2 and D3D11VA as well?  I'm guessing you
> probably didn't want to do that - I think it would be better with something
> more like  libavcodec/hevc.c;hb=HEAD#l350>.


I'll let you guys figure out the details for this, but generic vp9.[ch]
changes are OK with me.

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

Re: [FFmpeg-devel] [PATCH] Remove the ffserver program and the ffm muxer/demuxer

[Ronald S. Bultje]

Hi,

On Mon, Nov 28, 2016 at 10:25 AM, Nicolas George <geo...@nsup.org> wrote:

> L'octidi 8 frimaire, an CCXXV, Ronald S. Bultje a écrit :
> > The majority.
>
> Rational arguments first.


To what end? Aren't we dug in, don't we need a decision and follow through
with the elected outcome?

This has been going on for months. We need to move on, in some direction,
otherwise we'll keep talking about this for months to come.

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

Re: [FFmpeg-devel] [PATCH] Remove the ffserver program and the ffm muxer/demuxer

[Ronald S. Bultje]

Hi,

On Mon, Nov 28, 2016 at 10:17 AM, Nicolas George  wrote:

> L'octidi 8 frimaire, an CCXXV, compn a écrit :
> > these developers feel very strongly about removing ffserver.
>
> I feel very strongly about keeping ffserver. Who is right?


The majority. OK, so this is going nowhere. Vote, everyone? We need to
settle this, it's getting ridiculous.

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

Re: [FFmpeg-devel] [PATCH] ffserver: Remove last use of AVStream size

[Ronald S. Bultje]

Hi,

On Sun, Nov 27, 2016 at 1:26 PM, Michael Niedermayer  wrote:

> Signed-off-by: Michael Niedermayer 
> ---
>  ffserver.c | 18 --
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/ffserver.c b/ffserver.c
> index ded5149..9b1f6d5 100644
> --- a/ffserver.c
> +++ b/ffserver.c
> @@ -2961,7 +2961,6 @@ static int prepare_sdp_description(FFServerStream
> *stream, uint8_t **pbuffer,
> struct in_addr my_ip)
>  {
>  AVFormatContext *avc;
> -AVStream *avs = NULL;
>  AVOutputFormat *rtp_format = av_guess_format("rtp", NULL, NULL);
>  AVDictionaryEntry *entry = av_dict_get(stream->metadata, "title",
> NULL, 0);
>  int i;
> @@ -2975,7 +2974,6 @@ static int prepare_sdp_description(FFServerStream
> *stream, uint8_t **pbuffer,
>  avc->oformat = rtp_format;
>  av_dict_set(>metadata, "title",
>  entry ? entry->value : "No Title", 0);
> -avc->nb_streams = stream->nb_streams;
>  if (stream->is_multicast) {
>  snprintf(avc->filename, 1024, "rtp://%s:%d?multicast=1?ttl=%d",
>   inet_ntoa(stream->multicast_ip),
> @@ -2983,19 +2981,12 @@ static int prepare_sdp_description(FFServerStream
> *stream, uint8_t **pbuffer,
>  } else
>  snprintf(avc->filename, 1024, "rtp://0.0.0.0");
>
> -avc->streams = av_malloc_array(avc->nb_streams,
> sizeof(*avc->streams));
> -if (!avc->streams)
> -goto sdp_done;
> -
> -avs = av_malloc_array(avc->nb_streams, sizeof(*avs));
> -if (!avs)
> -goto sdp_done;
> -
>  for(i = 0; i < stream->nb_streams; i++) {
> -avc->streams[i] = [i];
> -avc->streams[i]->codec = stream->streams[i]->codec;
> +AVStream *st = avformat_new_stream(avc, NULL);
> +if (!st)
> +goto sdp_done;
>  avcodec_parameters_from_context(stream->streams[i]->codecpar,
> stream->streams[i]->codec);
> -avc->streams[i]->codecpar = stream->streams[i]->codecpar;
> +unlayer_stream(st, stream->streams[i]);
>  }
>  #define PBUFFER_SIZE 2048
>  *pbuffer = av_mallocz(PBUFFER_SIZE);
> @@ -3007,7 +2998,6 @@ static int prepare_sdp_description(FFServerStream
> *stream, uint8_t **pbuffer,
>  av_freep(>streams);
>  av_dict_free(>metadata);
>  av_free(avc);
> -av_free(avs);
>
>  return *pbuffer ? strlen(*pbuffer) : AVERROR(ENOMEM);
>  }
> --
> 2.10.2


I think you're sending this to the wrong repository, ffserver is not part
of the ffmpeg tree anymore.

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

Re: [FFmpeg-devel] [PATCH] Remove the ffserver program and the ffm muxer/demuxer

[Ronald S. Bultje]

Hi,

On Sun, Nov 27, 2016 at 1:46 PM, Michael Niedermayer  wrote:

> On Sun, Nov 27, 2016 at 07:30:24PM +0100, Clément Bœsch wrote:
> > On Sun, Nov 27, 2016 at 06:49:35PM +0100, Michael Niedermayer wrote:
> > > On Sun, Nov 27, 2016 at 04:57:36PM +0100, Clément Bœsch wrote:
> > > > On Sun, Nov 27, 2016 at 04:25:18PM +0100, Michael Niedermayer wrote:
> > > > [...]
> > > > > ffserver had 14 commits to it in about the last month
> > > >
> > > > That's also pretty close to the number of commits in the last years.
> > > >
> > > > Good will in the last weeks is not enough of a technical
> > > > merit/justification to prevent the removal of junk code scheduled for
> > > > deletion for a long time now.
> > >
> > > why do you call it junk ?
> >
> > because it's highly dependent on internal stuff, very limited, completely
> > untested, unmaintained for several years but still contains a ton of
> code.
> >
> > > and the sheduling for deletion was conditional on it not being fixed
> > > IIRC.
> > > Why the hurry to remove it while people work on fixing it ?
> > > its not blocking anything ATM AFAIK
> >
> > There is no hurry, but piling up a bunch patches to fix small things just
> > to use it as an argument to say "hey look now it's maintained" in order
> to
> > save it from being killed is really annoying. The people interested in it
> > had years to act.
> >
>
> > You can fix a ton of little things in it, but unless the fundamental
> > problems are addressed (ZERO internal API usage + at least partial FATE
> > coverage) it's pointless
>
> of course the goal is ZERO internal API usage + at least partial FATE
> coverage.
> Well in fact the lack of fate tests have been the primary reason
> why i didnt fix some of the API issues years ago. I felt uneasy
> changing it without regression tests
>
>
> > and will be seen as yet another case of "KEEP
> > EVERYTHING FOREVER" toxic mentality.
>
> The opposit is toxic too


I'm perfectly fine with keeping the code, just not in the ffmpeg tree.
Please move it to its own tree.

Everybody wants it out. Please follow majority.

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

Re: [FFmpeg-devel] [PATCH 2/2] ffserver: Remove extract_mpeg4_header()

[Ronald S. Bultje]

Hi,

On Wed, Nov 23, 2016 at 2:13 PM, Michael Niedermayer  wrote:

> This should not be needed, our AVParsers should do this
> I do not have a testcase though, please help testing this and please
> add fate tests if you can.


I thought we were going to remove ffserver?

Do I need to call for an official vote? What does it take for ffserver to
finally go away?

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

Re: [FFmpeg-devel] libavcodec : add psd image file decoder

[Ronald S. Bultje]

Hi,

On Mon, Nov 21, 2016 at 4:40 PM, Rostislav Pehlivanov 
wrote:

> On 21 November 2016 at 20:44, Martin Vignali 
> wrote:
>
> > Hello,
> >
> > New patchs in attach.
> > Correction have been made followings comments from Andreas and Carl.
> >
> > @Rotislav : thanks for your answer.
> >
> > Martin
> >
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> >
> I don't think you need the checks, it was pointed out that
> ff_set_dimensions already does that.
> Also IIRC lavc doesn't support widths or heights over 16384 anyway.
>
> >unsigned int pixel_size
> Please use uint32_t instead of unsigned ints throughout the patches.. I'm
> paranoid about unspecified sizes.


I don't think we typically encourage that, except for things that are
arrays...

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

Re: [FFmpeg-devel] [PATCH] avutil: add P016 pixel format

[Ronald S. Bultje]

Hi,

On Sun, Nov 20, 2016 at 4:59 PM, Philip Langdale  wrote:

> diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> index 96860ce..4cd3a77 100644
> --- a/libavutil/pixfmt.h
> +++ b/libavutil/pixfmt.h
> @@ -297,6 +297,8 @@ enum AVPixelFormat {
>
>  AV_PIX_FMT_P010LE, ///< like NV12, with 10bpp per component, data in
> the high bits, zeros in the low bits, little-endian
>  AV_PIX_FMT_P010BE, ///< like NV12, with 10bpp per component, data in
> the high bits, zeros in the low bits, big-endian
> +AV_PIX_FMT_P016LE, ///< like NV12, with 16bpp per component,
> little-endian
> +AV_PIX_FMT_P016BE, ///< like NV12, with 16bpp per component,
> big-endian
>
>  AV_PIX_FMT_GBRAP12BE,  ///< planar GBR 4:4:4:4 48bpp, big-endian
>  AV_PIX_FMT_GBRAP12LE,  ///< planar GBR 4:4:4:4 48bpp, little-endian


Doesn't this break ABI?

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

Re: [FFmpeg-devel] [PATCH 2/3] avutil/opt: Add AV_OPT_TYPE_UINT64

[Ronald S. Bultje]

Hi,

On Sun, Nov 20, 2016 at 9:16 AM, Michael Niedermayer <mich...@niedermayer.cc
> wrote:

> On Sun, Nov 20, 2016 at 08:56:15AM -0500, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Sun, Nov 20, 2016 at 6:57 AM, Michael Niedermayer
> <mich...@niedermayer.cc
> > > wrote:
> >
> > > @@ -131,6 +132,20 @@ static int write_number(void *obj, const AVOption
> *o,
> > > void *dst, double num, int
> > >  if (intnum == 1 && d == (double)INT64_MAX) *(int64_t *)dst =
> > > INT64_MAX;
> > >  else   *(int64_t *)dst =
> > > llrint(d) * intnum;
> > >  break;}
> > > +case AV_OPT_TYPE_UINT64:{
> > > +double d = num / den;
> > > +// We must special case uint64_t here as llrint() does not
> > > support values
> > > +// outside the int64_t range and there is no portable function
> > > which does
> > > +// "INT64_MAX + 1ULL" is used as it is representable exactly
> as
> > > IEEE double
> > > +// while INT64_MAX is not
> > > +if (intnum == 1 && d == (double)UINT64_MAX) {
> > > +*(int64_t *)dst = UINT64_MAX;
> > > +} else if (o->max > INT64_MAX + 1ULL && d > INT64_MAX + 1ULL)
> {
> > > +*(uint64_t *)dst = (llrint(d - (INT64_MAX + 1ULL)) +
> > > (INT64_MAX + 1ULL))*intnum;
> > > +} else {
> > > +*(int64_t *)dst = llrint(d) * intnum;
> > > +}
> > > +break;}
> > >  case AV_OPT_TYPE_FLOAT:
> > >  *(float *)dst = num * intnum / den;
> > >  break;
> >
> >
> > For the stupid, like me: what does this do? More specifically, this seems
> > an integer codepath, but there is a double in there. Why?
>
> write_number() has a num/den double argument. If this is used to
> set a uint64_t to UINT64_MAX things fail as double cannot exactly
> represent UINT64_MAX.  (the closest it can represent is "UINT64_MAX + 1"
> So it needs to be handled as a special case. Otherwise it turns into 0


This sounds incredibly shaky. Is write_number() so fringe that we don't
need an integer codepath?

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

Re: [FFmpeg-devel] [PATCH 2/3] avutil/opt: Add AV_OPT_TYPE_UINT64

[Ronald S. Bultje]

Hi,

On Sun, Nov 20, 2016 at 6:57 AM, Michael Niedermayer  wrote:

> @@ -131,6 +132,20 @@ static int write_number(void *obj, const AVOption *o,
> void *dst, double num, int
>  if (intnum == 1 && d == (double)INT64_MAX) *(int64_t *)dst =
> INT64_MAX;
>  else   *(int64_t *)dst =
> llrint(d) * intnum;
>  break;}
> +case AV_OPT_TYPE_UINT64:{
> +double d = num / den;
> +// We must special case uint64_t here as llrint() does not
> support values
> +// outside the int64_t range and there is no portable function
> which does
> +// "INT64_MAX + 1ULL" is used as it is representable exactly as
> IEEE double
> +// while INT64_MAX is not
> +if (intnum == 1 && d == (double)UINT64_MAX) {
> +*(int64_t *)dst = UINT64_MAX;
> +} else if (o->max > INT64_MAX + 1ULL && d > INT64_MAX + 1ULL) {
> +*(uint64_t *)dst = (llrint(d - (INT64_MAX + 1ULL)) +
> (INT64_MAX + 1ULL))*intnum;
> +} else {
> +*(int64_t *)dst = llrint(d) * intnum;
> +}
> +break;}
>  case AV_OPT_TYPE_FLOAT:
>  *(float *)dst = num * intnum / den;
>  break;


For the stupid, like me: what does this do? More specifically, this seems
an integer codepath, but there is a double in there. Why?

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

Re: [FFmpeg-devel] [PATCH] configure: reserve a register for gcc before 5 on x86_32 with PIE

[Ronald S. Bultje]

Hi,

On Sat, Nov 19, 2016 at 7:07 PM, Andreas Cadhalpun <
andreas.cadhal...@googlemail.com> wrote:

> gcc before gcc-5 reserves a register on x86_32 for the GOT, when PIE is
> enabled.
>
> This fixes build failures due to:
> error: 'asm' operand has impossible constraints
>
> Signed-off-by: Andreas Cadhalpun 
> ---
>
> A build log of a failed build with gcc 4.9 is available at:
> https://buildd.debian.org/status/fetch.php?pkg=ffmpeg;
> arch=i386=7%3A3.2-2~bpo8%2B1=1478791165
>
> ---
>  configure | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/configure b/configure
> index b5bfad6..8794580 100755
> --- a/configure
> +++ b/configure
> @@ -5364,6 +5364,19 @@ EOF
>  enabled ssse3  && check_inline_asm ssse3_inline  '"pabsw %xmm0,
> %xmm0"'
>  enabled mmxext && check_inline_asm mmxext_inline '"pmaxub %mm0, %mm1"'
>
> +case "$toolchain" in
> +hardened)
> +if enabled x86_32; then
> +# When PIE is enabled on x86_32, gcc before gcc-5
> reserves a register for the GOT.
> +case $gcc_basever in
> +2*|3*|4*)
> +disable ebp_available
> +;;
> +esac
> +fi
> +;;
> +esac


This doesn't test whether PIE is enabled, it does it unconditionally. I'm
almost 100% sure that's not necessary.

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

Re: [FFmpeg-devel] [PATCHv2] vf_colorspace: Forbid odd dimensions

[Ronald S. Bultje]

Hi,

On Thu, Nov 17, 2016 at 5:02 PM, Vittorio Giovara <
vittorio.giov...@gmail.com> wrote:

> On Thu, Nov 17, 2016 at 4:11 PM, Ronald S. Bultje <rsbul...@gmail.com>
> wrote:
> > Hi,
> >
> > On Thu, Nov 17, 2016 at 4:09 PM, Vittorio Giovara
> > <vittorio.giov...@gmail.com> wrote:
> >>
> >> This prevents writing past bounds.
> >>
> >> Signed-off-by: Vittorio Giovara <vittorio.giov...@gmail.com>
> >> ---
> >> Updated according review.
> >> Vittorio
> >>
> >>  doc/filters.texi| 1 +
> >>  libavfilter/vf_colorspace.c | 9 -
> >>  2 files changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/doc/filters.texi b/doc/filters.texi
> >> index 208be42..aee43a8 100644
> >> --- a/doc/filters.texi
> >> +++ b/doc/filters.texi
> >> @@ -5278,6 +5278,7 @@ colormatrix=bt601:smpte240m
> >>  @section colorspace
> >>
> >>  Convert colorspace, transfer characteristics or color primaries.
> >> +Input video needs to have an even size.
> >>
> >>  The filter accepts the following options:
> >>
> >> diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
> >> index c7a2286..0024505 100644
> >> --- a/libavfilter/vf_colorspace.c
> >> +++ b/libavfilter/vf_colorspace.c
> >> @@ -168,7 +168,7 @@ typedef struct ColorSpaceContext {
> >>  int did_warn_range;
> >>  } ColorSpaceContext;
> >>
> >> -// FIXME deal with odd width/heights (or just forbid it)
> >> +// FIXME deal with odd width/heights
> >>  // FIXME faster linearize/delinearize implementation (integer pow)
> >>  // FIXME bt2020cl support (linearization between yuv/rgb step instead
> of
> >> between rgb/xyz)
> >>  // FIXME test that the values in (de)lin_lut don't exceed their
> container
> >> storage
> >> @@ -1031,8 +1031,15 @@ static int query_formats(AVFilterContext *ctx)
> >>
> >>  static int config_props(AVFilterLink *outlink)
> >>  {
> >> +AVFilterContext *ctx = outlink->dst;
> >>  AVFilterLink *inlink = outlink->src->inputs[0];
> >>
> >> +if (inlink->w % 2 || inlink->h % 2) {
> >> +av_log(ctx, AV_LOG_ERROR, "Invalid odd size (%dx%d)\n",
> >> +   inlink->w, inlink->h);
> >> +return AVERROR_PATCHWELCOME;
> >> +}
> >
> >
> > If 422 && w%2 or 420 && (w%2||h%2), or some variant thereof. Odd sizes
> for
> > 444 or odd vertical sizes for 422 should be OK, right?
>
> well in theory, but i actually got a segfault on 422
>
> ==4064== Invalid write of size 2
> ==4064==at 0x4ED6637: yuv2rgb_422p10_c (colorspacedsp_template.c:89)
> ==4064==by 0x4F0444C: convert (vf_colorspace.c:504)
>
> and on 444 (with testsrc).


Well that's embarrassing. OK, I guess patch should go in then. I'll fix
that later if anyone actually cares about odd resolutions...

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

Re: [FFmpeg-devel] [RFC]lavc/ffv1dec: Scale msb-packed output to full 16bit

[Ronald S. Bultje]

Hi,

On Thu, Nov 17, 2016 at 4:11 PM, Michael Niedermayer  wrote:

> On Thu, Nov 17, 2016 at 09:13:55PM +0100, Carl Eugen Hoyos wrote:
> > 2016-11-17 14:49 GMT+01:00 Rostislav Pehlivanov :
> > > On 16 November 2016 at 11:15, Carl Eugen Hoyos 
> wrote:
> > >
> > >> Hi!
> > >>
> > >> Attached patch improves output for some ffv1 files imo.
> > >> Current slowdown for the existing decode-line timer is
> > >> 2%, I wonder if this can be improved through refactoring.
> > >>
> > >> Please comment, Carl Eugen
> > >>
> > >> ___
> > >> ffmpeg-devel mailing list
> > >> ffmpeg-devel@ffmpeg.org
> > >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >>
> > >>
> > > So AFAIK the encoder pushes the values to the LSBs but the decoder
> didn't
> > > shift them back up?
> >
> > I don't think the encoder does any shifts here but I may misunderstand.
> >
> > > I think you should add a comment explaining that happens.
> >
> > Many (older) decoders have to do this and there is nowhere a
> > comment, I really believe that this is not particularly convoluted
> > code.
> >
> > > Also 2% on a decoder doesn't sound that great,
> >
> > It's 2% in a function of a decoder.
> >
> > > did you try using an if case for the entire loop for when the
> > > values need to be shifted?
> >
> > That is what I tried to suggest with "refactoring", I suspect
> > Michael wasn't too happy about the idea.
>
> can the whole "what to put in the lsb" question be avoided by adding
> gray10 support to ffv1dec ?
> if so this might be the best solution


I believe that would also resolve the issue, yes...

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

Re: [FFmpeg-devel] [PATCHv2] vf_colorspace: Forbid odd dimensions

[Ronald S. Bultje]

Hi,

On Thu, Nov 17, 2016 at 4:09 PM, Vittorio Giovara <
vittorio.giov...@gmail.com> wrote:

> This prevents writing past bounds.
>
> Signed-off-by: Vittorio Giovara 
> ---
> Updated according review.
> Vittorio
>
>  doc/filters.texi| 1 +
>  libavfilter/vf_colorspace.c | 9 -
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 208be42..aee43a8 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -5278,6 +5278,7 @@ colormatrix=bt601:smpte240m
>  @section colorspace
>
>  Convert colorspace, transfer characteristics or color primaries.
> +Input video needs to have an even size.
>
>  The filter accepts the following options:
>
> diff --git a/libavfilter/vf_colorspace.c b/libavfilter/vf_colorspace.c
> index c7a2286..0024505 100644
> --- a/libavfilter/vf_colorspace.c
> +++ b/libavfilter/vf_colorspace.c
> @@ -168,7 +168,7 @@ typedef struct ColorSpaceContext {
>  int did_warn_range;
>  } ColorSpaceContext;
>
> -// FIXME deal with odd width/heights (or just forbid it)
> +// FIXME deal with odd width/heights
>  // FIXME faster linearize/delinearize implementation (integer pow)
>  // FIXME bt2020cl support (linearization between yuv/rgb step instead of
> between rgb/xyz)
>  // FIXME test that the values in (de)lin_lut don't exceed their container
> storage
> @@ -1031,8 +1031,15 @@ static int query_formats(AVFilterContext *ctx)
>
>  static int config_props(AVFilterLink *outlink)
>  {
> +AVFilterContext *ctx = outlink->dst;
>  AVFilterLink *inlink = outlink->src->inputs[0];
>
> +if (inlink->w % 2 || inlink->h % 2) {
> +av_log(ctx, AV_LOG_ERROR, "Invalid odd size (%dx%d)\n",
> +   inlink->w, inlink->h);
> +return AVERROR_PATCHWELCOME;
> +}
>

If 422 && w%2 or 420 && (w%2||h%2), or some variant thereof. Odd sizes for
444 or odd vertical sizes for 422 should be OK, right?

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

Re: [FFmpeg-devel] [PATCH] vf_colorspace: Forbid odd dimensions

[Ronald S. Bultje]

Hi,

On Thu, Nov 17, 2016 at 3:53 PM, Vittorio Giovara <
vittorio.giov...@gmail.com> wrote:

> This prevents writing past bounds.
>
> Signed-off-by: Vittorio Giovara 
> ---
> Any opinions about this?


Hm... I agree right now it's probably broken, but I'd argue that we should
fix it, not forbid it.

That may be out of scope for you, but then at least don't remove the FIXME
in the TODO list, and output AVERROR_PATCHWELCOME?

But maybe I'm thinking about this too much/idealistically...

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

Re: [FFmpeg-devel] [RFC]lavc/ffv1dec: Scale msb-packed output to full 16bit

[Ronald S. Bultje]

Hi Carl,

On Thu, Nov 17, 2016 at 3:10 PM, Carl Eugen Hoyos <ceffm...@gmail.com>
wrote:

> 2016-11-17 19:34 GMT+01:00 Ronald S. Bultje <rsbul...@gmail.com>:
> > Carl, the reason the patch is wrong is that luma range does not scale up
> > from 16< > This is documented in numerous places, see e.g.
> > https://msdn.microsoft.com/en-us/library/windows/desktop/
> bb970578(v=vs.85).aspx
> > :
> >
> > "For example, if the white point of an 8-bit format is 235, the
> > corresponding 10-bit format has a white point at 940 (235 × 4)."
>
> This is indeed very difficult to understand: How is this related?
>
> AV_PIX_FMT_GRAY was changed years ago after several users
> protested that it did conform to above specification, since it doesn't
> do now, the paragraph looks unrelated.
> (AV_PIX_FMT_GRAY is full-scale as is AV_PIX_FMT_GRAY16)
>
> More important:
> My patch is not related to 10-bit output format, so above calculations
> are also not related afaict.


The relevant field in the decoder is called bits_per_raw_sample. If this is
10, the format is 10-bit. The output format is therefore also 10-bit,
otherwise it's not lossless. How you represent that in an uint16_t is up to
you, obviously, but it's 10bit, right? So the patch relates to 10bit
formats.

The question then seems, what do you do with the ("filler") bits if we
shift them to MSB in gray16, i.e. packed_at_lsb=0? My assumption (from the
decision to represent the coding as gray16 while setting
bits_per_raw_sample) is that you want it to represent a value in MSB of
uint16_t that would be closest to what it would have been if the value was
actually coded as 16bit, right?

(I'm assuming the above is logical and we all agree on it, please let me
know if you disagree, otherwise let's go on to the apparently controversial
stuff.)

> Your patch is therefore theoretically wrong. The correct behaviour in
> > limited-range upscaling is indeed to leave the lower bits empty. For
> > full-range, the lower bits might be filled if the intention is for the
> > pixel value to be a representation of what the 16bit result would look
> > like, but whether this belongs in a decoder or not is up for discussion.
>
> Decoders tend to be used by video players and if white looks gray on a
> screen, it doesn't make much sense to say "the player should have
> worked-around this".


I'm fine with that, but it depends on the format. If the input was
full-range, then yes. If the input was not full-range, then no.

Assume I have a H264 file with a PPS range=limited and a chroma_idc=0
(4:0:0, i.e. grayscale). You agree from the H264 spec that this is a legal
file, right? I could also generate source data by taking any (10bit) Bt709
YUV file (which almost universally use limited-range coding) and stripping
the UV planes out and leaving the Y data untouched.

How do I represent this data losslessly in ffv1 after your patch? I don't
think I can, because a value of 0x3ac (235x4=940, i.e. white) would be
upshifted to (0x3ac << 6) || (0x3ac >> 4) = 0xeb3a, which converting back
(as e.g. swscale does) to 10bits is 940.9, so after rounding it would be
941. That is not 940, thus not lossless and therefore a bug (IMO).

I agree that if the data was fullrange, your patch may be correct (it
depends on some other stuff, but I don't want to get into the
he-said-she-said stuff, I'm describing a bug here), but your patch breaks
something that worked previously.

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

Re: [FFmpeg-devel] [RFC]lavc/ffv1dec: Scale msb-packed output to full 16bit

[Ronald S. Bultje]

Hi,

On Thu, Nov 17, 2016 at 1:06 PM, Kieran Kunhya  wrote:

> On Thu, 17 Nov 2016 at 18:05 Kieran Kunhya  wrote:
>
> > On Wed, 16 Nov 2016 at 23:49 Kieran Kunhya  wrote:
> >
> > It was lossless before and is lossless after the patch but it
> > output the 16bit format that you don't like and of which you
> > claim it breaks with find_best_pix_fmt() - I still believe this
> > should be trivial to fix.
> >
> > Currently white is gray, the patch tries to fix this.
> >
> >
> > erm...in what way does it fix that?
> > A white/black value in 16-235 needs to remain like that when shifted.
> This
> > isn't RGB-land.
> >
> >
> > This incorrect patch has been committed in spite of clear objections:
> >
> >
> > https://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=
> 55a424c5a836523828b3b2f02b7db610e898b3fc
> >
> > Regards,
> > Kieran Kunhya
> >
>
> Furthermore I will be reverting this patch in two hours.


There's no need to wait two hours, the patch is fundamentally wrong.

Carl, the reason the patch is wrong is that luma range does not scale up
from 16

Re: [FFmpeg-devel] [PATCH] doc/filters: adds recently added -vf colorspace options

[Ronald S. Bultje]

Hi Kieran,

On Tue, Nov 15, 2016 at 7:18 PM, Kieran O Leary 
wrote:

> From: kieranjol 


Is your name spelled like this intentionally? Maybe change your git config
so this has your full name instead of what appears to be your login?

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

Re: [FFmpeg-devel] [PATCH 1/9] vp9dsp: Deduplicate the subpel filters

[Ronald S. Bultje]

Hi,

On Mon, Nov 14, 2016 at 9:29 AM, Michael Niedermayer  wrote:

> On Mon, Nov 14, 2016 at 12:32:19PM +0200, Martin Storsjö wrote:
> > Make them aligned, to allow efficient access to them from simd.
> >
> > This is an adapted cherry-pick from libav commit
> > a4cfcddcb0f76e837d5abc06840c2b26c0e8aefc.
> > ---
> >  libavcodec/vp9dsp.c  | 56 ++
> +
> >  libavcodec/vp9dsp.h  |  3 +++
> >  libavcodec/vp9dsp_template.c | 63 +++---
> --
> >  3 files changed, 63 insertions(+), 59 deletions(-)
>
> patchset tested with fate on arm-qemu, all fate tests pass


Thanks for testing, and thanks for the patchset.

Pushed.

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

Re: [FFmpeg-devel] [PATCH] vp9: add avx2 iadst16 implementations.

[Ronald S. Bultje]

Hi,

On Mon, Nov 14, 2016 at 4:26 PM, James Almer <jamr...@gmail.com> wrote:

> On 11/8/2016 1:22 PM, Ronald S. Bultje wrote:
> > Also a small cosmetic change to the avx2 idct16 version to make it
> > explicit that one of the arguments to the write-out macros is unused
> > for >=avx2 (it uses pmovzxbw instead of punpcklbw).
>
> A braindead test (ffmpeg -i 4kHDRsample.webm -benchmark -f null -) on an i5
> Haswell went from
>
> frame= 2000 fps= 73 q=-0.0 Lsize=N/A time=00:00:33.36 bitrate=N/A
> speed=1.21x
> bench: utime=92.250s
>
> To
>
> frame= 2000 fps= 77 q=-0.0 Lsize=N/A time=00:00:33.36 bitrate=N/A
> speed=1.28x
> bench: utime=86.891s
>
> In comparison, a 1080p version of the same video now reaches ~360fps.
>
> FATE passes, so LGTM (After the x86_32 fix).


Sorry, lost track a bit, pushed.

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

Re: [FFmpeg-devel] [PATCH 1/9] vp9dsp: Deduplicate the subpel filters

[Ronald S. Bultje]

Hi,

On Mon, Nov 14, 2016 at 5:32 AM, Martin Storsjö  wrote:

> Make them aligned, to allow efficient access to them from simd.
>
> This is an adapted cherry-pick from libav commit
> a4cfcddcb0f76e837d5abc06840c2b26c0e8aefc.
> ---
>  libavcodec/vp9dsp.c  | 56 +++
>  libavcodec/vp9dsp.h  |  3 +++
>  libavcodec/vp9dsp_template.c | 63 +++---
> --
>  3 files changed, 63 insertions(+), 59 deletions(-)


OK.

Do I need to queue them up? I thought they would be merged automagically
from Libav...

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