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

[TIMOB-7137] Implementation of Ti.Media.Sound #3801

Closed
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@edgithub
Contributor

edgithub commented Jan 28, 2013

Proposed implementation of Ti.Media.Sound using HTML5 (backport from the Tizen repo).

Jira ticket: https://jira.appcelerator.org/browse/TIMOB-7137

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Jan 28, 2013

Contributor

Note: there are numerous coding standard violations in this PR that need to be fixed. Refer to https://wiki.appcelerator.org/display/guides2/JavaScript+Coding+Standards

Contributor

nebrius commented Jan 28, 2013

Note: there are numerous coding standard violations in this PR that need to be fixed. Refer to https://wiki.appcelerator.org/display/guides2/JavaScript+Coding+Standards

@edgithub

This comment has been minimized.

Show comment
Hide comment
@edgithub

edgithub Jan 29, 2013

Contributor

Please consider our latest modifications.

Contributor

edgithub commented Jan 29, 2013

Please consider our latest modifications.

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 7, 2013

Contributor

Setting time, looping, etc before the sound is loaded shouldn't be reset after the sound is loaded

Contributor

nebrius commented Feb 7, 2013

Setting time, looping, etc before the sound is loaded shouldn't be reset after the sound is loaded

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 7, 2013

Contributor

After release()'ing the sound and then setUrl()'ing the url, nothing happens (play does nothing). Tested in chrome desktop

Contributor

nebrius commented Feb 7, 2013

After release()'ing the sound and then setUrl()'ing the url, nothing happens (play does nothing). Tested in chrome desktop

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 7, 2013

Contributor

Crashes on Android 2.3.7 on startup. I attached the sample app I'm using to the JIRA ticket

Contributor

nebrius commented Feb 7, 2013

Crashes on Android 2.3.7 on startup. I attached the sample app I'm using to the JIRA ticket

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 7, 2013

Contributor

Also, you should add support for multiple URLs, similar to how Ti.Media.VideoPlayer does to support multiple codecs

Contributor

nebrius commented Feb 7, 2013

Also, you should add support for multiple URLs, similar to how Ti.Media.VideoPlayer does to support multiple codecs

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 7, 2013

Contributor

Ti/Media/Sound.js needs to be cached like the other files by updating the dependency map

Contributor

nebrius commented Feb 7, 2013

Ti/Media/Sound.js needs to be cached like the other files by updating the dependency map

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 7, 2013

Contributor

Sometimes crashes on startup in iOS.

Contributor

nebrius commented Feb 7, 2013

Sometimes crashes on startup in iOS.

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 7, 2013

Contributor

Duration is always 0 on Windows Phone 8

Contributor

nebrius commented Feb 7, 2013

Duration is always 0 on Windows Phone 8

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 7, 2013

Contributor

Stop doesn't reset the position in Windows phone 8

Contributor

nebrius commented Feb 7, 2013

Stop doesn't reset the position in Windows phone 8

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 7, 2013

Contributor

Duration doesn't show up until the file has started playing on Blackberry 7

Contributor

nebrius commented Feb 7, 2013

Duration doesn't show up until the file has started playing on Blackberry 7

edgithub added some commits Feb 8, 2013

- updated dependency map
- changed property paused (read-write)
- fixed url/setUrl
- changed duration (sec)
@edgithub

This comment has been minimized.

Show comment
Hide comment
@edgithub

edgithub Feb 8, 2013

Contributor

'paused' property is not read-only

changed

duration should be in seconds, not ms (ios takes precedence)

changed

Setting time, looping, etc before the sound is loaded shouldn't be reset after the sound is loaded

  • Setting time : fixed, but we reset the time after changing an url. Looping and other properties are currently not reset after setting a new url.

After release()'ing the sound and then setUrl()'ing the url, nothing happens (play does nothing). Tested in chrome desktop

Fixed.

Ti/Media/Sound.js needs to be cached like the other files by updating the dependency map

Fixed.

Also, you should add support for multiple URLs, similar to how Ti.Media.VideoPlayer does to support multiple codecs

Supported

Duration is always 0 on Windows Phone 8
Stop doesn't reset the position in Windows phone 8
Duration doesn't show up until the file has started playing on Blackberry 7

We cannot test on Windows Phone 8 and Blackberry,
only on Android and iOS (for project scope reasons).
We suggest that appropriate tickets are opened at these platforms,
after/if the PR is accepted

Contributor

edgithub commented Feb 8, 2013

'paused' property is not read-only

changed

duration should be in seconds, not ms (ios takes precedence)

changed

Setting time, looping, etc before the sound is loaded shouldn't be reset after the sound is loaded

  • Setting time : fixed, but we reset the time after changing an url. Looping and other properties are currently not reset after setting a new url.

After release()'ing the sound and then setUrl()'ing the url, nothing happens (play does nothing). Tested in chrome desktop

Fixed.

Ti/Media/Sound.js needs to be cached like the other files by updating the dependency map

Fixed.

Also, you should add support for multiple URLs, similar to how Ti.Media.VideoPlayer does to support multiple codecs

Supported

Duration is always 0 on Windows Phone 8
Stop doesn't reset the position in Windows phone 8
Duration doesn't show up until the file has started playing on Blackberry 7

We cannot test on Windows Phone 8 and Blackberry,
only on Android and iOS (for project scope reasons).
We suggest that appropriate tickets are opened at these platforms,
after/if the PR is accepted

@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 9, 2013

Contributor

Sorry, I didn't have time to do another FR round today. Two comments though:

  1. Blackberry and Windows Phone support are required for this PR to be accepted
  2. time should not be reset after changing the URL unless the previous time is longer than the duration, in which case the time should be set to the duration (duration will always be changed of course)

I also briefly peeked at the AudioPlayer PR and I noticed that there is a lot of duplicated code from Sound. You need to refactor these so that either AudioPlayer extends Sound or a common base class is implemented in Ti/_/Media that AudioPlayer and Sound both extend (the latter being preferable).

Remember, code size is the single most important performance metric in mobile web. You should always be trying to find ways to reduce code size. If you can do this, it will help the PR process will go faster.

FYI I'm going to be out of town for a few days, so I won't be able to get back to this PR until Wednesday, February 13th

Contributor

nebrius commented Feb 9, 2013

Sorry, I didn't have time to do another FR round today. Two comments though:

  1. Blackberry and Windows Phone support are required for this PR to be accepted
  2. time should not be reset after changing the URL unless the previous time is longer than the duration, in which case the time should be set to the duration (duration will always be changed of course)

I also briefly peeked at the AudioPlayer PR and I noticed that there is a lot of duplicated code from Sound. You need to refactor these so that either AudioPlayer extends Sound or a common base class is implemented in Ti/_/Media that AudioPlayer and Sound both extend (the latter being preferable).

Remember, code size is the single most important performance metric in mobile web. You should always be trying to find ways to reduce code size. If you can do this, it will help the PR process will go faster.

FYI I'm going to be out of town for a few days, so I won't be able to get back to this PR until Wednesday, February 13th

@edgithub

This comment has been minimized.

Show comment
Hide comment
@edgithub

edgithub Feb 18, 2013

Contributor
  • implemented common base class in Ti/_/Media/Player
  • issues related to 'duration'
Contributor

edgithub commented Feb 18, 2013

  • implemented common base class in Ti/_/Media/Player
  • issues related to 'duration'
@nebrius

This comment has been minimized.

Show comment
Hide comment
@nebrius

nebrius Feb 19, 2013

Contributor

Player should be renamed to something else since VideoPlayer doesn't use it. I would say Ti/_/Media/Audio

Contributor

nebrius commented Feb 19, 2013

Player should be renamed to something else since VideoPlayer doesn't use it. I would say Ti/_/Media/Audio

d === Infinity || (this.constants.__values__.duration = Math.floor(d));
},
_loadedmetadata: function() {

This comment has been minimized.

@nebrius

nebrius Feb 19, 2013

Contributor

The next several functions just call a single function in return, which is a waste. Just call directly

@nebrius

nebrius Feb 19, 2013

Contributor

The next several functions just call a single function in return, which is a waste. Just call directly

This comment has been minimized.

@edgithub

edgithub Mar 29, 2013

Contributor

Done

@edgithub

edgithub Mar 29, 2013

Contributor

Done

@edgithub

This comment has been minimized.

Show comment
Hide comment
@edgithub

edgithub Mar 29, 2013

Contributor
  • renamed Player.js to Audio.js
  • removed empty functions and rearchitected
  • reworked 'if statement'
  • reworked 'return statement'
Contributor

edgithub commented Mar 29, 2013

  • renamed Player.js to Audio.js
  • removed empty functions and rearchitected
  • reworked 'if statement'
  • reworked 'return statement'

@edgithub edgithub closed this Mar 29, 2013

@edgithub edgithub reopened this Mar 29, 2013

@ingo

This comment has been minimized.

Show comment
Hide comment
@ingo

ingo Feb 27, 2014

Member

Thank you for the suggested implementation. If we decide to use any part of this in addressing the ticket, we will make sure to credit you appropriately.

Member

ingo commented Feb 27, 2014

Thank you for the suggested implementation. If we decide to use any part of this in addressing the ticket, we will make sure to credit you appropriately.

@ingo ingo closed this Feb 27, 2014

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