Re: [FFmpeg-devel] [PATCH 2/3] avformat/dashenc: opening a segment file when its first frame is ready

2018-02-24 Thread Jeyapal, Karthick 


On 2/24/18 9:11 PM, James Almer wrote:
> On 2/24/2018 12:19 PM, Marton Balint wrote:
>>
>>
>> On Mon, 19 Feb 2018, vdi...@akamai.com wrote:
>>
>>> From: Vishwanath Dixit 
>>>
>>> ---
>>> libavformat/dashenc.c | 57
>>> ---
>>> 1 file changed, 36 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>>> index 0f6f4f2..0eb4b25 100644
>>> --- a/libavformat/dashenc.c
>>> +++ b/libavformat/dashenc.c
>>> @@ -81,6 +81,9 @@ typedef struct OutputStream {
>>> char bandwidth_str[64];
>>>
>>> char codec_str[100];
>>> +char filename[1024];
>>> +char full_path[1024];
>>> +char temp_path[1024];
>>> } OutputStream;
>>
>> I know it's late, but in the future please work toward supporting
>> unlimited path lengths, that was the whole point of the deprecation of
>> AVFormatContext->filename.
Thanks for pointing it out. Sure, we will fix it. We had done these long 
back(atleast 6 months back), when filename was still in active use.
Only now we had the time to merge these changes with the latest ffmpeg.
>>
>> Thanks,
>> Marton
>
> It's not late, it can and should be changed. New code using deprecated
> APIs that will require changes in the long run should have not been
> added, so better change it now.
Just to clarify here, we are not using any deprecated API in that patch. We are 
still using the url parameter.
In fact, yesterday I sent out a different patch to fix deprecated API usage 
that was previously present. 
http://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225739.html 
The only issue was that the size of filename has been limited to 1024 
internally. This needs to be replaced by a dynamic size buffer.  
But I agree with you that it is better to change now rather than later. 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



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

Re: [FFmpeg-devel] Fix memset size on ctts_data in mov_read_trun()

2018-02-24 Thread Michael Niedermayer 
On Fri, Feb 23, 2018 at 05:12:05PM -0800, Xiaohan Wang (王消寒) wrote:
> Michael: Dale and I dig into history a bit more and we don't understand why
> the code is changed to the current state around memset. This new patch
> reverted the change back to the previous state which we felt is much
> cleaner. Please see the CL description for details and take a look at the
> new patch. Thanks!
> 
> On Wed, Feb 21, 2018 at 1:14 PM, Xiaohan Wang (王消寒) 
> wrote:
> 
> > jstebbins: kindly ping!
> >
> > On Fri, Feb 16, 2018 at 2:42 PM, Xiaohan Wang (王消寒) 
> > wrote:
> >
> >> +jstebbins@ who wrote that code.
> >>
> >> On Fri, Feb 16, 2018 at 12:30 PM, Michael Niedermayer <
> >> mich...@niedermayer.cc> wrote:
> >>
> >>> On Thu, Feb 15, 2018 at 12:10:33PM -0800, Xiaohan Wang (王消寒) wrote:
> >>> >
> >>>
> >>> >  mov.c |3 ++-
> >>> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >>> > 5597d0b095f8b15eb11503010a51c2bc2c022413
> >>> 0001-ffmpeg-Fix-memset-size-on-ctts_data-in-mov_read_trun.patch
> >>> > From 7c1e6b50ebe35b2a38c4f1d0a988e31eccbd0ead Mon Sep 17 00:00:00 2001
> >>> > From: Xiaohan Wang 
> >>> > Date: Thu, 15 Feb 2018 12:05:53 -0800
> >>> > Subject: [PATCH] ffmpeg: Fix memset size on ctts_data in
> >>> mov_read_trun()
> >>> >
> >>> > The allocated size of sc->ctts_data is
> >>> > (st->nb_index_entries + entries) * sizeof(*sc->ctts_data).
> >>> >
> >>> > The size to memset at offset sc->ctts_data + sc->ctts_count should be
> >>> > (st->nb_index_entries + entries - sc->ctts_count) *
> >>> sizeof(*sc->ctts_data))
> >>> >
> >>> > The current code missed |entries| I believe.
> >>>
> >>> shouldnt "entries" be read by this function later and so shouldnt need a
> >>> memset?
> >>> I didnt write this, but it looks a bit to me as if it was intended to
> >>> only
> >>> clear the area that would not be read later
> >>>
> >>
> >> I thought we only had sc->ctts_count entries before av_fast_realloc, so
> >> memset everything starting from sc->ctts_data + sc->ctts_count couldn't
> >> go wrong. But I am not familiar with this code and that could totally be
> >> wrong. I added jstebbins@ who wrote the code and hopefully we can get
> >> expert opinion there.
> >>
> >>
> >>> [...]
> >>> --
> >>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >>>
> >>> No great genius has ever existed without some touch of madness. --
> >>> Aristotle
> >>>
> >>> ___
> >>> ffmpeg-devel mailing list
> >>> ffmpeg-devel@ffmpeg.org
> >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>
> >>>
> >>
> >

>  mov.c |5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> e5bbe55f0b1260f787f431b5c45e6ca49a7d2d1e  
> 0001-Fix-memset-size-on-ctts_data-in-mov_read_trun-round-.patch
> From f34b35ec5749c17b21f80665a0b8859bce3e84ab Mon Sep 17 00:00:00 2001
> From: Xiaohan Wang 
> Date: Fri, 23 Feb 2018 10:51:30 -0800
> Subject: [PATCH] Fix memset size on ctts_data in mov_read_trun() (round 2)
> 
> The allocated size of sc->ctts_data is
> (st->nb_index_entries + entries) * sizeof(*sc->ctts_data).
> 
> The size to memset at offset sc->ctts_data + sc->ctts_count should be
> (st->nb_index_entries + entries - sc->ctts_count) *
> sizeof(*sc->ctts_data))
> 
> The current code missed |entries| I believe, which was introduced in
> https://patchwork.ffmpeg.org/patch/5541/.
> 
> However, after offline discussion, it seems the original code is much
> more clear to read (before https://patchwork.ffmpeg.org/patch/5541/).
> 
> Hence this CL revert the memset logic to it's previous state by
> remembering the |old_ctts_allocated_size|, and only memset the newly
> allocated entries.
> ---
>  libavformat/mov.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index a3725692a7..f8d79c7b02 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4713,6 +4713,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  st->index_entries= new_entries;
>  
>  requested_size = (st->nb_index_entries + entries) * 
> sizeof(*sc->ctts_data);
> +unsigned int old_ctts_allocated_size = sc->ctts_allocated_size;

this should possibly be size_t 

and declaration and statements should not be mixed

libavformat/mov.c: In function ‘mov_read_trun’:
libavformat/mov.c:4691:5: warning: ISO C90 forbids mixed declarations and code 
[-Wdeclaration-after-statement]

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

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


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

Re: [FFmpeg-devel] [PATCH 5/9] sbc: implement SBC encoder (low-complexity subband codec)

2018-02-24 Thread Rostislav Pehlivanov 
On 24 February 2018 at 12:01, Aurelien Jacobs  wrote:

> On Thu, Feb 22, 2018 at 06:18:48PM +, Rostislav Pehlivanov wrote:
> > On 21 February 2018 at 22:37, Aurelien Jacobs  wrote:
> >
> > > This was originally based on libsbc, and was fully integrated into
> ffmpeg.
> > > ---
> > >  doc/general.texi |   2 +-
> > >  libavcodec/Makefile  |   1 +
> > >  libavcodec/allcodecs.c   |   1 +
> > >  libavcodec/sbcdsp.c  | 382 ++
> > > +
> > >  libavcodec/sbcdsp.h  |  83 ++
> > >  libavcodec/sbcdsp_data.c | 329 +
> > >  libavcodec/sbcdsp_data.h |  55 +++
> > >  libavcodec/sbcenc.c  | 411 ++
> > > +
> > >  8 files changed, 1263 insertions(+), 1 deletion(-)
> > >  create mode 100644 libavcodec/sbcdsp.c
> > >  create mode 100644 libavcodec/sbcdsp.h
> > >  create mode 100644 libavcodec/sbcdsp_data.c
> > >  create mode 100644 libavcodec/sbcdsp_data.h
> > >  create mode 100644 libavcodec/sbcenc.c
> > >
> > > +
> > > +#define OFFSET(x) offsetof(SBCEncContext, x)
> > > +#define AE AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
> > > +static const AVOption options[] = {
> > > +{ "joint_stereo", "use joint stereo",
> > > +  OFFSET(joint_stereo), AV_OPT_TYPE_BOOL, { .i64 =  0 }, 0,   1,
> AE },
> > >
> > +{ "dual_channel", "use dual channel",
> > > +  OFFSET(dual_channel), AV_OPT_TYPE_BOOL, { .i64 =  0 }, 0,   1,
> AE },
> > >
> >
> > Erm those 2 things should be decided by the encoder, not by exposing them
> > to the user. The encoder should decide which mode has lower distortion
> for
> > a given signal.
>
> See bellow.
>
> > > +{ "subbands", "number of subbands (4 or 8)",
> > > +  OFFSET(subbands), AV_OPT_TYPE_INT,  { .i64 =  8 }, 4,   8,
> AE },
> > >
> >
> > The encoder doesn't check if the value isn't 4 or 8 so 5, 6 and 7 are all
> > accepted. Similar issue to the previous option too.
>
> OK, fixed.
>
> > > +{ "bitpool",  "bitpool value",
> > > +  OFFSET(bitpool),  AV_OPT_TYPE_INT,  { .i64 = 32 }, 0, 255,
> AE },
> > >
> >
> > This should be controlled by the bitrate setting. Either have a function
> to
> > translate bitrate to bitpool value or a table which approximately maps
> > bitrate values supplied to bitpools. You could expose it directly as well
> > as mapping it to a bitrate value by using the global_quality setting so
> it
> > shouldn't be a custom encoder option.
>
> Indeed, this is better this way, thanks for the suggestion.
>
> > > +{ "blocks",   "number of blocks (4, 8, 12 or 16)",
> > > +  OFFSET(blocks),   AV_OPT_TYPE_INT,  { .i64 = 16 }, 4,  16,
> AE },
> > > +{ "snr",  "use SNR mode (instead of loudness)",
> > > +  OFFSET(allocation),   AV_OPT_TYPE_BOOL, { .i64 =  0 }, 0,   1,
> AE },
> > >
> >
> > SNR mode too needs to be decided by the encoder rather than exposing it
> as
> > a setting.
>
> See bellow.
>
> > > +{ "msbc", "use mSBC mode (wideband speech mono SBC)",
> > >
> >
> > Add a profile fallback setting for this as well, like in aac where
> -aac_ltp
> > turns LTP mode on and -profile:a aac_ltp does the same.
>
> Not sure of the benefits of having 2 redundant way to do this, but OK.
>
> > You don't have to make the encoder decide which stereo coupling mode or
> > snr/loudness setting to use, you can implement that with a later patch.
> > I think you should remove the "blocks" and "subbands" settings as well
> and
> > instead replace those with a single "latency" setting like the native
> Opus
> > encoder in milliseconds which would adjust both of them on init to set
> the
> > frame size. This would also allow the encoder to change them. Again, you
> > don't have to do this now, you can send a patch which adds a "latency"
> > option later.
>
> I can see the value in what you propose, and I think that indeed, it
> would be great for the encoder to choose by itself the most appropriate
> parameters by default.
> But on the other hand, we are talking about a codec targetting some
> hardware decoders (blutetooth speakers, headsets...), and all those
> parameters have to be negotiated between the encoder and the hardware
> decoder. So I think it is mandatory to keep all those parameters
> accessible to be able to setup the encoder according to the target
> hardware capabilities.
>
>
Hardware isn't as limited as you might think it is, and there's also no
negotiation happening between encoders and decoders IRL (except for modern
streaming protocols which suck). While its true that we've had some issues
with hardware decoders not supporting rarely used features we wait for
users to report them and don't assume that all hardware violates the specs.
And looking at the code, only 2 settings strike out as being able to cause
issues: the frame size, controlled by subbands and blocks and the msbc
mode. There's no way in

Re: [FFmpeg-devel] [PATCH]lavc/exr: Ignore long names flag

2018-02-24 Thread Martin Vignali 
>
> Hello,
>
> If no one is against, i will apply the patch in attach in few days (change
> since previous patch (mention ticket in commit msg))
>
> Martin
>
>
Pushed
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] avfilter/vf/blend : add 16 bits SIMD version for basic mode (v2)

2018-02-24 Thread Martin Vignali 
2018-02-17 21:51 GMT+01:00 Martin Vignali :

> Hello,
>
> New patchs in attach
> Limit the asm version to x86_64
>
> tested on osx X86_64 and osx X86_32
>
>
> Martin
>

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

Re: [FFmpeg-devel] fate/exr : add test for ticket 6994 (long name flag)

2018-02-24 Thread Martin Vignali 
2018-02-13 12:30 GMT+01:00 Martin Vignali :

>
>
> 2018-02-09 9:22 GMT+01:00 Martin Vignali :
>
>> Hello,
>>
>> Patch in attach add test for ticket 6994 (flag long name)
>>
>> Sample can be found here :
>> https://we.tl/WBnt10VSA1
>>
>> and need to be put inside ./fate-suite/exr
>>
>> Can be test with :
>> make fate-exr SAMPLES=fate-suite/
>>
>>
>> Need to be apply after one of the patch in discussion : "lavc/exr: Ignore
>> long names flag"
>>
>>
>>
>>
>
Sample upload some days ago
Pushed.

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

Re: [FFmpeg-devel] [PATCH 1/2] avformat/aviobuf: add ff_read_line_to_bprint and ff_read_line_to_bprint_overwrite functions

2018-02-24 Thread Marton Balint 


On Wed, 14 Feb 2018, Nicolas George wrote:


Marton Balint (2018-02-11):

It is a 500% speed improvement on a 260 MB line compared to using
av_bprint_chars, so I'd rather leave it as is. I can add a comment saying
"for performance reasons we fill a temporary buffer, and use av_bprint
functions on chunks of data".


This is assuming reading the text file is the only thing that happens.
In practice, the lines will be parsed, tokenized, with probably quite a
few mallocs, making the overhead negligible. And if performance were
really critic, reading the file whole and splitting in memory would
probably be best.

But of course, since the optimization is already written and gives a
significant difference on pure tests, I have no objection as is.


Thanks, applied the series.

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

Re: [FFmpeg-devel] [PATCH 2/3] Add muxer/demuxer for raw codec2 and .c2 files

2018-02-24 Thread Tomas Härdin 
ons 2018-02-14 klockan 10:20 +0100 skrev Tomas Härdin:
> tis 2018-02-13 klockan 11:48 +0100 skrev Tomas Härdin:
> > fre 2018-02-09 klockan 11:29 +0100 skrev Carl Eugen Hoyos:
> > > 2018-01-15 22:36 GMT+01:00 Tomas Härdin :
> > > 
> > > > > +if (p->buf[4] >  EXPECTED_CODEC2_MINOR_VERSION) score -=
> > > > > AVPROBE_SCORE_MAX/5;
> > > > > +if (p->buf[5] >  AVPRIV_CODEC2_MODE_MAX)score -=
> > > > > AVPROBE_SCORE_MAX/5;
> > > > > +if (p->buf[6] != 0) score -=
> > > > > AVPROBE_SCORE_MAX/5;
> > > > > +return score;
> > > > > 
> > > > > Imo, this is too complicated:
> > > > > If the first 32bit are correct, return MAX/2, for 24bit,
> > > > > return
> > > > > less
> > > 
> > > This should have been AVPROBE_SCORE_EXTENSION + 1,
> > > sorry about my mistake.
> > 
> > Done.
> > 
> > > Please threaten to push this and push after a few days.
> > 
> > Alright, rebased. I'll push on Sunday if there's no objections
> > 
> > /Tomas
> 
> Aaaand a set that actually passes FATE this time :)

Pushed

/Tomas

signature.asc
Description: This is a digitally signed message part
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] Building FFmpeg dylibs for OSX

2018-02-24 Thread livinginlosangeles 
Will examine that.

Kolya

> On Feb 23, 2018, at 11:52 PM, Helmut K. C. Tessarek  
> wrote:
> 
> On 2018-02-24 01:22, livinginlosange...@mac.com wrote:
>> I can successfully compile ffmpeg info lgpl compliant dylibs which I can 
>> link to from /usr/local. I specify /usr/local during my ./configure phase. 
>> All OK. I want to be able to include them as dylibs in my OSX app package. 
>> The app wants to see dylibs which are built using @rpath like 
>> @rpath/libavutil.dylib when inspecting the dylib using otool. Would anyone 
>> have any recommendations on how to accomplish this? I must include ffmpeg 
>> dylibs in my app.
> 
> You can have a look at how the VLC project does it. All of their libs
> are dynamic libs which are located in one sub directory of the app.
> 
> So you have to set the rpath relative to your binary that makes the calls.
> 
> Cheers,
>  K. C.
> 
> -- 
> regards Helmut K. C. Tessarek  KeyID 0x172380A011EF4944
> Key fingerprint = 8A55 70C1 BD85 D34E ADBC 386C 1723 80A0 11EF 4944
> 
> /*
>   Thou shalt not follow the NULL pointer for chaos and madness
>   await thee at its end.
> */
> 

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

Re: [FFmpeg-devel] [PATCH 2/3] avformat/dashenc: opening a segment file when its first frame is ready

2018-02-24 Thread James Almer 
On 2/24/2018 12:19 PM, Marton Balint wrote:
> 
> 
> On Mon, 19 Feb 2018, vdi...@akamai.com wrote:
> 
>> From: Vishwanath Dixit 
>>
>> ---
>> libavformat/dashenc.c | 57
>> ---
>> 1 file changed, 36 insertions(+), 21 deletions(-)
>>
>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>> index 0f6f4f2..0eb4b25 100644
>> --- a/libavformat/dashenc.c
>> +++ b/libavformat/dashenc.c
>> @@ -81,6 +81,9 @@ typedef struct OutputStream {
>>     char bandwidth_str[64];
>>
>>     char codec_str[100];
>> +    char filename[1024];
>> +    char full_path[1024];
>> +    char temp_path[1024];
>> } OutputStream;
> 
> I know it's late, but in the future please work toward supporting
> unlimited path lengths, that was the whole point of the deprecation of
> AVFormatContext->filename.
> 
> Thanks,
> Marton

It's not late, it can and should be changed. New code using deprecated
APIs that will require changes in the long run should have not been
added, so better change it now.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/3] avformat/dashenc: opening a segment file when its first frame is ready

2018-02-24 Thread Marton Balint 



On Mon, 19 Feb 2018, vdi...@akamai.com wrote:


From: Vishwanath Dixit 

---
libavformat/dashenc.c | 57 ---
1 file changed, 36 insertions(+), 21 deletions(-)

diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
index 0f6f4f2..0eb4b25 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -81,6 +81,9 @@ typedef struct OutputStream {
char bandwidth_str[64];

char codec_str[100];
+char filename[1024];
+char full_path[1024];
+char temp_path[1024];
} OutputStream;


I know it's late, but in the future please work toward supporting 
unlimited path lengths, that was the whole point of the deprecation of 
AVFormatContext->filename.


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

Re: [FFmpeg-devel] [PATCH 7/9] sbcenc: add MMX optimizations

2018-02-24 Thread Aurelien Jacobs 
On Thu, Feb 22, 2018 at 05:21:57PM +, Rostislav Pehlivanov wrote:
> On 21 February 2018 at 22:37, Aurelien Jacobs  wrote:
> [...]
> > +;***
> > +;void ff_sbc_analyze_4(const int16_t *in, int32_t *out, const int16_t
> > *consts);
> > +;***
> > +INIT_MMX mmx
> > +cglobal sbc_analyze_4, 3, 3, 4, in, out, consts
> > +movq  m0, [inq]
> > +movq  m1, [inq+8]
> > +pmaddwd   m0, [constsq]
> > +pmaddwd   m1, [constsq+8]
> > +paddd m0, [scale_mask]
> > +paddd m1, [scale_mask]
> > +
> > +movq  m2, [inq+16]
> > +movq  m3, [inq+24]
> > +pmaddwd   m2, [constsq+16]
> > +pmaddwd   m3, [constsq+24]
> > +paddd m0, m2
> > +paddd m1, m3
> > +
> > +movq  m2, [inq+32]
> > +movq  m3, [inq+40]
> > +pmaddwd   m2, [constsq+32]
> > +pmaddwd   m3, [constsq+40]
> > +paddd m0, m2
> > +paddd m1, m3
> > +
> > +movq  m2, [inq+48]
> > +movq  m3, [inq+56]
> > +pmaddwd   m2, [constsq+48]
> > +pmaddwd   m3, [constsq+56]
> > +paddd m0, m2
> > +paddd m1, m3
> > +
> > +movq  m2, [inq+64]
> > +movq  m3, [inq+72]
> > +pmaddwd   m2, [constsq+64]
> > +pmaddwd   m3, [constsq+72]
> > +paddd m0, m2
> > +paddd m1, m3
> >
> 
> You can macro the top 3 blocks
> 
> [...]
> > +;***
> > +;void ff_sbc_analyze_8(const int16_t *in, int32_t *out, const int16_t
> > *consts);
> > +;***
> > +INIT_MMX mmx
> > +cglobal sbc_analyze_8, 3, 3, 4, in, out, consts
> > +movq  m0, [inq]
> > +movq  m1, [inq+8]
> > +movq  m2, [inq+16]
> > +movq  m3, [inq+24]
> > +pmaddwd   m0, [constsq]
> > +pmaddwd   m1, [constsq+8]
> > +pmaddwd   m2, [constsq+16]
> > +pmaddwd   m3, [constsq+24]
> > +paddd m0, [scale_mask]
> > +paddd m1, [scale_mask]
> > +paddd m2, [scale_mask]
> > +paddd m3, [scale_mask]
> > +
> > +movq  m4, [inq+32]
> > +movq  m5, [inq+40]
> > +movq  m6, [inq+48]
> > +movq  m7, [inq+56]
> > +pmaddwd   m4, [constsq+32]
> > +pmaddwd   m5, [constsq+40]
> > +pmaddwd   m6, [constsq+48]
> > +pmaddwd   m7, [constsq+56]
> > +paddd m0, m4
> > +paddd m1, m5
> > +paddd m2, m6
> > +paddd m3, m7
> > +
> > +movq  m4, [inq+64]
> > +movq  m5, [inq+72]
> > +movq  m6, [inq+80]
> > +movq  m7, [inq+88]
> > +pmaddwd   m4, [constsq+64]
> > +pmaddwd   m5, [constsq+72]
> > +pmaddwd   m6, [constsq+80]
> > +pmaddwd   m7, [constsq+88]
> > +paddd m0, m4
> > +paddd m1, m5
> > +paddd m2, m6
> > +paddd m3, m7
> > +
> > +movq  m4, [inq+96]
> > +movq  m5, [inq+104]
> > +movq  m6, [inq+112]
> > +movq  m7, [inq+120]
> > +pmaddwd   m4, [constsq+96]
> > +pmaddwd   m5, [constsq+104]
> > +pmaddwd   m6, [constsq+112]
> > +pmaddwd   m7, [constsq+120]
> > +paddd m0, m4
> > +paddd m1, m5
> > +paddd m2, m6
> > +paddd m3, m7
> > +
> > +movq  m4, [inq+128]
> > +movq  m5, [inq+136]
> > +movq  m6, [inq+144]
> > +movq  m7, [inq+152]
> > +pmaddwd   m4, [constsq+128]
> > +pmaddwd   m5, [constsq+136]
> > +pmaddwd   m6, [constsq+144]
> > +pmaddwd   m7, [constsq+152]
> > +paddd m0, m4
> > +paddd m1, m5
> > +paddd m2, m6
> > +paddd m3, m7
> >
> 
> And those 5 blocks
> 
> 
> > +
> > +psrad m0, 16; SBC_PROTO_FIXED_SCALE
> > +psrad m1, 16; SBC_PROTO_FIXED_SCALE
> > +psrad m2, 16; SBC_PROTO_FIXED_SCALE
> > +psrad m3, 16; SBC_PROTO_FIXED_SCALE
> > +
> > +packssdw  m0, m0
> > +packssdw  m1, m1
> > +packssdw  m2, m2
> > +packssdw  m3, m3
> > +
> > +movq  m4, m0
> > +movq  m5, m0
> > +pmaddwd   m4, [constsq+160]
> > +pmaddwd   m5, [constsq+168]
> > +
> > +movq  m6, m1
> > +movq  m7, m1
> > +pmaddwd   m6, [constsq+192]
> > +pmaddwd   m7, [constsq+200]
> > +paddd m4, m6
> > +paddd m5, m7
> > +
> > +movq  m6, m2
> > +movq  m7, m2
> > +pmaddwd   m6, [constsq+224]
> > +pmaddwd   m7,

Re: [FFmpeg-devel] [PATCH 5/9] sbc: implement SBC encoder (low-complexity subband codec)

2018-02-24 Thread Aurelien Jacobs 
On Thu, Feb 22, 2018 at 06:18:48PM +, Rostislav Pehlivanov wrote:
> On 21 February 2018 at 22:37, Aurelien Jacobs  wrote:
> 
> > This was originally based on libsbc, and was fully integrated into ffmpeg.
> > ---
> >  doc/general.texi |   2 +-
> >  libavcodec/Makefile  |   1 +
> >  libavcodec/allcodecs.c   |   1 +
> >  libavcodec/sbcdsp.c  | 382 ++
> > +
> >  libavcodec/sbcdsp.h  |  83 ++
> >  libavcodec/sbcdsp_data.c | 329 +
> >  libavcodec/sbcdsp_data.h |  55 +++
> >  libavcodec/sbcenc.c  | 411 ++
> > +
> >  8 files changed, 1263 insertions(+), 1 deletion(-)
> >  create mode 100644 libavcodec/sbcdsp.c
> >  create mode 100644 libavcodec/sbcdsp.h
> >  create mode 100644 libavcodec/sbcdsp_data.c
> >  create mode 100644 libavcodec/sbcdsp_data.h
> >  create mode 100644 libavcodec/sbcenc.c
> >
> > +
> > +#define OFFSET(x) offsetof(SBCEncContext, x)
> > +#define AE AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
> > +static const AVOption options[] = {
> > +{ "joint_stereo", "use joint stereo",
> > +  OFFSET(joint_stereo), AV_OPT_TYPE_BOOL, { .i64 =  0 }, 0,   1, AE },
> >
> +{ "dual_channel", "use dual channel",
> > +  OFFSET(dual_channel), AV_OPT_TYPE_BOOL, { .i64 =  0 }, 0,   1, AE },
> >
> 
> Erm those 2 things should be decided by the encoder, not by exposing them
> to the user. The encoder should decide which mode has lower distortion for
> a given signal.

See bellow.

> > +{ "subbands", "number of subbands (4 or 8)",
> > +  OFFSET(subbands), AV_OPT_TYPE_INT,  { .i64 =  8 }, 4,   8, AE },
> >
> 
> The encoder doesn't check if the value isn't 4 or 8 so 5, 6 and 7 are all
> accepted. Similar issue to the previous option too.

OK, fixed.

> > +{ "bitpool",  "bitpool value",
> > +  OFFSET(bitpool),  AV_OPT_TYPE_INT,  { .i64 = 32 }, 0, 255, AE },
> >
> 
> This should be controlled by the bitrate setting. Either have a function to
> translate bitrate to bitpool value or a table which approximately maps
> bitrate values supplied to bitpools. You could expose it directly as well
> as mapping it to a bitrate value by using the global_quality setting so it
> shouldn't be a custom encoder option.

Indeed, this is better this way, thanks for the suggestion.

> > +{ "blocks",   "number of blocks (4, 8, 12 or 16)",
> > +  OFFSET(blocks),   AV_OPT_TYPE_INT,  { .i64 = 16 }, 4,  16, AE },
> > +{ "snr",  "use SNR mode (instead of loudness)",
> > +  OFFSET(allocation),   AV_OPT_TYPE_BOOL, { .i64 =  0 }, 0,   1, AE },
> >
> 
> SNR mode too needs to be decided by the encoder rather than exposing it as
> a setting.

See bellow.

> > +{ "msbc", "use mSBC mode (wideband speech mono SBC)",
> >
> 
> Add a profile fallback setting for this as well, like in aac where -aac_ltp
> turns LTP mode on and -profile:a aac_ltp does the same.

Not sure of the benefits of having 2 redundant way to do this, but OK.

> You don't have to make the encoder decide which stereo coupling mode or
> snr/loudness setting to use, you can implement that with a later patch.
> I think you should remove the "blocks" and "subbands" settings as well and
> instead replace those with a single "latency" setting like the native Opus
> encoder in milliseconds which would adjust both of them on init to set the
> frame size. This would also allow the encoder to change them. Again, you
> don't have to do this now, you can send a patch which adds a "latency"
> option later.

I can see the value in what you propose, and I think that indeed, it
would be great for the encoder to choose by itself the most appropriate
parameters by default.
But on the other hand, we are talking about a codec targetting some
hardware decoders (blutetooth speakers, headsets...), and all those
parameters have to be negotiated between the encoder and the hardware
decoder. So I think it is mandatory to keep all those parameters
accessible to be able to setup the encoder according to the target
hardware capabilities.

> Apart from that, I tested the encoder, valgrind looks clean, the SIMD is
> bitexact and all advertised samplerates are supported.

Great.

Updated patch attached.>From e0fa5e81606047e31c4f9d0c7773f82a044473f3 Mon Sep 17 00:00:00 2001
From: Aurelien Jacobs 
Date: Sun, 17 Dec 2017 19:59:30 +0100
Subject: [PATCH 5/9] sbc: implement SBC encoder (low-complexity subband codec)

This was originally based on libsbc, and was fully integrated into ffmpeg.
---
 doc/general.texi   |   2 +-
 libavcodec/Makefile|   1 +
 libavcodec/allcodecs.c |   1 +
 libavcodec/avcodec.h   |   2 +
 libavcodec/options_table.h |   1 +
 libavcodec/profiles.c  |   5 +
 libavcodec/profiles.h  |   1 +
 libavcodec/sbcdsp.c| 382 +++
 libavcodec/sbcdsp.h