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

fix(ios): New method allowing to disable automated memory release on memoryWarning (CB-11409) #101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tbrebant
Copy link

Platforms affected

iOS

What does this PR do?

It is adding a method allowing the user to disable (and re-enable) the automated resource releasing when a memory warning is received on iOS. Why? Because user may want to handle memory warnings by himself (such as releasing other less important resource first).

This method is static, on the class:

Media.shouldReleaseOnMemoryWarning(false);
// Here comes custom code to handle memoryWarnings

What testing has been done on this change?

  • Played multiple (non streaming) audio files after having disabled the auto-release and while having some heavy memory process running, until a memoryWarning is received: audio continue to play.
  • Played multiple (non streaming) audio files with the auto-release enabled and while having some heavy memory process running, until a memoryWarning is received: audio stops.

Ran the integrated tests. Before doing any change (version 2.3.1-dev) the result was:

cordova-media-plugin 2 3 1-dev

After the change the result is exactly the same:

img_0194

All tests was made with an iPad2 on iOS8.4.

Checklist

  • ICLA has been signed and submitted to [email protected].
  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

About the last point: tests will come as soon as possible, meanwhile please have a look at the code and review it! Thank you.

@cordova-qa
Copy link

Cordova CI Build has one or more failures.

Commit - Link
Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 8.1 Store Link Link Link
Windows 10 Store Link Link Link
Windows 8.1 Phone Link Link Link
iOS Link Link Link
Android Link Link Link

@tbrebant
Copy link
Author

What about this? Would it be possible to have a comment or an opinion?

@elgerlambert
Copy link

Hi,

Just dropping by to say that the current behaviour (stopping the music as soon as a memory warning is received) poses an issue for us as well. Having the music stop is the very last thing we want to do.

@katzlbt
Copy link

katzlbt commented Nov 5, 2016

Please check if PullRequest #120 will resolve your issue. I think, the user should not be involved in this issue, especially as it only affects iOS. Releasing playing/recording media is unnecessary in the first place.

@janpio
Copy link
Member

janpio commented Sep 16, 2018

Hey @tbrebant, seems there is a merge conflict now. Could you please take care of this? Thanks.

@janpio janpio added the feature label Sep 17, 2018
@janpio janpio changed the title CB-11409: (iOS) New method allowing to disable automated memory release on memoryWarning fix(ios): New method allowing to disable automated memory release on memoryWarning (CB-11409) Jul 3, 2019
Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard conflict now, will have to rebase and see if mergeable.
Possibly similar feature was merged in the meantime, so should be checked if still necessary.

@katzlbt
Copy link

katzlbt commented Jul 4, 2019

The issue addressed in this PR was fixed by: #120
This PR can be dropped.

@janpio
Copy link
Member

janpio commented Jul 4, 2019

Thanks for the feedback @katzlbt.

If I understand #120 correctly, that PR changed the way the memory pressure warning is handled to only drop non playing sounds. This PR would have added an option to disable the dropping completely - correct? There is no possible other need for the functionality contained in this PR?

(Sorry if my question are a bit basic here, I am not super accustomed to this plugin and iOS dev in general)

@katzlbt
Copy link

katzlbt commented Jul 4, 2019

Yes your assessment is correct, and there is no other functionality contained in the PR. The problem addressed by this PR is already fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants