Skip to content

Conversation

@San9H0
Copy link

@San9H0 San9H0 commented Apr 7, 2025

Description

The RegisterCodec function is public and takes the mimeType as a string. Therefore, it should compare values using strings.EqualFold.
In general, MIME type comparisons should be done in a case-insensitive manner.

Reference issue

Fixes #...

@codecov
Copy link

codecov bot commented Apr 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.72%. Comparing base (788147d) to head (3e2a804).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3085      +/-   ##
==========================================
- Coverage   78.74%   78.72%   -0.02%     
==========================================
  Files          91       91              
  Lines       11401    11401              
==========================================
- Hits         8978     8976       -2     
- Misses       1935     1937       +2     
  Partials      488      488              
Flag Coverage Δ
go 80.62% <100.00%> (-0.02%) ⬇️
wasm 63.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Sean-Der
Copy link
Member

Sean-Der commented Apr 7, 2025

This looks great to me @San9H0!

would you mind adding a simple test? Just to make sure no one breaks your fix

@San9H0 San9H0 force-pushed the fix-equalfold-for-mimetype branch from 5c070c0 to 5ca0470 Compare April 8, 2025 01:52
@San9H0
Copy link
Author

San9H0 commented Apr 8, 2025

Thanks for the review! I've added a test case to verify the case-insensitive MIME type comparison. Let me know if you need any changes.

@JoeTurki JoeTurki self-requested a review April 8, 2025 02:00
@JoeTurki
Copy link
Member

JoeTurki commented Apr 8, 2025

Thank you :)

@JoeTurki JoeTurki self-requested a review April 8, 2025 02:12
@San9H0 San9H0 force-pushed the fix-equalfold-for-mimetype branch 3 times, most recently from 32a4661 to e4863c8 Compare April 8, 2025 09:21
@JoeTurki
Copy link
Member

JoeTurki commented Apr 9, 2025

@San9H0 is it ready? :)

@San9H0
Copy link
Author

San9H0 commented Apr 10, 2025

OK. It's all done on my side. Let me know if anything else is needed

@3DRX 3DRX closed this Apr 13, 2025
@3DRX 3DRX reopened this Apr 13, 2025
@3DRX
Copy link
Member

3DRX commented Apr 13, 2025

Can we call strings.ToLower when MIME type is passed into media engine? Then internally it will always be lowercase and we don't have to worry about case insensitiveness. I'm thinking about this because if future changes adds more MIME type comparison, more tests on MIME type comparison needs to be added.

(Mistakenly closed this for a second because I hit the wrong button when I tried to comment, sorry about that)

@JoeTurki
Copy link
Member

JoeTurki commented Apr 28, 2025

Can we call strings.ToLower when MIME type is passed into media engine? Then internally it will always be lowercase and we don't have to worry about case insensitiveness. I'm thinking about this because if future changes adds more MIME type comparison, more tests on MIME type comparison needs to be added.

I agree that would make much more sense

Edit: after tests, this would wouldn't be simple, sadly, it would break some things that rely on the current behavior, but we can do it later.

@JoeTurki JoeTurki force-pushed the fix-equalfold-for-mimetype branch from 3d489e8 to c0bb88d Compare April 28, 2025 02:30
@JoeTurki JoeTurki force-pushed the fix-equalfold-for-mimetype branch from c0bb88d to 3e2a804 Compare April 28, 2025 02:34
@JoeTurki JoeTurki merged commit 3e2a804 into pion:master Apr 28, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants