On Tue, Dec 15, 2015 at 11:20 AM, Michael Niedermayer <michae...@gmx.at> wrote:
> On Tue, Dec 15, 2015 at 08:58:01AM +0100, Jean Delvare wrote:
>> Hallo Michael,
>>
>> On Mon, 14 Dec 2015 23:18:39 +0100, Michael Niedermayer wrote:
>> > On Mon, Dec 14, 2015 at 07:36:51PM +0100, Jean Delvare wrote:
>> > > As I understand it, the temporary stack buffer "src_data" was
>> > > introduced solely to avoid a compiler warning. I believe that a better
>> > > way to solve this warning it to explicitly cast src->data. This
>> > > should be somewhat faster, and just as safe.
>> > >
>> > > Signed-off-by: Jean Delvare <jdelv...@suse.de>
>> > > Cc: Anton Khirnov <an...@khirnov.net>
>> > > ---
>> > > libavutil/frame.c | 4 +---
>> > > 1 file changed, 1 insertion(+), 3 deletions(-)
>> > >
>> > > --- ffmpeg.orig/libavutil/frame.c 2015-12-14 18:42:06.272234579 +0100
>> > > +++ ffmpeg/libavutil/frame.c 2015-12-14 19:05:18.501745387 +0100
>> > > @@ -647,7 +647,6 @@ AVFrameSideData *av_frame_get_side_data(
>> > >
>> > > static int frame_copy_video(AVFrame *dst, const AVFrame *src)
>> > > {
>> > > - const uint8_t *src_data[4];
>> > > int i, planes;
>> > >
>> > > if (dst->width < src->width ||
>> > > @@ -659,9 +658,8 @@ static int frame_copy_video(AVFrame *dst
>> > > if (!dst->data[i] || !src->data[i])
>> > > return AVERROR(EINVAL);
>> > >
>> > > - memcpy(src_data, src->data, sizeof(src_data));
>> > > av_image_copy(dst->data, dst->linesize,
>> > > - src_data, src->linesize,
>> > > + (const uint8_t **)src->data, src->linesize,
>> >
>> > I think this may be a aliasing violation and thus undefined
>> > not that i like that or consider that sane
>> > so if someone says iam wrong, i would certainly not be unhappy
>>
>> Why would it be an aliasing violation? We do not change the fundamental
>> type of the pointer, we only add a const where the function wants it.
>> For more simple pointer types the compiler would do it for us silently.
>
> A. uint8_t and const uint8_t are distinct types
> "Any type so far mentioned is an unqualified type. Each unqualified
> type has several
> qualified versions of its type,38) corresponding to the combinations
> of one, two, or all
> three of the const, volatile, and restrict qualifiers. The qualified
> or unqualified
> versions of a type are distinct types that belong to the same type
> category and have the
> same representation and alignment requirements."
> B. a pointer to uint8_t and const uint8_t are not qualified types of the same
> type
> See the example in 6.2.5
> "28 EXAMPLE 1 The type designated as ‘‘float *’’ has type ‘‘pointer
> to float’’. Its type category is
> pointer, not a floating type. The const-qualified version of this
> type is designated as ‘‘float * const’’
> whereas the type designated as ‘‘const float *’’ is not a qualified
> type — its type is ‘‘pointer to constqualified
> float’’ and is a pointer to a qualified type."
> C. They are not compatible
> "Two types have compatible type if their types are the same.
> Additional rules for
> determining whether two types are compatible are described in 6.7.2
> for type specifiers,
> in 6.7.3 for type qualifiers, and in 6.7.5 for declarators.46)"
> D. None of the aliasing exceptions seems to apply
> "An object shall have its stored value accessed only by an lvalue
> expression that has one of the following types:76)
> — atype compatible with the effective type of the object,
> — aqualified version of a type compatible with the effective type of
> the object,"
> (its not a qualified version)
> " — a type that is the signed or unsigned type corresponding to the
> effective type of the object,
> — a type that is the signed or unsigned type corresponding to a
> qualified version of the effective type of the object,
> — an aggregate or union type that includes one of the aforementioned
> types among its members (including, recursively, a member of a subaggregate
> or contained union), or
> — acharacter type."
> (dont apply)
>
> also:
> "For two qualified types to be compatible, both shall have the
> identically qualified version
> of a compatible type; the order of type qualifiers within a list of
> specifiers or qualifiers
> does not affect the specified type."
>
> "For two pointer types to be compatible, both shall be identically
> qualified and both shall
> be pointers to compatible types."
>
> I quite possible could misinterpret or miss some parts though ...
>
>
>>
>> Also I can see 12 occurrences of the same cast for this parameter of
>> function av_image_copy() in the ffmpeg code already. And over 20 more
>> similar casts for similar parameters of other functions
>> (ff_combine_frame, swr_convert, copy_picture_field...) So I'm not
>> introducing anything new, just proposing one more of the same.
>
> yes, I have no real oppinion on this except that C is insane or I am
> and i dont really mind to apply the patch if thats what people prefer.
> Any real compiler that follows this litterally and breaks the code is
> IMHO a compiler one should avoid (quite independant of it being used
> for FFmpeg or other things) unless one wants to language lawyer around
> on such things instead of writing usefull code
>
The "speed up" from removing a copy of 4 pointers is negligible as
well however, so maybe it should just be kept like it is.
- Hendrik
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
