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 bitdepth, samplingrate and channelCount optional fields #83

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

GioF71
Copy link
Contributor

@GioF71 GioF71 commented Jan 13, 2024

Hello, this is related to this discussion.
Thank you for your attention.

Copy link

netlify bot commented Jan 13, 2024

Deploy Preview for opensubsonic ready!

Name Link
🔨 Latest commit b3836cb
🔍 Latest deploy log https://app.netlify.com/sites/opensubsonic/deploys/661312f268e4390008544a0c
😎 Deploy Preview https://deploy-preview-83--opensubsonic.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Tolriq
Copy link
Member

Tolriq commented Jan 13, 2024

We did not fully finished that discussion, we should also expose channelCount. Field doc should be more precise about the field values (with example in the doc to explain that it's full sample rate 44100 and no 44.1).
And we need to define a little more the codec field to avoid random values there.

Like enforced lowercase, and decide if it's the full codec codes from ffmpeg or only the short usual ones. (full codec would be better)

@GioF71
Copy link
Contributor Author

GioF71 commented Jan 13, 2024

We did not fully finished that discussion, we should also expose channelCount. Field doc should be more precise about the field values (with example in the doc to explain that it's full sample rate 44100 and no 44.1). And we need to define a little more the codec field to avoid random values there.

Like enforced lowercase, and decide if it's the full codec codes from ffmpeg or only the short usual ones. (full codec would be better)

Ops, sorry.
Some time passed so I though I had to do something myself in order to complete the change.
Do we need to close this PR, or can we keep it open so that I can push more changes?

Thank you

@Tolriq
Copy link
Member

Tolriq commented Jan 13, 2024

The discussion can continue here it often works like that to finalize the details, without a PR people stops talking :p

@epoupon
Copy link
Member

epoupon commented Jan 13, 2024

Indeed we can add channelCount in this PR, but I would discuss codec/format related info in another PR as it is not straightforward

@dweymouth
Copy link
Member

I think we should publish a list of well-known codec values, and try to be fairly extensive, whether we do the short-form or the long-form codec strings. (And I'd probably suggest the short-form, because that's what already is used de-facto for the "format" arg to the stream endpoint)

@GioF71
Copy link
Contributor Author

GioF71 commented Jan 13, 2024

@dweymouth @epoupon @Tolriq will we update the discussion, or can we collect the 'missing' information here in this PR?

@GioF71
Copy link
Contributor Author

GioF71 commented Jan 13, 2024

Indeed we can add channelCount in this PR, but I would discuss codec/format related info in another PR as it is not straightforward

If this is agreed, I can remove the "codec" entries from this PR and leave this aspect for another one.

@dweymouth
Copy link
Member

Let's return to the discussion for codec, but it's up to you @GioF71 if you want to do this PR without codec, or let it sit until we come to an agreement on how we want to handle codec

@Tolriq
Copy link
Member

Tolriq commented Jan 13, 2024

I would say that all the data is related and that for a server to store and expose most of the data the codec will be extracted at the same time, it's better if how it should be exposed is defined at the same time to avoid some chosing something that won't work with a future decision.

@GioF71
Copy link
Contributor Author

GioF71 commented Jan 13, 2024

ok I'll leave the "codec" fields, and will update the rest according to the discussion.
Thank you

@epoupon
Copy link
Member

epoupon commented Jan 13, 2024

I would say that all the data is related and that for a server to store and expose most of the data the codec will be extracted at the same time, it's better if how it should be exposed is defined at the same time to avoid some chosing something that won't work with a future decision.

Taglib can extract channel count, bitdepth and sampling rate out of the box, but this is more tricky to get exact info on formats and codecs used.
I would really "secure" these three fields first and spend some days/weeks debating on codec/formats on a separate PR (in the proposal of @dweymouth , there are entries like "FLAC audio in an OGG container (uncommon)" that are mixing codecs and formats, just to say that it will likely take some time to be agreed on...)

@Tolriq
Copy link
Member

Tolriq commented Jan 13, 2024

Codec and transcoding format are unrelated as not clear enough, the format param is more container than codec. The codecs will be used in the future transcoding API as the formats/ container

@dweymouth
Copy link
Member

The codecs will be used in the future transcoding API as the formats/ container

Not quite sure what you mean by this? Are you saying the client will execute something like /stream?format=flac&container=flac,ogg to request either OGG flac or standard flac?

@Tolriq
Copy link
Member

Tolriq commented Jan 13, 2024

To be defined, but the idea is the client says I support that that that and that with contraints on things. Then server returns either the file can be direct played or should be transcoded with x and t param.

@sentriz
Copy link
Member

sentriz commented Jan 13, 2024

is this consistent with the transcodedContentType, transcodedSuffix fields that we have already?

@dweymouth
Copy link
Member

is this consistent with the transcodedContentType, transcodedSuffix fields that we have already?

This would serve a different purpose as the contentType and extension fields are very non-specific about the actual codec/container. (eg there is no specific contentType for Apple Lossless, most servers would probably report audio/mp4). Suffix is also kind-of a useless field since file extensions don't matter at all (eg on Unix-like OSes you could have all your media files have no extension and it's fine because extension has nothing to do with the actual file type)

@epoupon epoupon added this to the mod milestone Jan 14, 2024
@Tolriq Tolriq removed this from the mod milestone Mar 12, 2024
@Tolriq
Copy link
Member

Tolriq commented Apr 2, 2024

Ok so since there's no movement on this one, let's remove the codec part and expose bitDepth, samplingRate and channelCount?

Would still be nice to continue the work on the codec/container/full codec ones.

@GioF71
Copy link
Contributor Author

GioF71 commented Apr 2, 2024

I agree.
I suppose we give some time for the others to confirm and/or elaborate?

Edit: some -> some time

@epoupon
Copy link
Member

epoupon commented Apr 2, 2024

Fine for me!

@GioF71 GioF71 force-pushed the add-bitdepth-samplingrate branch 2 times, most recently from 1257989 to 67282e7 Compare April 7, 2024 15:49
@GioF71
Copy link
Contributor Author

GioF71 commented Apr 7, 2024

Ok, I have removed the last commit, which added the codec field, so we are back with only samplingRate and bitDepth.
Let me know if other changes are needed.
Thank you

@Tolriq
Copy link
Member

Tolriq commented Apr 7, 2024

Can you add the requested channelCount ? There's no debate about this is and it's needed also to know if a media is playable or not.

@GioF71
Copy link
Contributor Author

GioF71 commented Apr 7, 2024

Can you add the requested channelCount ? There's no debate about this is and it's needed also to know if a media is playable or not.

Sorry, I missed that. I will update asap.

@GioF71 GioF71 force-pushed the add-bitdepth-samplingrate branch from 67282e7 to ac2d9ab Compare April 7, 2024 16:15
@GioF71 GioF71 force-pushed the add-bitdepth-samplingrate branch from ac2d9ab to c0e23a9 Compare April 7, 2024 16:15
@GioF71
Copy link
Contributor Author

GioF71 commented Apr 7, 2024

Field channelCount added

@Tolriq Tolriq changed the title Add bitdepth, samplingrate and codec optional fields Add bitdepth, samplingrate and channelCount optional fields Apr 7, 2024
Tolriq
Tolriq previously approved these changes Apr 7, 2024
epoupon
epoupon previously approved these changes Apr 7, 2024
content/en/docs/Responses/Child.md Outdated Show resolved Hide resolved
@GioF71 GioF71 dismissed stale reviews from epoupon and Tolriq via b3836cb April 7, 2024 21:41
@Tolriq
Copy link
Member

Tolriq commented Apr 9, 2024

Small @dweymouth ping before merging just in case.

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.

6 participants