[FFmpeg-devel] Various small fixes
Anders Grönberg
galileo.m2
Sat Dec 13 17:20:21 CET 2008
On Sat, Dec 13, 2008 at 3:52 PM, M?ns Rullg?rd <mans at mansr.com> wrote:
> "Anders Gr?nberg" <galileo.m2 at gmail.com> writes:
>
>> I have been experimenting with FFmpeg and this patch fixes some small
>> issues I found in the code base that should be beneficial to other
>> FFmpeg developers.
>>
>> Index: libavcodec/dsputil.c
>> ===================================================================
>> --- libavcodec/dsputil.c (revision 16089)
>> +++ libavcodec/dsputil.c (working copy)
>> @@ -3458,7 +3458,7 @@
>> static void diff_bytes_c(uint8_t *dst, uint8_t *src1, uint8_t *src2, int w){
>> long i;
>> #ifndef HAVE_FAST_UNALIGNED
>> - if((long)src2 & (sizeof(long)-1)){
>> + if((intptr_t)src2 & (sizeof(src2)-1)){
>> for(i=0; i+7<w; i+=8){
>> dst[i+0] = src1[i+0]-src2[i+0];
>> dst[i+1] = src1[i+1]-src2[i+1];
>> @@ -4186,7 +4186,7 @@
>> static int did_fail=0;
>> DECLARE_ALIGNED_16(int, aligned);
>>
>> - if((long)&aligned & 15){
>> + if((intptr_t)&aligned & 15){
>> if(!did_fail){
>> #if defined(HAVE_MMX) || defined(HAVE_ALTIVEC)
>> av_log(NULL, AV_LOG_ERROR,
>
> There was some system that didn't have intptr_t (although it is
> required), so using long (or int) here is more portable. What do you
> suppose this change fixes?
intptr_t is not required by the c99 standard but provided by most
compiler and its used in FFmpeg.
I change it because I think the intptr_t is slightly more correct.
>
>> Index: libavcodec/dsputil.h
>> ===================================================================
>> --- libavcodec/dsputil.h (revision 16089)
>> +++ libavcodec/dsputil.h (working copy)
>> @@ -712,11 +712,11 @@
>> * @param n size of half window
>> */
>> void ff_sine_window_init(float *window, int n);
>> -extern float ff_sine_128 [ 128];
>> -extern float ff_sine_256 [ 256];
>> -extern float ff_sine_512 [ 512];
>> -extern float ff_sine_1024[1024];
>> -extern float ff_sine_2048[2048];
>> +DECLARE_ALIGNED(16, extern float, ff_sine_128 [ 128]);
>> +DECLARE_ALIGNED(16, extern float, ff_sine_256 [ 256]);
>> +DECLARE_ALIGNED(16, extern float, ff_sine_512 [ 512]);
>> +DECLARE_ALIGNED(16, extern float, ff_sine_1024[1024]);
>> +DECLARE_ALIGNED(16, extern float, ff_sine_2048[2048]);
>> extern float *ff_sine_windows[5];
>>
>> int ff_mdct_init(MDCTContext *s, int nbits, int inverse);
>
> These are extern declarations of data. Using DECLARE_ALIGNED here
> will not make change anything.
The compiler could use the alignment information to generate faster
code and similar code already exists in aactab.h.
>
>> Index: libavcodec/imgconvert.c
>> ===================================================================
>> --- libavcodec/imgconvert.c (revision 16089)
>> +++ libavcodec/imgconvert.c (working copy)
>> @@ -783,7 +783,7 @@
>> dst_pix_fmt = -1;
>> min_dist = 0x7fffffff;
>> for(i = 0;i < PIX_FMT_NB; i++) {
>> - if (pix_fmt_mask & (1 << i)) {
>> + if (pix_fmt_mask & (1ULL << i)) {
>> loss = avcodec_get_pix_fmt_loss(i, src_pix_fmt, has_alpha) & loss_mask;
>> if (loss == 0) {
>> dist = avg_bits_per_pixel(i);
>
> This is madness. What do you believe the benefit of that to be?
>
>> Index: libavformat/mov.c
>> ===================================================================
>> --- libavformat/mov.c (revision 16089)
>> +++ libavformat/mov.c (working copy)
>> @@ -166,7 +166,7 @@
>> int (*parse)(MOVContext *ctx, ByteIOContext *pb, MOVAtom atom);
>> } MOVParseTableEntry;
>>
>> -static const MOVParseTableEntry mov_default_parse_table[];
>> +const MOVParseTableEntry mov_default_parse_table[];
>>
>> static int mov_read_default(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
>> {
>
> Why?
The c99 standard says that a forward declaration of an array of
unknown size should not contain the static keyword, it should only be
present when the finale size is known. A static forward declaration of
an array of unknown size is undefined behavior.
Although if it doesn't compile on gcc it should be left as it is.
>
>> Index: libavutil/common.h
>> ===================================================================
>> --- libavutil/common.h (revision 16089)
>> +++ libavutil/common.h (working copy)
>> @@ -108,7 +108,7 @@
>> /* assume b>0 */
>> #define ROUNDED_DIV(a,b) (((a)>0 ? (a) + ((b)>>1) : (a) - ((b)>>1))/(b))
>> #define FFABS(a) ((a) >= 0 ? (a) : (-(a)))
>> -#define FFSIGN(a) ((a) > 0 ? 1 : -1)
>> +#define FFSIGN(a) ((a) >= 0 ? 1 : -1)
>>
>> #define FFMAX(a,b) ((a) > (b) ? (a) : (b))
>> #define FFMAX3(a,b,c) FFMAX(FFMAX(a,b),c)
>
> Did this fix something? Are you sure it didn't break something?
Most specifications I have read assumes that 0 == -0 and defines 0 as positive.
If the current behavior is correct then maybe the macro should be renamed.
>
> --
> M?ns Rullg?rd
> mans at mansr.com
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remove_static.patch
Type: text/x-patch
Size: 1139 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081213/1b6a634c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: intptr_t.patch
Type: text/x-patch
Size: 798 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081213/1b6a634c/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: declare_aligned.patch
Type: text/x-patch
Size: 845 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081213/1b6a634c/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: const_correctness.patch
Type: text/x-patch
Size: 4387 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081213/1b6a634c/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shift_overflow.patch
Type: text/x-patch
Size: 545 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081213/1b6a634c/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffsign.patch
Type: text/x-patch
Size: 498 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081213/1b6a634c/attachment-0005.bin>
More information about the ffmpeg-devel
mailing list