i8beef / node-red-contrib-castv2

MIT License
22 stars 14 forks source link

Queue_Next does not work with DefaultMediaReceiver #57

Closed aXaut closed 3 years ago

aXaut commented 3 years ago

Hi,

I currently have the issue, that when I start playback of multiple items, QUEUE_NEXT does not work. What I then found out is, that also in the Google Home App, the "next" button is unavailable. After a little debugging (I'm quite new to Chromecast), I found out that supportedMediaCommands was 274447 which does not include the QUEUE_NEXT bit (64). Since you are checking this in castv2-sender.js:593 it all makes sense and works as it should. Still I was curious since I didn't understand why it should not work (GET_STATUS also showed me that all items were listed), tried removing this check and found out, that it actually does work.

As I said, I'm very new to the Chromecast protocol and cannot say if this is a bug in any of the used libraries or maybe even in Chromecast itself (since it is missing the flag in supportedMediaCommands) or if there is just some information missing that makes it not adding the flag. Maybe you have an idea about it.

I don't know what you think about just removing the check in castv2-sender.js (also doing the same for QUEUE_PREV in a locally patched version), feels kind of wrong but still - it works. But I'd also be very interested in why the bit is missing. Depending on what you think about it, I could commit and push the removal of the check and open a PR, but I didn't want to do this without getting the developer's opinion on that.

BR

btw, I'm really thankful for this Node, great job there!

Here are the bodys that I'm sending (of course, urls need to be adapted if trying to reproduce): Start playing media:

{
  payload: {
    app: "DefaultMediaReceiver",
    type: "MEDIA",
    media: [
      { 
        url: "http://mediaserver/static/tracks/track1.mp3"
      },
      { 
        url: "http://mediaserver/static/tracks/track2.mp3"
      }
    ]
  }
}

QUEUE_NEXT:

{
  payload: {
    type: "QUEUE_NEXT"
  }
}
i8beef commented 3 years ago

https://issuetracker.google.com/issues/139939455

Checking the supportedMediaCommands is exactly what Google has made their apps do, which is why you don't get the buttons in their apps either... Google says:

This is actually an intended change. We used to check the local queue's content to determine if the prev/next buttons should be enabled. This is not the right way because it won't work with cloud queue or some apps might have different policy to limit skips on free accounts (e.g. GPM).

Hence, we decided to use SupportedMediaCommand to determine if it should enable prev/next buttons. However, this also needs some receiver side change in default receiver, which unfortunately hasn't been done yet.

So, if you have custom receiver, -- The recommendation is to make the change on receiver, so that it can set SupportedMediaCommand on receiver when necessary.

If you are using default receiver, -- We don't have a solution yet. I'm going to discuss with receiver team to find a solution.

I am in that second "if you are using the default receiver" case... It looks like they have stalled even looking into it, so it might be worth creating an account to comment on this ticket.

i8beef commented 3 years ago

Removing the check would require two things:

  1. Validation that doing so doesn't open up other failures, for instance, sending queue commands when not supporting causing failures / errors that crash the node, etc. These guards are CORRECT but also broken because... well Google.
  2. A properly snarky comment in the codebase around the commented out check for these so that all who explore later can share in the disappointment of another google implementation left to languish.
aXaut commented 3 years ago

Ok got it... that's what I have feared...

About the change: I agree on both points, although I'm not sure if I personally can test all or at least most of the cases to see if there is any scenario which breaks the node. Till now, it always worked for me without breaking anything, but I don't know what other services use the DefaultMediaReceiver. We can definitly add to just skip the guard in case that it is the DefaultMediaReceiver and that there is more than 1 item in the queue (we have the status there anyway which allows us to check). This will also make clear to any other developer that this is obviously a workaround and that we tried to scope it as good as possible...

i8beef commented 3 years ago

Yes, good point I'd only want to do it with the DefaultMediaReceiver case. You could make that check on the appId... and it sounded like verifying queue lengths was what Google did on the original iOS implementation, so that sounds like a decent enough idea too. Something like

let bypassCheck = node.receiver.APP_ID === DefaultMediaReceiver.APP_ID && status.items.length > 1;
if (status.supportedMediaCommands & 64 || bypassCheck) {
    return node.receiver.queueNextAsync();
}

Edit: note pseudo code esque, I dont remember without looking what status.items is for instance.

i8beef commented 3 years ago

4.0.2 should be up in a little bit.

aXaut commented 3 years ago

Works great! Thx for the support.