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

feat: Add functionality for transcoding media. #516

Merged
merged 1 commit into from
Mar 5, 2023

Conversation

Mossop
Copy link
Contributor

@Mossop Mossop commented Mar 2, 2023

Transcoding is unsurprisingly a complex business so I've tried to strike a balance in the API given here between making it relatively easy to use for the simple case but still exposing a lot of control over the transcode options. That said I'm not totally sold on the API as-is.

Streaming transcodes aren't really implemented here beyond starting the process, I don't particularly need them myself but they work so similarly to offline transcodes that it made sense to just add them in. Offline transcodes are fully working (though I have no doubt I'll hit bugs once I make more real-world use of it).

This change is on the large side so no rush 😂

@Mossop
Copy link
Contributor Author

Mossop commented Mar 2, 2023

Forgot to add. The tests for offline transcodes require a plex pass subscription. They automatically skip if one isn't available but there isn't much of a way around that.

@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2023

Codecov Report

Patch coverage: 71.84% and project coverage change: +2.00 🎉

Comparison is base (0c376c6) 67.95% compared to head (2e5b4f2) 69.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
+ Coverage   67.95%   69.95%   +2.00%     
==========================================
  Files          18       19       +1     
  Lines         908     1245     +337     
==========================================
+ Hits          617      871     +254     
- Misses        291      374      +83     
Impacted Files Coverage Δ
crates/plex-api/src/error.rs 88.23% <ø> (ø)
...tes/plex-api/src/media_container/server/library.rs 100.00% <ø> (ø)
crates/plex-api/src/server/library.rs 41.23% <36.36%> (+5.32%) ⬆️
crates/plex-api/src/http_client.rs 85.71% <50.00%> (-5.70%) ⬇️
crates/plex-api/src/server/transcode.rs 76.53% <76.53%> (ø)
crates/plex-api/src/server/mod.rs 63.51% <100.00%> (+6.37%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andrey-yantsen
Copy link
Owner

I see you weren't wasting your time 😅

Thank you! I'll check it out tomorrow, or during the weekend.

@Mossop
Copy link
Contributor Author

Mossop commented Mar 2, 2023 via email

@andrey-yantsen
Copy link
Owner

@Mossop, it depends on what you want in the result — check whether transcoding is available or the owner has PlexPass.

I did some digging, and here's what I found:

  1. You can check if the owner has PlexPass using Server.my_plex_subscription.
  2. It looks like you don't need to have PlexPass to have offline transcoding. At least the tests don't fail for me with a free user. So I guess offline transcoding is available as long as the server is claimed.
  3. There's Server.offline_transcode, and when it's equal to Some(1) the transcoding seems to be working just fine :)

The only thing I had to change to make the tests work is the expected frame rate in online::movie::offline_transcode_copy from 24.0 to 25.0. Does the test work for you with 24 there? It's broken for me for any server type — unclaimed, plexpass or free.

@Mossop
Copy link
Contributor Author

Mossop commented Mar 2, 2023

That's interesting. Offline is meant to be one of the perks of PlexPass but maybe they are gating that in the clients. I don't mind how we check though at the moment I return a SubscriptionFeatureNotAvailable error in the case it fails and that takes a server feature. It may make sense to use a different error if we don't know the right feature.

The online::movie::offline_transcode_copy test is passing for me locally but I was worried about slight differences in encoding between different Plex versions causing different results which is why tests on the transcoded content are pretty limited. I can't think why framerate would differ between us though.

@andrey-yantsen
Copy link
Owner

What's offline transcoding, by the way? :) The only thing about transcoding I can see in Plex: Free vs Paid is Hardware-accelerated streaming/transcoding.

I'm on Apple M1, by the way. It could also add some side effects.

As for the test, I think it'll be enough to validate only the parameters passed to the transcoding session and ignore the frames.

@Mossop
Copy link
Contributor Author

Mossop commented Mar 2, 2023 via email

@andrey-yantsen
Copy link
Owner

Ah! https://support.plex.tv/articles/downloads-sync-faq/ this page states that the server's owner must always have PlexPass to get this working, and there's a case when the client doesn't have to have PlexPass.

Yet again, the test works for my free user. I registered it in Jan 2022, so I just tried registering another user to check if it's because of this. And the new one also had the SyncV3 feature 🤷‍♂️

Ok, let's guard this with SyncV3 for now — it is working, after all. For some strange reason :)

Copy link
Owner

@andrey-yantsen andrey-yantsen left a comment

Choose a reason for hiding this comment

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

Thanks for the work. Everything looks great, as always.

I have just a few questions about the code and a few minor-ish change requests. Feel free to disagree with any of the things I said :)

crates/plex-api/src/media_container/server/library.rs Outdated Show resolved Hide resolved
crates/plex-api/src/server/library.rs Show resolved Hide resolved
crates/plex-api/src/server/mod.rs Outdated Show resolved Hide resolved
crates/plex-api/src/server/transcode.rs Show resolved Hide resolved
crates/plex-api/src/server/transcode.rs Outdated Show resolved Hide resolved
crates/plex-api/src/server/transcode.rs Outdated Show resolved Hide resolved
crates/plex-api/src/server/transcode.rs Outdated Show resolved Hide resolved
crates/plex-api/src/server/transcode.rs Outdated Show resolved Hide resolved
crates/plex-api/tests/transcode.rs Outdated Show resolved Hide resolved
andrey-yantsen
andrey-yantsen previously approved these changes Mar 5, 2023
Copy link
Owner

@andrey-yantsen andrey-yantsen left a comment

Choose a reason for hiding this comment

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

Thank you!

Let's see how the tests will behave on GitHub :)

andrey-yantsen
andrey-yantsen previously approved these changes Mar 5, 2023
@andrey-yantsen
Copy link
Owner

thread 'online::movie::hls_transcode_copy_unclaimed_server' panicked at 'called `Result::unwrap()` on an `Err` value:
JsonDeserealiseError { source: Error("unknown field `width`, expected one of `key`, `throttled`, `complete`, `progress`,
`size`, `speed`, `error`, `duration`, `remaining`, `context`, `sourceVideoCodec`, `sourceAudioCodec`, `videoDecision`,
`audioDecision`, `subtitleDecision`, `protocol`, `container`, `videoCodec`, `audioCodec`, `audioChannels`,
`transcodeHwRequested`, `transcodeHwDecoding`, `transcodeHwEncoding`, `transcodeHwDecodingTitle`,
`transcodeHwFullPipeline`, `transcodeHwEncodingTitle`, `offlineTranscode`, `timeStamp`, `minOffsetAvailable`,
`maxOffsetAvailable`", line: 1, column: 412) }', crates/plex-api/tests/transcode.rs:1062:62

It looks like there're a few new fields in TranscodeSessionStats in Plex 1.31.1 :\ At least width. Could you please dig into this? If you're not available, I'll be able to take a look tomorrow evening.

@Mossop
Copy link
Contributor Author

Mossop commented Mar 5, 2023

I think that should take care of it.

Copy link
Owner

@andrey-yantsen andrey-yantsen left a comment

Choose a reason for hiding this comment

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

That was quick, thank you! :)

@mergify mergify bot merged commit 91c89ae into andrey-yantsen:main Mar 5, 2023
@Mossop Mossop deleted the transcode branch March 5, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants