{"action":"created","issue":{"url":"https://api.github.com/repos/google/oboe/issues/140","repository_url":"https://api.github.com/repos/google/oboe","labels_url":"https://api.github.com/repos/google/oboe/issues/140/labels{/name}","comments_url":"https://api.github.com/repos/google/oboe/issues/140/comments","events_url":"https://api.github.com/repos/google/oboe/issues/140/events","html_url":"https://github.com/google/oboe/pull/140","id":336678221,"node_id":"MDExOlB1bGxSZXF1ZXN0MTk4MDc0MTU2","number":140,"title":"Refactoring AudioStream::getBufferSizeInFrames to return a ResultWithValue","user":{"login":"dturner","id":873212,"node_id":"MDQ6VXNlcjg3MzIxMg==","avatar_url":"https://avatars1.githubusercontent.com/u/873212?v=4","gravatar_id":"","url":"https://api.github.com/users/dturner","html_url":"https://github.com/dturner","followers_url":"https://api.github.com/users/dturner/followers","following_url":"https://api.github.com/users/dturner/following{/other_user}","gists_url":"https://api.github.com/users/dturner/gists{/gist_id}","starred_url":"https://api.github.com/users/dturner/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dturner/subscriptions","organizations_url":"https://api.github.com/users/dturner/orgs","repos_url":"https://api.github.com/users/dturner/repos","events_url":"https://api.github.com/users/dturner/events{/privacy}","received_events_url":"https://api.github.com/users/dturner/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"dturner","id":873212,"node_id":"MDQ6VXNlcjg3MzIxMg==","avatar_url":"https://avatars1.githubusercontent.com/u/873212?v=4","gravatar_id":"","url":"https://api.github.com/users/dturner","html_url":"https://github.com/dturner","followers_url":"https://api.github.com/users/dturner/followers","following_url":"https://api.github.com/users/dturner/following{/other_user}","gists_url":"https://api.github.com/users/dturner/gists{/gist_id}","starred_url":"https://api.github.com/users/dturner/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dturner/subscriptions","organizations_url":"https://api.github.com/users/dturner/orgs","repos_url":"https://api.github.com/users/dturner/repos","events_url":"https://api.github.com/users/dturner/events{/privacy}","received_events_url":"https://api.github.com/users/dturner/received_events","type":"User","site_admin":false},"assignees":[{"login":"dturner","id":873212,"node_id":"MDQ6VXNlcjg3MzIxMg==","avatar_url":"https://avatars1.githubusercontent.com/u/873212?v=4","gravatar_id":"","url":"https://api.github.com/users/dturner","html_url":"https://github.com/dturner","followers_url":"https://api.github.com/users/dturner/followers","following_url":"https://api.github.com/users/dturner/following{/other_user}","gists_url":"https://api.github.com/users/dturner/gists{/gist_id}","starred_url":"https://api.github.com/users/dturner/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dturner/subscriptions","organizations_url":"https://api.github.com/users/dturner/orgs","repos_url":"https://api.github.com/users/dturner/repos","events_url":"https://api.github.com/users/dturner/events{/privacy}","received_events_url":"https://api.github.com/users/dturner/received_events","type":"User","site_admin":false}],"milestone":null,"comments":2,"created_at":"2018-06-28T15:47:14Z","updated_at":"2018-06-29T15:55:55Z","closed_at":null,"author_association":"COLLABORATOR","pull_request":{"url":"https://api.github.com/repos/google/oboe/pulls/140","html_url":"https://github.com/google/oboe/pull/140","diff_url":"https://github.com/google/oboe/pull/140.diff","patch_url":"https://github.com/google/oboe/pull/140.patch"},"body":"I started updating the `AudioStream::get*` methods to return a `ResultWithValue` based on the assumption that we need to handle the case when the stream is null. \r\n\r\nI really don't like this change because of the extra work and noise which is creates when calling the API. \r\n\r\nIn particular, take a look at code changes in [`PlayAudioEngine.cpp`](https://github.com/google/oboe/pull/140/commits/e513142b9009be9435f406c0b6459023dcada7a6#diff-0cab96b97d755d2a27561725775b00e0). Getting and setting the buffer size becomes an arduous job of checking the result of every call. \r\n\r\nThe thing which sticks in my mind is that if the stream were null you wouldn't be able to call `pStream->getBufferSizeInFrames()` anyway, so it's pointless to return an `Result::ErrorNull`.\r\n\r\nHaving tried this API change I would advocate for a solution where we (somehow) guarantee (either in code or in documentation) that the AudioStream is never null. Thoughts? "},"comment":{"url":"https://api.github.com/repos/google/oboe/issues/comments/401397463","html_url":"https://github.com/google/oboe/pull/140#issuecomment-401397463","issue_url":"https://api.github.com/repos/google/oboe/issues/140","id":401397463,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMTM5NzQ2Mw==","user":{"login":"dturner","id":873212,"node_id":"MDQ6VXNlcjg3MzIxMg==","avatar_url":"https://avatars1.githubusercontent.com/u/873212?v=4","gravatar_id":"","url":"https://api.github.com/users/dturner","html_url":"https://github.com/dturner","followers_url":"https://api.github.com/users/dturner/followers","following_url":"https://api.github.com/users/dturner/following{/other_user}","gists_url":"https://api.github.com/users/dturner/gists{/gist_id}","starred_url":"https://api.github.com/users/dturner/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dturner/subscriptions","organizations_url":"https://api.github.com/users/dturner/orgs","repos_url":"https://api.github.com/users/dturner/repos","events_url":"https://api.github.com/users/dturner/events{/privacy}","received_events_url":"https://api.github.com/users/dturner/received_events","type":"User","site_admin":false},"created_at":"2018-06-29T15:55:55Z","updated_at":"2018-06-29T15:55:55Z","author_association":"COLLABORATOR","body":"> OpenSL ES might return UNIMPLEMENTED\r\n\r\nRight, there's 2 separate issues here:\r\n\r\n- calling methods which are not implemented on the underlying API like `getTimestamp` \r\n- calling methods when the underlying stream is null \r\n\r\nI think the first isn't a problem because the affected methods already return a `Result` of some sort. [Spreadsheet AudioStream methods](https://docs.google.com/spreadsheets/d/1VdLpxCuKCxwrdK1-XVfZRVSDH4dTOSLh6jyLjxgjn9k/edit?usp=sharing), and the client can call `isLatencyDetectionSupported` to check first if they want. \r\n\r\nThe null stream is the bigger issue\r\n\r\n> One solution might be to say that the Oboe getter return values are undefined when the stream is closed. So it would be up to the app not to do that. We could just return zero. Or we could cache the last valid value and just return it after the stream is closed.\r\n\r\nWhy not both :) We cache the getter return values *and* say that the values are undefined after `requestClose` has been called.  \r\n"}}
{"actor":{"display_login":"dturner"}}