search

rust-mobile / ndk

Rust bindings to the Android NDK
Apache License 2.0
1.11k stars 110 forks source link

Investigate `MediaFormat` mutability / lifetimes #451

Closed MarijnS95 closed 7 months ago

MarijnS95 commented 9 months ago

Barely any of the MediaFormat functions from the "safe" MediaCodec PR #216 take self as mut, even though there are quite a few functions that return borrows from pointers which are very likely invalidated when the MediaFormat is modified in some way. Even "strict getters" have the ability to invalidate previously returned results and should be considered mut for that case.

See for example toString() which is mentioned in https://github.com/rust-mobile/ndk/pull/450:

The returned string is owned by the format, and remains valid until the next call to toString, or until the format is deleted.

Fortunately this function is not directly exposed to users, only via Display which copies the resulting string via write_str() in the same function where AMediaFormat_toString() is called.

getString() has the same reentrancy limitation:

The returned string is owned by the format, and remains valid until the next call to getString, or until the format is deleted.

getBuffer() strangely is not documented to be invalidated on repeated invocations, only when it (obviously) is removed from the MediaFormat:

The returned data is owned by the format and remains valid as long as the named entry is part of the format.

Note that I'm implying for both strings and buffers that they are invalidated when the same key is set to a different string/buffer/value (and maybe even when another key is changed).

Given these notions I think we should make all set functions, _and fn string() mut.

CC @zarik5 @spencercw in case I'm missing something in my observation.

spencercw commented 9 months ago

I think you're right. Good spot on the string thing, that's a bit of a footgun.

MarijnS95 commented 9 months ago

Thanks @spencercw! Mind reviewing the linked PR?

Note that I've asked for some extra clarification upstream, as I can think of way too many loopholes with these functions that clobber our implementation in Rust:

https://issuetracker.google.com/issues/300602767#comment9

spencercw commented 9 months ago

Might be worth perusing the Android source code which should answer your questions. Doesn't help that these details are not documented of course.

MarijnS95 commented 9 months ago

@spencercw while it does answer our questions, I prefer to leave source-code-diving as an exercise to the AOSP developers, and have them set these implicit assumptions in stone by describing it more clearly in a documentation field.