bluenviron / mediamtx

Ready-to-use SRT / WebRTC / RTSP / RTMP / LL-HLS media server and media proxy that allows to read, publish, proxy, record and playback video and audio streams.
MIT License
10.78k stars 1.4k forks source link

Extend AMF0 decode implementation #3189

Closed pedroSG94 closed 2 months ago

pedroSG94 commented 3 months ago

Hello,

I did this PR to fix the error mentioned here: https://github.com/bluenviron/mediamtx/issues/3188 Also, I added others AMF0 types to decode extending the support:

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 :( )

codecov[bot] commented 3 months ago

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).

:exclamation: 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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pedroSG94 commented 3 months ago

Fixed comment error and tests added

aler9 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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.

github-actions[bot] commented 2 months ago

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