| Message ID | 20180331225936.6046-1-atomnuker@gmail.com |
|---|---|
| State | Accepted |
| Commit | a1b91b0cc28ac9d7ca77f21a3010233edeee457c |
| Headers | show |
On 4/1/18, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > On modern x86 systems its around 2x faster. For systems without > FPUs it'll be slower, but our policy is to prefer floating point > implementations and to let users decide what's best (or just not > compile them on systems without FPUs). > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > --- > libavcodec/allcodecs.c | 6 +++--- > tests/fate/gapless.mak | 2 +- > tests/ref/fate/exif-image-embedded | 42 > +++++++++++++++++++------------------- > 3 files changed, 25 insertions(+), 25 deletions(-) > lgtm
On 1 April 2018 at 09:56, Paul B Mahol <onemda@gmail.com> wrote: > On 4/1/18, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > > On modern x86 systems its around 2x faster. For systems without > > FPUs it'll be slower, but our policy is to prefer floating point > > implementations and to let users decide what's best (or just not > > compile them on systems without FPUs). > > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > > --- > > libavcodec/allcodecs.c | 6 +++--- > > tests/fate/gapless.mak | 2 +- > > tests/ref/fate/exif-image-embedded | 42 > > +++++++++++++++++++------------------- > > 3 files changed, 25 insertions(+), 25 deletions(-) > > > > lgtm > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > Thanks, pushed
Rostislav Pehlivanov (2018-04-01): > > lgtm > Thanks, pushed I think some clarification is required about reviews. "LGTM" means "looks good to me". I insist: "me". When Paul writes "LGTM", it means the patch looks good to Paul, not anybody else. If Paul is the maintainer of the affected piece of code, of if he is an expert about that kind of code, then his LGTM is enough to go ahead. If the patch is simple and does not involve any policy choices, then all is needed is a second pair of eyes to find the obvious mistakes, and anybody's LGTM is enough to go ahead. But if the patch is complex or if it involves a policy choice, then a single LGTM is not enough. You need to ensure that the patch does not look BAD to anybody, and for that you need to leave time for all developers to look at the patch. Half a day is not enough, even discounting that if was a week-end half-day, and even more so a feast day for some. I do not think it warrants reverting, but please keep that in mind for the future. Regards,
On 1 April 2018 at 13:24, Nicolas George <george@nsup.org> wrote: > Rostislav Pehlivanov (2018-04-01): > > > lgtm > > > Thanks, pushed > > I think some clarification is required about reviews. > > "LGTM" means "looks good to me". I insist: "me". When Paul writes > "LGTM", it means the patch looks good to Paul, not anybody else. > > If Paul is the maintainer of the affected piece of code, of if he is an > expert about that kind of code, then his LGTM is enough to go ahead. > > If the patch is simple and does not involve any policy choices, then all > is needed is a second pair of eyes to find the obvious mistakes, and > anybody's LGTM is enough to go ahead. > > But if the patch is complex or if it involves a policy choice, then a > single LGTM is not enough. You need to ensure that the patch does not > look BAD to anybody, and for that you need to leave time for all > developers to look at the patch. > > Half a day is not enough, even discounting that if was a week-end > half-day, and even more so a feast day for some. I do not think it > warrants reverting, but please keep that in mind for the future. > > Regards, > > -- > Nicolas George > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > Please go on IRC, its where most developers talk, exchange ideas, information, discuss patches and so on. This patch was discussed there, and users and developers have had requests for this patch for over a year, all on IRC.
Rostislav Pehlivanov (2018-04-01): > Please go on IRC, its where most developers talk, exchange ideas, > information, discuss patches and so on. I do not have time to go on IRC. The place where most developers talk, exchange ideas, information, discuss patches and so on is the mailing-list. IRC has never been a requirement, that would be completely silly for a project where the members do not keep the same hours and do not even live all in the same timezone. > This patch was discussed there, and Then please do like other people do in that case: mention it in the mail, "discussed on IRC, pushed". And my comment still applies as a general remainder. Regards,
On 3/31/2018 7:59 PM, Rostislav Pehlivanov wrote: > On modern x86 systems its around 2x faster. For systems without > FPUs it'll be slower, but our policy is to prefer floating point > implementations and to let users decide what's best (or just not > compile them on systems without FPUs). > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> > --- > libavcodec/allcodecs.c | 6 +++--- > tests/fate/gapless.mak | 2 +- > tests/ref/fate/exif-image-embedded | 42 +++++++++++++++++++------------------- > 3 files changed, 25 insertions(+), 25 deletions(-) Regarding this, how about renaming them to follow the same naming scheme as every other float/fixed pair decoder we have? Meaning mp3 for the float version, and mp3_fixed for the fixed version.
On 1 April 2018 at 15:33, Nicolas George <george@nsup.org> wrote: > Rostislav Pehlivanov (2018-04-01): > > Please go on IRC, its where most developers talk, exchange ideas, > > information, discuss patches and so on. > > I do not have time to go on IRC. The place where most developers talk, > exchange ideas, information, discuss patches and so on is the > mailing-list. IRC has never been a requirement, that would be completely > silly for a project where the members do not keep the same hours and do > not even live all in the same timezone. > Backlogs are a thing. And most of us are up in roughly the same period.
Rostislav Pehlivanov (2018-04-01):
> Backlogs are a thing.
Backlogs are a waste of time. IRC discussions are conducted in direct
and barely readable afterwards.
Regards,
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c index 059445000d..4d4ef530e4 100644 --- a/libavcodec/allcodecs.c +++ b/libavcodec/allcodecs.c @@ -432,12 +432,12 @@ extern AVCodec ff_mp2_encoder; extern AVCodec ff_mp2_decoder; extern AVCodec ff_mp2float_decoder; extern AVCodec ff_mp2fixed_encoder; -extern AVCodec ff_mp3_decoder; extern AVCodec ff_mp3float_decoder; -extern AVCodec ff_mp3adu_decoder; +extern AVCodec ff_mp3_decoder; extern AVCodec ff_mp3adufloat_decoder; -extern AVCodec ff_mp3on4_decoder; +extern AVCodec ff_mp3adu_decoder; extern AVCodec ff_mp3on4float_decoder; +extern AVCodec ff_mp3on4_decoder; extern AVCodec ff_mpc7_decoder; extern AVCodec ff_mpc8_decoder; extern AVCodec ff_nellymoser_encoder; diff --git a/tests/fate/gapless.mak b/tests/fate/gapless.mak index 0253b9ec61..91fddb4130 100644 --- a/tests/fate/gapless.mak +++ b/tests/fate/gapless.mak @@ -1,5 +1,5 @@ FATE_GAPLESS-$(CONFIG_MP3_DEMUXER) += fate-gapless-mp3 -fate-gapless-mp3: CMD = gapless $(TARGET_SAMPLES)/gapless/gapless.mp3 +fate-gapless-mp3: CMD = gapless $(TARGET_SAMPLES)/gapless/gapless.mp3 "-c:a mp3" FATE_GAPLESS-$(CONFIG_MP3_DEMUXER) += fate-audiomatch-square-mp3 fate-audiomatch-square-mp3: CMD = audio_match $(TARGET_SAMPLES)/audiomatch/square3.mp3 $(TARGET_SAMPLES)/audiomatch/square3.wav diff --git a/tests/ref/fate/exif-image-embedded b/tests/ref/fate/exif-image-embedded index 0b640767a8..392c145efb 100644 --- a/tests/ref/fate/exif-image-embedded +++ b/tests/ref/fate/exif-image-embedded @@ -50,7 +50,7 @@ pkt_duration=15040 pkt_duration_time=0.001066 pkt_pos=16292 pkt_size=417 -sample_fmt=s16p +sample_fmt=fltp nb_samples=47 channels=2 channel_layout=stereo @@ -69,7 +69,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=16709 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -88,7 +88,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=17127 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -107,7 +107,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=17545 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -126,7 +126,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=17963 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -145,7 +145,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=18381 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -164,7 +164,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=18799 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -183,7 +183,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=19217 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -202,7 +202,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=19635 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -221,7 +221,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=20053 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -240,7 +240,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=20471 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -259,7 +259,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=20889 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -278,7 +278,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=21307 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -297,7 +297,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=21725 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -316,7 +316,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=22143 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -335,7 +335,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=22561 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -354,7 +354,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=22979 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -373,7 +373,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=23397 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -392,7 +392,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=23815 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -411,7 +411,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=24233 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo @@ -430,7 +430,7 @@ pkt_duration=368640 pkt_duration_time=0.026122 pkt_pos=24651 pkt_size=418 -sample_fmt=s16p +sample_fmt=fltp nb_samples=1152 channels=2 channel_layout=stereo
On modern x86 systems its around 2x faster. For systems without FPUs it'll be slower, but our policy is to prefer floating point implementations and to let users decide what's best (or just not compile them on systems without FPUs). Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> --- libavcodec/allcodecs.c | 6 +++--- tests/fate/gapless.mak | 2 +- tests/ref/fate/exif-image-embedded | 42 +++++++++++++++++++------------------- 3 files changed, 25 insertions(+), 25 deletions(-)