On Mon, 29 Oct 2018, Derek Buitenhuis wrote:
On 29/10/2018 14:10, Martin Storsjö wrote:
I don't understand why this is being used in favour of a proper
pointer field? An integer field is just ascting to be misused.
Even the doxygen is really sketchy on it.
It's essentially meant to be used as union { ptr; int64_t } assuming you
don't have pointers larger than 64 bits.
It's not a union in the API, and I'm pretty sure that it violates the C spec
to use a unions to get an integer out of a pointer, shove it into an int64_t,
and then get it back out, and chnage it back via union. Especially for
32-bit pointers. It encourages terrible code.
I just don't think we should revive this as-is purely for convenience.
I also don't understand why this is at the AVCodecContext level
and not packet/frame?
It is on the frame level, but not in the packet struct (probably for
historical reasons) - instead of in the packet, it's in AVCodecContext.
For decoding, you set the value in AVCodecContext before feeding packets
to it, and get the corresponding value reordered into the output AVFrame.
If things were to be redone from scratch, moving it into AVPacket would
probably make more sense, but there's not much point in doing that right
now.
I mean, this is pretty gross, and non-obvious as far as I'm concerned.
Modifying the AVCodecContext on every call is just... eugh.
At some point, the doxygen got markers saying this mechanism was
deprecated and one should use the new pkt_pts instead. Before that,
reordered_opaque was mainly used for getting reordered pts as there
was no other mechanism for it.
But even with the proper pkt_pts field, having a generic opaque field that
travels along with the reordering is useful, which is why the deprecation
doxygen comments were removed in ad1ee5fa7. But that commit just missed to
remove one of the doxygen deprecation.
I agree it's very useful, and something we should have, but not that we should
revive/use this partiular field... it's nasty.
Sorry, I think you've misunderstood this patch altogether.
It's not about reviving this field or not, it's all in full use
already. It was never deprecated with any active plan to remove it, the
only steps were a few doxygen comments, never any attributes that would
actually prompt action.
And a few years later someone noticed that these doxygen comments didn't
match up with reality, and it was decided (with no objections on either
project) that these really shouldn't be deprecated as it is the only
actual mechanism we have for doing exactly this.
It's just that the undeprecation commit, ad1ee5fa7, missed one field. And
the one I'm removing the stray deprecation comment from, is the very
properly placed one in AVFrame non the less.
// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
