Skip to content
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

Closed
MarijnS95 opened this issue Nov 20, 2023 · 4 comments · Fixed by #452
Closed

Investigate MediaFormat mutability / lifetimes #451

MarijnS95 opened this issue Nov 20, 2023 · 4 comments · Fixed by #452
Labels
impact: breaking API/ABI-breaking change status: needs investigation Issue must be confirmed and researched

Comments

@MarijnS95
Copy link
Member

MarijnS95 commented Nov 20, 2023

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 #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.

@MarijnS95 MarijnS95 added status: needs investigation Issue must be confirmed and researched impact: breaking API/ABI-breaking change labels Nov 20, 2023
MarijnS95 added a commit that referenced this issue Nov 20, 2023
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
@spencercw
Copy link
Contributor

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

@MarijnS95
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

@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.

MarijnS95 added a commit that referenced this issue Nov 23, 2023
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
MarijnS95 added a commit that referenced this issue Jan 27, 2024
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
MarijnS95 added a commit that referenced this issue Jan 27, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: breaking API/ABI-breaking change status: needs investigation Issue must be confirmed and researched
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants