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

Add node version and orch addr to transcoded metadata #3165

Merged
merged 6 commits into from
Sep 9, 2024
Merged

Conversation

j0sh
Copy link
Collaborator

@j0sh j0sh commented Sep 6, 2024

What does this pull request do? Explain your changes. (required)
Adds metadata that incorporates the node version and orchestrator address. This makes it easier to determine whether a given video came from the Livepeer network, and if so, who processed it.

For mpegts, the service_provider metadata field is used; for mp4 it is the comment field. Both are added unconditionally before passing on to the transcoder, but the muxer will actually only pick one or the other.

The actual transcoder version is used, rather than the orchestrator version to accommodate split O/T. If the network is offchain, the orchestrator address will say "offchain"

This metadata is taken as-is and not verified; transcoders could modify this with a custom build if they really wanted to. We could just insert the orch addr metadata from the broadcaster itself, but we would not have the transcoder version.

josh@josh-gpu:~/bench$ ffprobe -hide_banner http://localhost:8935/stream/stream/P240p30fps16x9/10.ts
Input #0, mpegts, from 'http://localhost:8935/stream/stream/P240p30fps16x9/10.ts':
  Duration: N/A, start: 101.400000, bitrate: N/A
  Program 1
    Metadata:
      service_name    : Service01
      service_provider: Livepeer Transcoder 0.7.8-48a92a17-dirty (offchain)
  Stream #0:0[0x100]: Video: h264 (Main) ([27][0][0][0] / 0x001B), yuv420p(progressive), 426x240 [SAR 1:1 DAR 71:40], 30 fps, 30 tbr, 90k tbn
  Stream #0:1[0x101]: Audio: aac (LC) ([15][0][0][0] / 0x000F), 48000 Hz, stereo, fltp, 163 kb/s

Specific updates (required)

  • Store the PM recipient address (orch's onchain address) in the LivepeerNode
  • Adds a new orchId field to the O -> T protobuf definitions to carry the RecipientAddr to the transcoder
  • Set the orchId to "offchain" for off-chain transcoding. Use RecipientAddr otherwise.
  • Add a new helper, MakeMetadata for use both with local O transcoding and split O+T
  • Update LPMS for metadata support - ffmpeg: Add a metadata option to each output lpms#421
  • Regenerate the protobuf definitions - I could not get an older version of protoc installed, so ended up with this mess of a commit : 3fa2382

How did you test each of these updates (required)

Manual testing with combined OT and split O+T

Does this pull request close any open issues?

Checklist:

Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

The PR looks good. Very nice change @j0sh

I have just one product question (CC: @thomshutt @hthillman ), I wonder if there is any case in which we would not want to expose that the video is coming from Livepeer Network. I remember that we had some customers with the special DNS domain to not expose it's coming from Livepeer. Not sure it's valid anymore or/and if there would be any issue exposing it in the video segment metadata.

@@ -340,6 +340,9 @@ message NotifySegment {
// ID for this particular transcoding task.
int64 taskId = 16;

// Orchestrator identifier for segment metadata
string orchId = 18;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: shouldn't you move it under profiles, so that the indexing is in the order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't profiles deprecated?

@thomshutt
Copy link
Contributor

I remember that we had some customers with the special DNS domain to not expose it's coming from Livepeer.

Good point Rafal, but I think it's buried enough that we don't mind Livepeer's name being attached here - even for the other stuff it was more of a "best effort" approach to anonymity.

Copy link
Contributor

@thomshutt thomshutt left a comment

Choose a reason for hiding this comment

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

@j0sh looks good! Is there any other metadata (timestamp?) to hand that would be useful to add while we're in here?

Copy link

codecov bot commented Sep 6, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 8 lines in your changes missing coverage. Please review.

Project coverage is 57.38905%. Comparing base (a99a631) to head (bff415c).

Files with missing lines Patch % Lines
core/orchestrator.go 37.50000% 2 Missing and 3 partials ⚠️
core/transcoder.go 81.81818% 2 Missing ⚠️
cmd/livepeer/starter/starter.go 0.00000% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #3165         +/-   ##
===================================================
+ Coverage   57.36772%   57.38905%   +0.02133%     
===================================================
  Files             92          92                 
  Lines          15819       15841         +22     
===================================================
+ Hits            9075        9091         +16     
- Misses          6139        6142          +3     
- Partials         605         608          +3     
Files with missing lines Coverage Δ
core/livepeernode.go 68.18182% <ø> (ø)
core/streamdata.go 64.19753% <100.00000%> (+2.35542%) ⬆️
server/ot_rpc.go 36.13139% <100.00000%> (+0.23395%) ⬆️
cmd/livepeer/starter/starter.go 7.73694% <0.00000%> (-0.00749%) ⬇️
core/transcoder.go 43.30709% <81.81818%> (+1.60669%) ⬆️
core/orchestrator.go 77.77778% <37.50000%> (-0.54155%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a99a631...bff415c. Read the comment docs.

Files with missing lines Coverage Δ
core/livepeernode.go 68.18182% <ø> (ø)
core/streamdata.go 64.19753% <100.00000%> (+2.35542%) ⬆️
server/ot_rpc.go 36.13139% <100.00000%> (+0.23395%) ⬆️
cmd/livepeer/starter/starter.go 7.73694% <0.00000%> (-0.00749%) ⬇️
core/transcoder.go 43.30709% <81.81818%> (+1.60669%) ⬆️
core/orchestrator.go 77.77778% <37.50000%> (-0.54155%) ⬇️

@j0sh
Copy link
Collaborator Author

j0sh commented Sep 6, 2024

Is there any other metadata (timestamp?) to hand that would be useful to add while we're in here?

Yep, there is a lot we could collect (someone in Discord suggested the GPU type, etc) but I am not sure how much we really want to stuff into this one field. It was mostly meant to scratch my own itch around not being able to figure out where segments came from.

Can definitely add the timestamp if it seems like it would be near-term useful, though. UTC or Unix? I also wonder if there is a better 'place' in mpegts we can put things like the creation date, but google and some quick tests are not turning up anything...

@j0sh j0sh merged commit eec6ed3 into master Sep 9, 2024
18 checks passed
@j0sh j0sh deleted the ja/add-metadata branch September 9, 2024 18:19
@j0sh j0sh mentioned this pull request Sep 9, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants