#1320 ✓ staged
brianchirls

YouTube wrapper does not accurately report muted or volume

Reported by brianchirls | September 4th, 2012 @ 09:57 AM | in 1.4

In setMuted, if it's called after playerReady is set to true, the function never sets impl.muted to true, so when getMuted is called later, it returns false, when it should be true.

On a related note, getVolume returns 1 if playerReady has not been set, but it should return 100, since YouTube is on a 0-100 scale and the volume property getter will divide that value by 100, incorrectly returning 0.1.

Comments and changes to this ticket

  • brianchirls

    brianchirls September 4th, 2012 @ 10:38 AM

    While we're at it, it looks like YouTube's API doesn't update volume right away when you ask it to. So the dispatchEvent calls after setting volume and mute should be in a setTimeout. 10ms seems to work.

  • Charlie Stigler

    Charlie Stigler September 19th, 2012 @ 03:12 AM

    • State changed from “new” to “peer-review-requested”
    • Assigned user set to “mjschranz”
    • Milestone order changed from “168” to “0”

    OK, I dealt with the problems you mentioned pretty much exactly. I don't really like the setTimeouts, since they seem messy and prone to becoming obsolete as the YT API changes, but I guess they are necessary for now.

    Pull request: https://github.com/mozilla/popcorn-js/pull/213

  • brianchirls

    brianchirls September 19th, 2012 @ 08:46 AM

    Looks good to me. Thanks, Charlie.

  • brianchirls

    brianchirls September 21st, 2012 @ 09:06 AM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “mjschranz” to “David Humphrey”
    • Milestone order changed from “10” to “0”
  • David Seifried

    David Seifried September 21st, 2012 @ 04:19 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “David Humphrey” to “Charlie Stigler”
    • Milestone order changed from “40” to “0”

    Nice, this looks great Charlie. One improvement we might be able to make tho, is instead of doing the 10 second timeout (I don't know if 10 seconds has meaning or not) we could add a timeout inside this block https://github.com/mozilla/popcorn-js/blob/master/wrappers/youtube/... that will continue to call setVolume until playerReady is true. This might look something like

        function setVolume( aValue ) {
          console.log( playerReady, "ASDASD" );
          if( !playerReady ) {
            impl.volume = aValue;
            setTimeout(function() {
              setVolume( impl.volume );
            }, 100);
            return;
          }
          player.setVolume( aValue );
          self.dispatchEvent( "volumechange" );
        }
    

    It seems like it is a bit more stable and we will only attempt to set the volume once playerReady is true.

  • Charlie Stigler

    Charlie Stigler September 21st, 2012 @ 06:01 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Charlie Stigler” to “David Seifried”
    • Milestone order changed from “7” to “0”

    Yeah the problem is that the lag is not only when the player isn't ready. It actually lags at all times. So if we call player.setVolume(50) and then immediately call player.getVolume(), it'll still return the old value for a little bit. So the issue is that if we dispatch the volumechange event, and then the listener calls pop.volume() to get the new value, the new value will be incorrect. We actually already wait for playerReady, and that works pretty well, but it doesn't solve the problem.

    Unfortunately, I can't think of a better way to do this than a timeout (although this is obviously not great). I thought of making it a repeating timer, but since YT tends to fire events in semi-random order, a series of quick volume changes could break things in that case.

    10ms has no particular significance, but both Brian and I have found that to be a good amount of time to wait. I found that almost all of my volume changes were reflected within 5ms, but I think 10ms is safer and there isn't a huge difference for most applications.

  • David Seifried

    David Seifried September 27th, 2012 @ 09:34 PM

    • State changed from “super-review-requested” to “feedback-requested”
    • Assigned user changed from “David Seifried” to “Charlie Stigler”
    • Milestone order changed from “30” to “0”

    Ah I understand now, sorry about that. I still feel that 10ms is still pretty arbitrary. What do you think about doing something like this instead https://github.com/dseif/popcorn-js/compare/t1320.

    I essentially made a queue for each time we set the volume and then ensure the volume has updated accordingly before actually dispatching the event. I like this approach because it doesn't feel like we are firing an event when there is a possibility that the volume may have not actually updated.

    Thoughts?

  • Charlie Stigler

    Charlie Stigler October 21st, 2012 @ 02:07 AM

    • State changed from “feedback-requested” to “super-review-requested”
    • Assigned user changed from “Charlie Stigler” to “David Seifried”
    • Milestone order changed from “7” to “0”

    Coming back to this much later, I looked through that code and it seems good. I tried to run tests on it but it looks like something else is breaking the tests... the onPlayerReady callback isn't being called. But outside of that looks good, can we merge without waiting for the unrelated issue?

  • Scott Downe

    Scott Downe December 13th, 2012 @ 01:56 PM

    • Assigned user changed from “David Seifried” to “Scott Downe”
  • Scott Downe

    Scott Downe December 13th, 2012 @ 03:01 PM

    Why don't we just store the intended volume internally, and return that when requested? Eliminating the timeout?

    We may still need to add volume changes to a queue, though.

    Also, if we're expecting the normal youtube controls to also work, we'll need to use the timeout.

  • Scott Downe

    Scott Downe December 13th, 2012 @ 03:18 PM

    I disagree with returning 0-100. We should do what HTML5 does and return 0-1.

  • Scott Downe

    Scott Downe December 13th, 2012 @ 03:29 PM

    • State changed from “super-review-requested” to “peer-review-requested”
    • Assigned user changed from “Scott Downe” to “mjschranz”

    I am attempting a simple fix for this, to avoid the setTimeout, if possible: https://github.com/mozilla/popcorn-js/pull/252

  • mjschranz

    mjschranz December 13th, 2012 @ 05:53 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “mjschranz” to “Scott Downe”

    Minor nit.

    I guess I'm only thinking that without the setTimeout there technically could be the issues of having the volume/mutechange events firing before YouTube has actually changed it. I guess though even with a short setTimeout that was in Charlies patch it wasn't a guarantee either.

  • Scott Downe

    Scott Downe December 13th, 2012 @ 06:02 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “Scott Downe” to “mjschranz”

    Good catch. Fixed.

    So, there are downfalls to this, and things I want fixed.

    Mainly, if volume changes from youtube's controls, the wrapper does not know (Youtube had no event for this, and previously we would poll it. I am starting to think their controls should be replaced by something the wrapper makes, thus that can never happen.)

    Also, like Matt said, we're not waiting for youtube to update the times before we trigger the event, again, youtube has no event for this, and we have to poll.

    Both of which I want to do in another ticket as they are a can of worms and one that does not need to be solved to land the above fixes.

  • mjschranz

    mjschranz December 13th, 2012 @ 06:08 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “mjschranz” to “Jon Buckley”

    PR+

  • Jon Buckley

    Jon Buckley December 14th, 2012 @ 10:31 AM

    • State changed from “super-review-requested” to “review-looks-good”
    • Assigned user changed from “Jon Buckley” to “Scott Downe”

    It'd be nice to have a sanity check for valid input values for the volume. However, I consider this optional, SR+ either way.

  • Scott Downe

    Scott Downe December 14th, 2012 @ 12:09 PM

    I think adding sanity checks can be filed.

  • Scott Downe

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Pages