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

Extend AMF0 decode implementation #3189

Merged
merged 7 commits into from Apr 12, 2024
Merged

Conversation

pedroSG94
Copy link
Contributor

@pedroSG94 pedroSG94 commented Apr 3, 2024

Hello,

I did this PR to fix the error mentioned here:
#3188
Also, I added others AMF0 types to decode extending the support:

  • Add AMF0 Strict Array type decode
  • Add AMF0 Long String type decode
  • Add AMF0 XML Document type decode
  • Add AMF0 Unsupported type decode
  • Add AMF0 Undefined type decode
  • Add AMF0 Date type decode

With this, the server should support all AMF0 types decode except recordedset, movieclip and typed-object.
recordedset and movieclip are reserved so you can ignore it.
typed-object is a custom object serialized so ignored it should be fine too

Note: Strict Array implementation return nil value because I'm not sure how to return an array of item. Maybe you need fix it or give me a tip for it ( I'm new with golang :( )

@pedroSG94 pedroSG94 changed the title add amf0 strict array decode Extend AMF0 decode implementation Apr 4, 2024
Copy link

codecov bot commented Apr 6, 2024

Codecov Report

Attention: Patch coverage is 2.77778% with 35 lines in your changes are missing coverage. Please review.

Project coverage is 55.53%. Comparing base (d59174d) to head (27ac498).

❗ Current head 27ac498 differs from pull request most recent head 3cbdcff. Consider uploading reports for the commit 3cbdcff to get more accurate results

Files Patch % Lines
internal/protocols/rtmp/amf0/unmarshal.go 2.77% 35 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3189      +/-   ##
==========================================
- Coverage   55.95%   55.53%   -0.42%     
==========================================
  Files         154      151       -3     
  Lines       16993    16909      -84     
==========================================
- Hits         9508     9391     -117     
- Misses       6734     6779      +45     
+ Partials      751      739      -12     

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

@pedroSG94
Copy link
Contributor Author

pedroSG94 commented Apr 6, 2024

Fixed comment error and tests added

@aler9
Copy link
Member

aler9 commented Apr 7, 2024

Hello, thank you very much for this patch - just one question, i noticed that you edited the parser in order to silently skip markerUnsupported and markerUndefined. Silently skipping things is something we've always avoided unless strictly necessary, because it makes detecting problems harder. Are you sure that this is necessary? thanks

@pedroSG94
Copy link
Contributor Author

Hello, thank you very much for this patch - just one question, i noticed that you edited the parser in order to silently skip markerUnsupported and markerUndefined. Silently skipping things is something we've always avoided unless strictly necessary, because it makes detecting problems harder. Are you sure that this is necessary? thanks

Hello,

It is not necessary, I only did this to avoid close a connection if this types are received because the connection could be possible skipping it. Maybe a good solution could be show a warning log for debug purpose.
We can do it as you want. You know more about possible side effects, so maybe it is not a good idea to skip it anyway

@aler9
Copy link
Member

aler9 commented Apr 12, 2024

i implemented StrictArray marshaling, added fuzz tests and removed the long string / XML / date handling, since those last entities are seldom used and would require additional logic. If you encounter these entities in the future, feel free to submit another patch.

@aler9 aler9 merged commit 8aa26d6 into bluenviron:main Apr 12, 2024
6 checks passed
Copy link
Contributor

This issue is mentioned in release v1.7.0 🚀
Check out the entire changelog by clicking here

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.

None yet

2 participants