-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Investigate MediaFormat
mutability / lifetimes
#451
Comments
All setters can inherently change the internals of the format, and should furthermore invalidate - or not be allowed while - borrowed strings or buffers (are) acquired from it. Unforatunately [`getString()` is documented as]: The returned string is owned by the format, and remains valid until the next call to getString, or until the format is deleted. Meaning that it is not reentrant, and invalidates any data previously acquired from `str()`; hence this getter is marked as `&mut self` too. The same is however not the case for `getBuffer()`. [`getString()` is documented as]: #451
I think you're right. Good spot on the string thing, that's a bit of a footgun. |
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: |
Might be worth perusing the Android source code which should answer your questions. Doesn't help that these details are not documented of course. |
@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. |
All setters can inherently change the internals of the format, and should furthermore invalidate - or not be allowed while - borrowed strings or buffers (are) acquired from it. Unforatunately [`getString()` is documented as]: The returned string is owned by the format, and remains valid until the next call to getString, or until the format is deleted. Meaning that it is not reentrant, and invalidates any data previously acquired from `str()`; hence this getter is marked as `&mut self` too. The same is however not the case for `getBuffer()`. [`getString()` is documented as]: #451
All setters can inherently change the internals of the format, and should furthermore invalidate - or not be allowed while - borrowed strings or buffers (are) acquired from it. Unforatunately [`getString()` is documented as]: The returned string is owned by the format, and remains valid until the next call to getString, or until the format is deleted. Meaning that it is not reentrant, and invalidates any data previously acquired from `str()`; hence this getter is marked as `&mut self` too. The same is however not the case for `getBuffer()`. [`getString()` is documented as]: #451
…452) All setters can inherently change the internals of the format, and should furthermore invalidate - or not be allowed while - borrowed strings or buffers (are) acquired from it. Unforatunately [`getString()` is documented as]: The returned string is owned by the format, and remains valid until the next call to getString, or until the format is deleted. Meaning that it is not reentrant, and invalidates any data previously acquired from `str()`; hence this getter is marked as `&mut self` too. The same is however not the case for `getBuffer()`. [`getString()` is documented as]: #451
Barely any of the
MediaFormat
functions from the "safe"MediaCodec
PR #216 takeself
asmut
, even though there are quite a few functions that return borrows from pointers which are very likely invalidated when theMediaFormat
is modified in some way. Even "strict getters" have the ability to invalidate previously returned results and should be consideredmut
for that case.See for example
toString()
which is mentioned in #450:Fortunately this function is not directly exposed to users, only via
Display
which copies the resulting string viawrite_str()
in the same function whereAMediaFormat_toString()
is called.getString()
has the same reentrancy limitation:getBuffer()
strangely is not documented to be invalidated on repeated invocations, only when it (obviously) is removed from theMediaFormat
: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, _andfn string()
mut
.CC @zarik5 @spencercw in case I'm missing something in my observation.
The text was updated successfully, but these errors were encountered: