Re: [FFmpeg-devel] fate: Do not report side data size

[Ronald S. Bultje]

Hi,

On Fri, Mar 17, 2017 at 11:43 AM, Vittorio Giovara <
vittorio.giov...@gmail.com> wrote:

> On Tue, Mar 7, 2017 at 7:17 PM, Vittorio Giovara
>  wrote:
> > This should address the mismatch between different archs
> > Please CC.
> > --
> > Vittorio
>
> ping


No objections issues after 24hrs means patch is OK in FFmpeg development.
But since you're asking for explicit approval: patch is OK. Please push.

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

Re: [FFmpeg-devel] fate: Do not report side data size

[Vittorio Giovara]

On Tue, Mar 7, 2017 at 7:17 PM, Vittorio Giovara
 wrote:
> This should address the mismatch between different archs
> Please CC.
> --
> Vittorio

ping

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

Re: [FFmpeg-devel] fate: Do not report side data size

[wm4]

On Thu, 9 Mar 2017 20:48:51 +0100
Michael Niedermayer  wrote:

> On Thu, Mar 09, 2017 at 07:48:53AM +0100, wm4 wrote:
> > On Thu, 9 Mar 2017 02:20:03 +0100
> > Michael Niedermayer  wrote:
> >   
> > > On Wed, Mar 08, 2017 at 11:54:59PM +0100, Hendrik Leppkes wrote:  
> > > > On Wed, Mar 8, 2017 at 3:42 PM, Ronald S. Bultje  
> > > > wrote:
> > > > > Hi,
> > > > >
> > > > > On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:
> > > > >
> > > > >> On Wed, 8 Mar 2017 14:09:53 +0100
> > > > >> Michael Niedermayer  wrote:
> > > > >>
> > > > >> If the size printing is removed then other code should be added
> > > > >> > to test for the size to match the correct value
> > > > >>
> > > > >> Then it would be more reasonable to make av_packet_add_side_data()
> > > > >> check whether the size is correct for the given side data type.
> > > > >
> > > > >
> > > > > I think you're both right here, this is a pretty good idea (for 
> > > > > fixed-size
> > > > > side-data types).
> > > > >
> > > > 
> > > > So how do we fix fate now? Change the datatypes to uint32_t, remove
> > > > the size print out?
> > >   
> > > > Shouldn't keep all 32-bit fate clients broken for much longer.
> > > 
> > > +1
> > >   
> > 
> > You're the one stopping the simple fix.  
> 
> If you or anyone belives this, you can just ask me privatly if i meant
> to object to the patch. Noone asked
> one can even ask me in public, as in "do you object to this patch
> being pushed ?"
> and to save everyone the delay and troubble the awnser is
> 
> I do NOT object to it.
> I would prefer if the final implementation would still print the size
> where it is relevant. That can be done in a seperate patch. Its
> important to fix this issue so it should not be delayed by this
> discussion (thats the long form of the "+1" above really)
> 
> But instead of asking, you publically claim that iam stoping the fix.
> that is not ok IMO

OK, so after this long, exhausting discussion that seemed to lead
nowhere, you actually were never against this patch all along. Maybe
you should be more explicit about whether you think a patch needs
requires more changes, or if it's fine to push. Because this sort
of thing seemed to happen multiple times in the past.

A patch author can't push a patch if there are still open requests for
amendments, so this should be made clear to not block application
unnecessarily.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] fate: Do not report side data size

[Michael Niedermayer]

On Thu, Mar 09, 2017 at 07:48:53AM +0100, wm4 wrote:
> On Thu, 9 Mar 2017 02:20:03 +0100
> Michael Niedermayer  wrote:
> 
> > On Wed, Mar 08, 2017 at 11:54:59PM +0100, Hendrik Leppkes wrote:
> > > On Wed, Mar 8, 2017 at 3:42 PM, Ronald S. Bultje  
> > > wrote:  
> > > > Hi,
> > > >
> > > > On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:
> > > >  
> > > >> On Wed, 8 Mar 2017 14:09:53 +0100
> > > >> Michael Niedermayer  wrote:
> > > >>
> > > >> If the size printing is removed then other code should be added  
> > > >> > to test for the size to match the correct value  
> > > >>
> > > >> Then it would be more reasonable to make av_packet_add_side_data()
> > > >> check whether the size is correct for the given side data type.  
> > > >
> > > >
> > > > I think you're both right here, this is a pretty good idea (for 
> > > > fixed-size
> > > > side-data types).
> > > >  
> > > 
> > > So how do we fix fate now? Change the datatypes to uint32_t, remove
> > > the size print out?  
> > 
> > > Shouldn't keep all 32-bit fate clients broken for much longer.  
> > 
> > +1
> > 
> 
> You're the one stopping the simple fix.

If you or anyone belives this, you can just ask me privatly if i meant
to object to the patch. Noone asked
one can even ask me in public, as in "do you object to this patch
being pushed ?"
and to save everyone the delay and troubble the awnser is

I do NOT object to it.
I would prefer if the final implementation would still print the size
where it is relevant. That can be done in a seperate patch. Its
important to fix this issue so it should not be delayed by this
discussion (thats the long form of the "+1" above really)

But instead of asking, you publically claim that iam stoping the fix.
that is not ok IMO


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.


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

Re: [FFmpeg-devel] fate: Do not report side data size

[Nicolas George]

Le nonidi 19 ventôse, an CCXXV, Michael Niedermayer a écrit :
> it is a property of the file in multiple cases
> 
> through the split side data code but even if this is removed
> for example
> AV_PKT_DATA_NEW_EXTRADATA is just binary data from the file the
> length is from the file
> 
> or AV_PKT_DATA_STRINGS_METADATA, which are multiple zero terminated
> strings
> 
> and there is AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL which again is
> basically binary data and length from the file
> 
> If you remove displaying the size based on the argumnet that it
> ideally should be the sizeof of a struct and thus should not be
> printed.
> Then that change should be limited to the cases where this
> argumnet applies

No, this is taking it the wrong way. Side data size being relevant is
the exception, not the rule. Even for AV_PKT_DATA_STRINGS_METADATA, it
is not the relevant information.

Treating it as the rule and not the exception is exactly what lead us to
this problem, and will cause still more maintainability problems in the
future.

The only sane way to go is to remove it now altogether, and only then
add back printing relevant information for specific kinds of side data.

Regards,

-- 
  Nicolas George


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

Re: [FFmpeg-devel] fate: Do not report side data size

[wm4]

On Thu, 9 Mar 2017 02:20:03 +0100
Michael Niedermayer  wrote:

> On Wed, Mar 08, 2017 at 11:54:59PM +0100, Hendrik Leppkes wrote:
> > On Wed, Mar 8, 2017 at 3:42 PM, Ronald S. Bultje  
> > wrote:  
> > > Hi,
> > >
> > > On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:
> > >  
> > >> On Wed, 8 Mar 2017 14:09:53 +0100
> > >> Michael Niedermayer  wrote:
> > >>
> > >> If the size printing is removed then other code should be added  
> > >> > to test for the size to match the correct value  
> > >>
> > >> Then it would be more reasonable to make av_packet_add_side_data()
> > >> check whether the size is correct for the given side data type.  
> > >
> > >
> > > I think you're both right here, this is a pretty good idea (for fixed-size
> > > side-data types).
> > >  
> > 
> > So how do we fix fate now? Change the datatypes to uint32_t, remove
> > the size print out?  
> 
> > Shouldn't keep all 32-bit fate clients broken for much longer.  
> 
> +1
> 

You're the one stopping the simple fix.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] fate: Do not report side data size

[wm4]

On Thu, 9 Mar 2017 01:45:29 +0100
Michael Niedermayer  wrote:

> On Wed, Mar 08, 2017 at 11:54:59PM +0100, Hendrik Leppkes wrote:
> > On Wed, Mar 8, 2017 at 3:42 PM, Ronald S. Bultje  
> > wrote:  
> > > Hi,
> > >
> > > On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:
> > >  
> > >> On Wed, 8 Mar 2017 14:09:53 +0100
> > >> Michael Niedermayer  wrote:
> > >>
> > >> If the size printing is removed then other code should be added  
> > >> > to test for the size to match the correct value  
> > >>
> > >> Then it would be more reasonable to make av_packet_add_side_data()
> > >> check whether the size is correct for the given side data type.  
> > >
> > >
> > > I think you're both right here, this is a pretty good idea (for fixed-size
> > > side-data types).
> > >  
> > 
> > So how do we fix fate now? Change the datatypes to uint32_t, remove
> > the size print out?  
> 
> why is the data type size_t and not uint32_t int64_t or unsigned int ?
> 
> independant of the fate issue i mean, size_t seems a strange choice

Do you even read my posts?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] fate: Do not report side data size

[Vittorio Giovara]

On Wed, Mar 8, 2017 at 8:15 PM, James Almer  wrote:
> On 3/8/2017 9:45 PM, Michael Niedermayer wrote:
>> On Wed, Mar 08, 2017 at 11:54:59PM +0100, Hendrik Leppkes wrote:
>>> On Wed, Mar 8, 2017 at 3:42 PM, Ronald S. Bultje  wrote:
 Hi,

 On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:

> On Wed, 8 Mar 2017 14:09:53 +0100
> Michael Niedermayer  wrote:
>
> If the size printing is removed then other code should be added
>> to test for the size to match the correct value
>
> Then it would be more reasonable to make av_packet_add_side_data()
> check whether the size is correct for the given side data type.


 I think you're both right here, this is a pretty good idea (for fixed-size
 side-data types).

>>>
>>> So how do we fix fate now? Change the datatypes to uint32_t, remove
>>> the size print out?
>>
>> why is the data type size_t and not uint32_t int64_t or unsigned int ?
>>
>> independant of the fate issue i mean, size_t seems a strange choice

Well as the name implies, size_t should be usable to represent sizes ;)
Beside that, the choice of making these elements size_t was made after
the rectangle defined in avframe, whose fields are defined with size_t
too.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] fate: Do not report side data size

[Michael Niedermayer]

On Wed, Mar 08, 2017 at 11:54:59PM +0100, Hendrik Leppkes wrote:
> On Wed, Mar 8, 2017 at 3:42 PM, Ronald S. Bultje  wrote:
> > Hi,
> >
> > On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:
> >
> >> On Wed, 8 Mar 2017 14:09:53 +0100
> >> Michael Niedermayer  wrote:
> >>
> >> If the size printing is removed then other code should be added
> >> > to test for the size to match the correct value
> >>
> >> Then it would be more reasonable to make av_packet_add_side_data()
> >> check whether the size is correct for the given side data type.
> >
> >
> > I think you're both right here, this is a pretty good idea (for fixed-size
> > side-data types).
> >
> 
> So how do we fix fate now? Change the datatypes to uint32_t, remove
> the size print out?

> Shouldn't keep all 32-bit fate clients broken for much longer.

+1

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire


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

Re: [FFmpeg-devel] fate: Do not report side data size

[James Almer]

On 3/8/2017 1:27 PM, Nicolas George wrote:
> L'octidi 18 ventôse, an CCXXV, Vittorio Giovara a écrit :
>> I'm just going to say that for the case at hand
>> sizeof(AVSphericalMapping) is not part of the ABI, and so it is
>> allowed to have different sizes on different architectures.
> 
> I am not sure exactly what you are asserting here, but just for the
> record, having different sizes on different architectures is completely
> orthogonal to ABI stability, since the ABI is specific to a certain
> architecture anyway.
> 
> For example, sizeof(AVOption) is part of the ABI, but it is not the same
> on 32 and 64-bits systems.
> 
> Regards,

CCing Vittorio.

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

Re: [FFmpeg-devel] fate: Do not report side data size

[James Almer]

On 3/8/2017 9:45 PM, Michael Niedermayer wrote:
> On Wed, Mar 08, 2017 at 11:54:59PM +0100, Hendrik Leppkes wrote:
>> On Wed, Mar 8, 2017 at 3:42 PM, Ronald S. Bultje  wrote:
>>> Hi,
>>>
>>> On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:
>>>
 On Wed, 8 Mar 2017 14:09:53 +0100
 Michael Niedermayer  wrote:

 If the size printing is removed then other code should be added
> to test for the size to match the correct value

 Then it would be more reasonable to make av_packet_add_side_data()
 check whether the size is correct for the given side data type.
>>>
>>>
>>> I think you're both right here, this is a pretty good idea (for fixed-size
>>> side-data types).
>>>
>>
>> So how do we fix fate now? Change the datatypes to uint32_t, remove
>> the size print out?
> 
> why is the data type size_t and not uint32_t int64_t or unsigned int ?
> 
> independant of the fate issue i mean, size_t seems a strange choice

CCing Vittorio since the CC was lost at some point.

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

Re: [FFmpeg-devel] fate: Do not report side data size

[Michael Niedermayer]

On Thu, Mar 09, 2017 at 12:07:38AM +0100, Nicolas George wrote:
> L'octidi 18 ventôse, an CCXXV, Hendrik Leppkes a écrit :
> > So how do we fix fate now? Change the datatypes to uint32_t, remove
> > the size print out?
> > Shouldn't keep all 32-bit fate clients broken for much longer.
> 
> Changing the types like that will not guarantee that all architectures
> have have the same struct size, it will only make it happen because all
> 32-bits architectures are basically identical and all 64-bits
> architectures too.
> 
> If we want a guarantee, we need to consider side data as an octet buffer
> where fields must be explicitly serialized and deserialized. But I think
> this would be piling worse design on top of bad design.
> 
> In the long run, we should either get rid of side data entirely or turn
> it into something actually useful. Right now, it only brings complexity
> with zero benefit.
> 

> In the short run, I would say that printing the side data size in
> ffprobe is wrong: this size is an artifact of FFmpeg's internal data
> structures, not a property of the file, printing it makes as much sense
> as printing sizeof(AVPacket) instead of packet->size.

it is a property of the file in multiple cases

through the split side data code but even if this is removed
for example
AV_PKT_DATA_NEW_EXTRADATA is just binary data from the file the
length is from the file

or AV_PKT_DATA_STRINGS_METADATA, which are multiple zero terminated
strings

and there is AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL which again is
basically binary data and length from the file

If you remove displaying the size based on the argumnet that it
ideally should be the sizeof of a struct and thus should not be
printed.
Then that change should be limited to the cases where this
argumnet applies


[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle


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

Re: [FFmpeg-devel] fate: Do not report side data size

[Michael Niedermayer]

On Wed, Mar 08, 2017 at 11:54:59PM +0100, Hendrik Leppkes wrote:
> On Wed, Mar 8, 2017 at 3:42 PM, Ronald S. Bultje  wrote:
> > Hi,
> >
> > On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:
> >
> >> On Wed, 8 Mar 2017 14:09:53 +0100
> >> Michael Niedermayer  wrote:
> >>
> >> If the size printing is removed then other code should be added
> >> > to test for the size to match the correct value
> >>
> >> Then it would be more reasonable to make av_packet_add_side_data()
> >> check whether the size is correct for the given side data type.
> >
> >
> > I think you're both right here, this is a pretty good idea (for fixed-size
> > side-data types).
> >
> 
> So how do we fix fate now? Change the datatypes to uint32_t, remove
> the size print out?

why is the data type size_t and not uint32_t int64_t or unsigned int ?

independant of the fate issue i mean, size_t seems a strange choice

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes


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

Re: [FFmpeg-devel] fate: Do not report side data size

[Nicolas George]

L'octidi 18 ventôse, an CCXXV, Hendrik Leppkes a écrit :
> So how do we fix fate now? Change the datatypes to uint32_t, remove
> the size print out?
> Shouldn't keep all 32-bit fate clients broken for much longer.

Changing the types like that will not guarantee that all architectures
have have the same struct size, it will only make it happen because all
32-bits architectures are basically identical and all 64-bits
architectures too.

If we want a guarantee, we need to consider side data as an octet buffer
where fields must be explicitly serialized and deserialized. But I think
this would be piling worse design on top of bad design.

In the long run, we should either get rid of side data entirely or turn
it into something actually useful. Right now, it only brings complexity
with zero benefit.

In the short run, I would say that printing the side data size in
ffprobe is wrong: this size is an artifact of FFmpeg's internal data
structures, not a property of the file, printing it makes as much sense
as printing sizeof(AVPacket) instead of packet->size.

Regards,

-- 
  Nicolas George


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

Re: [FFmpeg-devel] fate: Do not report side data size

[Hendrik Leppkes]

On Wed, Mar 8, 2017 at 3:42 PM, Ronald S. Bultje  wrote:
> Hi,
>
> On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:
>
>> On Wed, 8 Mar 2017 14:09:53 +0100
>> Michael Niedermayer  wrote:
>>
>> If the size printing is removed then other code should be added
>> > to test for the size to match the correct value
>>
>> Then it would be more reasonable to make av_packet_add_side_data()
>> check whether the size is correct for the given side data type.
>
>
> I think you're both right here, this is a pretty good idea (for fixed-size
> side-data types).
>

So how do we fix fate now? Change the datatypes to uint32_t, remove
the size print out?
Shouldn't keep all 32-bit fate clients broken for much longer.

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

Re: [FFmpeg-devel] fate: Do not report side data size

[Nicolas George]

L'octidi 18 ventôse, an CCXXV, Vittorio Giovara a écrit :
> I'm just going to say that for the case at hand
> sizeof(AVSphericalMapping) is not part of the ABI, and so it is
> allowed to have different sizes on different architectures.

I am not sure exactly what you are asserting here, but just for the
record, having different sizes on different architectures is completely
orthogonal to ABI stability, since the ABI is specific to a certain
architecture anyway.

For example, sizeof(AVOption) is part of the ABI, but it is not the same
on 32 and 64-bits systems.

Regards,

-- 
  Nicolas George


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

Re: [FFmpeg-devel] fate: Do not report side data size

[Vittorio Giovara]

On Wed, Mar 8, 2017 at 6:51 AM, Michael Niedermayer
 wrote:
>> Removing the side_data_size from output should be fine, as its a
>> implementation detail and as seen here can even vary between
>> architecture or possibly even compiler.
>> Maybe someone that uses that ffprobe output more often can comment?
>
> I use all kinds of stuff
> if something is removed from ffprobes output it wont be tested anymore.
> We should test more not less.

We should test better, not more.

>> An alternative for fixing fate would be to use fixed size fields in
>> the new sidedata, although the possibility of it breaking similarly
>> again in the future then remains.
>
> I strongly prefer fixed size to be used in side data over platform
> dependant fields. Not only does size become testable but theres also
> a platform specific difference less in the interface which should
> help bug reproducability between platforms

I won't comment on this statement which I find wrong in many ways, but
I'm just going to say that for the case at hand
sizeof(AVSphericalMapping) is not part of the ABI, and so it is
allowed to have different sizes on different architectures.
Same for many other side data objects, this one just happens to use
size_t, which is the right type for expressing a size.

Printing the size of a struct adds absolutely no value to a test, and
changing a type because of a test is a hacky workaround I have no
intention to pursue.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] fate: Do not report side data size

[Ronald S. Bultje]

Hi,

On Wed, Mar 8, 2017 at 9:31 AM, wm4  wrote:

> On Wed, 8 Mar 2017 14:09:53 +0100
> Michael Niedermayer  wrote:
>
> If the size printing is removed then other code should be added
> > to test for the size to match the correct value
>
> Then it would be more reasonable to make av_packet_add_side_data()
> check whether the size is correct for the given side data type.


I think you're both right here, this is a pretty good idea (for fixed-size
side-data types).

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

Re: [FFmpeg-devel] fate: Do not report side data size

[wm4]

On Wed, 8 Mar 2017 14:09:53 +0100
Michael Niedermayer  wrote:

> On Wed, Mar 08, 2017 at 01:12:04PM +0100, wm4 wrote:
> > On Wed, 8 Mar 2017 13:04:11 +0100
> > Michael Niedermayer  wrote:
> >   
> > > On Wed, Mar 08, 2017 at 12:56:31PM +0100, wm4 wrote:  
> > > > On Wed, 8 Mar 2017 12:51:06 +0100
> > > > Michael Niedermayer  wrote:
> > > > 
> > > > > On Wed, Mar 08, 2017 at 12:28:17PM +0100, Hendrik Leppkes wrote:
> > > > > > On Wed, Mar 8, 2017 at 1:17 AM, Vittorio Giovara
> > > > > >  wrote:  
> > > > > > > This should address the mismatch between different archs  
> > > > > 
> > > > > iam not in favor of this solution
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Removing the side_data_size from output should be fine, as its a
> > > > > > implementation detail and as seen here can even vary between
> > > > > > architecture or possibly even compiler.
> > > > > > Maybe someone that uses that ffprobe output more often can comment? 
> > > > > >  
> > > > > 
> > > > > I use all kinds of stuff
> > > > > if something is removed from ffprobes output it wont be tested 
> > > > > anymore.
> > > > > We should test more not less.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > An alternative for fixing fate would be to use fixed size fields in
> > > > > > the new sidedata, although the possibility of it breaking similarly
> > > > > > again in the future then remains.  
> > > > > 
> > > > > I strongly prefer fixed size to be used in side data over platform
> > > > > dependant fields. Not only does size become testable but theres also
> > > > > a platform specific difference less in the interface which should
> > > > > help bug reproducability between platforms
> > > > > 
> > > > > thanks
> > > > > 
> > > > > [...]
> > > > 
> > >   
> > > > So why don't let we fate test e.g. sizeof(AVPacket)? Makes as much
> > > > sense.
> > > 
> > > this is trolling  
> > 
> > Really, I'm not sure if your first post in this thread isn't trolling.
> >   
> > > No change in our code could have sizeof(AVPacket) be "wrong"
> > > but a
> > > int side_data_size
> > > can be wrong easily  
> > 
> > Side datas are generally structs (well, some aren't, but most newer
> > ones). How does it make sense to "test" the sizes of those structs?  
> 
> the "int side_data_size" matches the stuct sizeof only if it was
> set correctly. This field can be read from the main data of a AVPacket
> and in that case can be anything its also possible a simple typo in
> the code could set it incorrectly or linking to a old lib with a
> struct with fewer fields could cause it to be incorrect.

This could happen with anything. Except when a side-data-"merged"
packet was read from a raw file, which I consider a potentially
security relevant issue. I've sent a patch against it.

But in all other cases, do you have any reason to believe that any code
could accidentally set the wrong size in a way that wouldn't just be
normal C undefined behavior (like passing the wrong size to av_malloc)?

> 
> Theres alot that can make the field wrong.

Like what.

> displaying the size in the output and thus testing it is easy and
> usefull IMHO

Well, you still can do that, just not to FATE.

> 
> > 
> > If you want to do it right, then dump the side data contents with
> > ffprobe or whatever.  
> 
> Thats already done for some side data, but peple do not update the code
> when new cases are added. Also until now the size was platform
> independant in practice.

Then just make people add appropriate tests when side data is added.
New codecs, (de)muxers, and filters are also not tested just because
they are added, and need explicit fate tests. Why should it be
different here?

Currently, the side data is (probably) the same on all platforms only
by coincidence. Some structs use types that have no guaranteed fixed
size in the strict portable sense, but they're happen to be the same on
all platforms.

I don't think it's a good idea to rely on ABI coincidences. It's
literally the same as doing file I/O by writing entire structs with
single write/read calls.

In this case, size_t breaks the thing, but that's because size_t is
increasingly going to be used for memory sizes and (in this case) image
dimensions. (Although I find it stupid to use size_t for image
dimensions, but that's a different topic.)

> 
> If the size printing is removed then other code should be added
> to test for the size to match the correct value

Then it would be more reasonable to make av_packet_add_side_data()
check whether the size is correct for the given side data type.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] fate: Do not report side data size

[Michael Niedermayer]

On Wed, Mar 08, 2017 at 01:12:04PM +0100, wm4 wrote:
> On Wed, 8 Mar 2017 13:04:11 +0100
> Michael Niedermayer  wrote:
> 
> > On Wed, Mar 08, 2017 at 12:56:31PM +0100, wm4 wrote:
> > > On Wed, 8 Mar 2017 12:51:06 +0100
> > > Michael Niedermayer  wrote:
> > >   
> > > > On Wed, Mar 08, 2017 at 12:28:17PM +0100, Hendrik Leppkes wrote:  
> > > > > On Wed, Mar 8, 2017 at 1:17 AM, Vittorio Giovara
> > > > >  wrote:
> > > > > > This should address the mismatch between different archs
> > > > 
> > > > iam not in favor of this solution
> > > > 
> > > >   
> > > > > 
> > > > > Removing the side_data_size from output should be fine, as its a
> > > > > implementation detail and as seen here can even vary between
> > > > > architecture or possibly even compiler.
> > > > > Maybe someone that uses that ffprobe output more often can comment?   
> > > > >  
> > > > 
> > > > I use all kinds of stuff
> > > > if something is removed from ffprobes output it wont be tested anymore.
> > > > We should test more not less.
> > > > 
> > > >   
> > > > > 
> > > > > An alternative for fixing fate would be to use fixed size fields in
> > > > > the new sidedata, although the possibility of it breaking similarly
> > > > > again in the future then remains.
> > > > 
> > > > I strongly prefer fixed size to be used in side data over platform
> > > > dependant fields. Not only does size become testable but theres also
> > > > a platform specific difference less in the interface which should
> > > > help bug reproducability between platforms
> > > > 
> > > > thanks
> > > > 
> > > > [...]  
> > >   
> > 
> > > So why don't let we fate test e.g. sizeof(AVPacket)? Makes as much
> > > sense.  
> > 
> > this is trolling
> 
> Really, I'm not sure if your first post in this thread isn't trolling.
> 
> > No change in our code could have sizeof(AVPacket) be "wrong"
> > but a
> > int side_data_size
> > can be wrong easily
> 
> Side datas are generally structs (well, some aren't, but most newer
> ones). How does it make sense to "test" the sizes of those structs?

the "int side_data_size" matches the stuct sizeof only if it was
set correctly. This field can be read from the main data of a AVPacket
and in that case can be anything its also possible a simple typo in
the code could set it incorrectly or linking to a old lib with a
struct with fewer fields could cause it to be incorrect.

Theres alot that can make the field wrong.
displaying the size in the output and thus testing it is easy and
usefull IMHO


> 
> If you want to do it right, then dump the side data contents with
> ffprobe or whatever.

Thats already done for some side data, but peple do not update the code
when new cases are added. Also until now the size was platform
independant in practice.

If the size printing is removed then other code should be added
to test for the size to match the correct value


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates


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

Re: [FFmpeg-devel] fate: Do not report side data size

[wm4]

On Wed, 8 Mar 2017 13:04:11 +0100
Michael Niedermayer  wrote:

> On Wed, Mar 08, 2017 at 12:56:31PM +0100, wm4 wrote:
> > On Wed, 8 Mar 2017 12:51:06 +0100
> > Michael Niedermayer  wrote:
> >   
> > > On Wed, Mar 08, 2017 at 12:28:17PM +0100, Hendrik Leppkes wrote:  
> > > > On Wed, Mar 8, 2017 at 1:17 AM, Vittorio Giovara
> > > >  wrote:
> > > > > This should address the mismatch between different archs
> > > 
> > > iam not in favor of this solution
> > > 
> > >   
> > > > 
> > > > Removing the side_data_size from output should be fine, as its a
> > > > implementation detail and as seen here can even vary between
> > > > architecture or possibly even compiler.
> > > > Maybe someone that uses that ffprobe output more often can comment?
> > > 
> > > I use all kinds of stuff
> > > if something is removed from ffprobes output it wont be tested anymore.
> > > We should test more not less.
> > > 
> > >   
> > > > 
> > > > An alternative for fixing fate would be to use fixed size fields in
> > > > the new sidedata, although the possibility of it breaking similarly
> > > > again in the future then remains.
> > > 
> > > I strongly prefer fixed size to be used in side data over platform
> > > dependant fields. Not only does size become testable but theres also
> > > a platform specific difference less in the interface which should
> > > help bug reproducability between platforms
> > > 
> > > thanks
> > > 
> > > [...]  
> >   
> 
> > So why don't let we fate test e.g. sizeof(AVPacket)? Makes as much
> > sense.  
> 
> this is trolling

Really, I'm not sure if your first post in this thread isn't trolling.

> No change in our code could have sizeof(AVPacket) be "wrong"
> but a
> int side_data_size
> can be wrong easily

Side datas are generally structs (well, some aren't, but most newer
ones). How does it make sense to "test" the sizes of those structs?

If you want to do it right, then dump the side data contents with
ffprobe or whatever.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] fate: Do not report side data size

[Michael Niedermayer]

On Wed, Mar 08, 2017 at 12:56:31PM +0100, wm4 wrote:
> On Wed, 8 Mar 2017 12:51:06 +0100
> Michael Niedermayer  wrote:
> 
> > On Wed, Mar 08, 2017 at 12:28:17PM +0100, Hendrik Leppkes wrote:
> > > On Wed, Mar 8, 2017 at 1:17 AM, Vittorio Giovara
> > >  wrote:  
> > > > This should address the mismatch between different archs  
> > 
> > iam not in favor of this solution
> > 
> > 
> > > 
> > > Removing the side_data_size from output should be fine, as its a
> > > implementation detail and as seen here can even vary between
> > > architecture or possibly even compiler.
> > > Maybe someone that uses that ffprobe output more often can comment?  
> > 
> > I use all kinds of stuff
> > if something is removed from ffprobes output it wont be tested anymore.
> > We should test more not less.
> > 
> > 
> > > 
> > > An alternative for fixing fate would be to use fixed size fields in
> > > the new sidedata, although the possibility of it breaking similarly
> > > again in the future then remains.  
> > 
> > I strongly prefer fixed size to be used in side data over platform
> > dependant fields. Not only does size become testable but theres also
> > a platform specific difference less in the interface which should
> > help bug reproducability between platforms
> > 
> > thanks
> > 
> > [...]
> 

> So why don't let we fate test e.g. sizeof(AVPacket)? Makes as much
> sense.

this is trolling

No change in our code could have sizeof(AVPacket) be "wrong"
but a
int side_data_size
can be wrong easily


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell


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

Re: [FFmpeg-devel] fate: Do not report side data size

[wm4]

On Wed, 8 Mar 2017 12:51:06 +0100
Michael Niedermayer  wrote:

> On Wed, Mar 08, 2017 at 12:28:17PM +0100, Hendrik Leppkes wrote:
> > On Wed, Mar 8, 2017 at 1:17 AM, Vittorio Giovara
> >  wrote:  
> > > This should address the mismatch between different archs  
> 
> iam not in favor of this solution
> 
> 
> > 
> > Removing the side_data_size from output should be fine, as its a
> > implementation detail and as seen here can even vary between
> > architecture or possibly even compiler.
> > Maybe someone that uses that ffprobe output more often can comment?  
> 
> I use all kinds of stuff
> if something is removed from ffprobes output it wont be tested anymore.
> We should test more not less.
> 
> 
> > 
> > An alternative for fixing fate would be to use fixed size fields in
> > the new sidedata, although the possibility of it breaking similarly
> > again in the future then remains.  
> 
> I strongly prefer fixed size to be used in side data over platform
> dependant fields. Not only does size become testable but theres also
> a platform specific difference less in the interface which should
> help bug reproducability between platforms
> 
> thanks
> 
> [...]

So why don't let we fate test e.g. sizeof(AVPacket)? Makes as much
sense.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] fate: Do not report side data size

[Michael Niedermayer]

On Wed, Mar 08, 2017 at 12:28:17PM +0100, Hendrik Leppkes wrote:
> On Wed, Mar 8, 2017 at 1:17 AM, Vittorio Giovara
>  wrote:
> > This should address the mismatch between different archs

iam not in favor of this solution


> 
> Removing the side_data_size from output should be fine, as its a
> implementation detail and as seen here can even vary between
> architecture or possibly even compiler.
> Maybe someone that uses that ffprobe output more often can comment?

I use all kinds of stuff
if something is removed from ffprobes output it wont be tested anymore.
We should test more not less.


> 
> An alternative for fixing fate would be to use fixed size fields in
> the new sidedata, although the possibility of it breaking similarly
> again in the future then remains.

I strongly prefer fixed size to be used in side data over platform
dependant fields. Not only does size become testable but theres also
a platform specific difference less in the interface which should
help bug reproducability between platforms

thanks

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides


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

Re: [FFmpeg-devel] fate: Do not report side data size

[Hendrik Leppkes]

On Wed, Mar 8, 2017 at 1:17 AM, Vittorio Giovara
 wrote:
> This should address the mismatch between different archs

Removing the side_data_size from output should be fine, as its a
implementation detail and as seen here can even vary between
architecture or possibly even compiler.
Maybe someone that uses that ffprobe output more often can comment?

An alternative for fixing fate would be to use fixed size fields in
the new sidedata, although the possibility of it breaking similarly
again in the future then remains.

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