[FFmpeg-devel] [PATCH] avformat/utils: add helper functions to retrieve index entries from an AVStream

[Marvin Scholz]

On 25 Mar 2021, at 12:55, Nicolas George wrote:

> James Almer (12021-03-24):
>> I think it's clear by now that nothing i could say will convince you it's
>> better to not return a pointer to an internal array when there are safer
>> alternatives, and i already gave my reasons why, none of which satisfied
>> you, so i don't see the point in keeping this discussion going.
>
> I find this comment quite offensive. You did not manage to convince me
> because your arguments have not been up to the task. Do not try to push
> the fault on me, and I will refrain from accusing you of not taking my
> arguments into account. Coming to an agreement is a process, it requires
> both parts to refine their arguments progressively.
>
> This is a matter of choosing the least of several drawbacks. So let us
> compare the drawbacks and not muddle things further.
>
> For me:
>
> 1. having a dynamic allocation is way way worse than
> 2. having sizeof(AVIndexEntry) in the ABI, which is somewhat worse than
> 3. having a function with many arguments, which is a tiny bit worse than
> 4. having a "use this pointer immediately" constraint.
>
> We agree except on 3>4, so let us focus on that.
>
> Option (3) has these practical drawbacks: Many arguments involves more
> typing and the risk of messing with the order and getting invalid
> values. It also requires redesigning the API if we add fields and
> exporting them is useful. And it requires either the overhead of NULL
> checks or the caller declaring unneeded variables.
>
> Option (4) has the obvious practical drawback that misusing the API
> causes undefined behavior.
>
> The constraint of using a pointer immediately on risk of undefined
> behavior is actually a frequent one, in FFmpeg but also in C at large:
> gethosbtyname, localtime, etc.
>
> For me, that makes it approximately on par with the risk of messing the
> order of the many arguments.
>
> Which leaves more typing, NULL checks overhead or useless variables
> (still more typing).
>
> It is tiny, I have no trouble admitting, but it is tiny in favor of one
> solution.
>
> If you do not agree with these estimates, please explain exactly where.

I disagree with your ordering too,
for me 4. is clearly worse than 3. here, as the harm that can be
done by mixing up arguments (in this case) is less than use of a
possibly invalid pointer. And mixed up arguments would probably be
noticed easier when testing than reads of possibly invalid memory.

Even when documenting the constraint of immediately using the pointer,
it could easily be overlooked/forgotten.
It does not even has to be me forgetting the constraint, but could be
someone else changing the code in the future, adding some API call
before the pointer is used, unaware of the possible consequences and
that could invalidate the pointer (if I understand James explanation
correctly). This could go unnoticed easily.

So IMO having a function with many arguments seems like a better and
safer tradeoff in this case to me…

>
>> If some other developer wants to chime in and comment which approach they
>> prefer, then that would be ideal.
>
> Indeed.
>
> Regards,
>
> -- 
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".