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

Fixed StreamID and Smoother name swapping for big endian machines #644

Merged
merged 2 commits into from
Apr 29, 2019

Conversation

ethouris
Copy link
Collaborator

Fixes #634.

The protocol definitions stays as before (that is, characters in StreamID and Smoother are fourwise inverted). There's a fix made especially for big endian machines, which turns them to the "little endian convention" for the sake of transport.

@ethouris ethouris changed the title Changed endian-swapping rules Fixed StreamID and Smoother name swapping for big endian machines Apr 16, 2019
@ethouris
Copy link
Collaborator Author

NOTE: the theoretical endian-swapping has been added and portable endian-swapping functions have been added to utilities, but this PR wasn't tested on any big endian machine. It was tested as little-endian and it is confirmed that these calls to HtoILA and ItoHLA functions does not change the order (the functionality works exactly the same as before).

@jlsantiago0
Copy link
Contributor

@ethouris If you have any unit tests I can run them on a big endian machine for you. I have an IBookG4 and a PowerMacG5. I can also provide you with a QEMU Power7 Image running CentOS if you would prefer to do it yourself.

@ethouris
Copy link
Collaborator Author

@jlsantiago0 I don't have unit tests for that, but you can simply try to run a file transmission (srt-file-transmit or srt-test-file) between one machine with little endian and one with big endian. Apps for file transmission use the congestion controller id as "file" in the handshake extension. If the byte swapping is made wrong, the connection will be rejected (as it can't find "elif" smoother). If you specify the directory as target with the SRT receiver specified as listener, then the filename will be also passed through StreamID - so you kill two birds with one stone.

So: receiver: srt-test-file srt://:5000 . and sender: srt-test-file FILENAME srt://HOST:5000. You should have the file transmitted and the target filename should be FILENAME (not ELIFEMAN).

@jlsantiago0
Copy link
Contributor

jlsantiago0 commented Apr 17, 2019

Hi @ethouris

I have built the current SRT Master branch with and without your patches on the following targets:

  • Manjaro18 Linux-x86_64 running on i7 hardware
  • CentOS7 Linux-PPC64 running on Power7/8 Emulated system under QEMU
x86Host$ uname -a
Linux jlsws0.haivision.com 4.19.32-1-MANJARO #1 SMP PREEMPT Wed Mar 27 18:55:07 UTC 2019 x86_64 GNU/Linux

ppc64Host$ uname -a
Linux qmc7ppc64.localdomain 3.10.0-957.10.1.el7.ppc64 #1 SMP Thu Mar 14 14:54:09 GMT 2019 ppc64 ppc64 ppc64 GNU/Linux

Configured as follows:

cmake -DCMAKE_INSTALL_PREFIX=`pwd`/stage
make install

There does not appear to be an application called srt-test-file produced by the build, but there is one named srt-file-transmit which seems to also demonstrate the issue.

When running without your patch from the PPC64 host to the X86_64 host, I observe the following:

x86Host$ <path-to-unpatched-binary>/srt-file-transmit  srt://:5000 .
11:18:56.287570/SRT:RcvQ:worker*E:SRT.c: PEER'S SMOOTHER 'elif' does not match AGENT'S SMOOTHER 'file'
11:18:56.287774/SRT:RcvQ:worker*E:SRT.c: @888111946:newConnection: connection rejected due to: ACCEPT ERROR
11:18:56.287806/SRT:RcvQ:worker*E:SRT.c: UU:newConnection: rsp(REJECT): 1002

ppc64Host$  <path-to-unpatched-binary>/srt-file-transmit   /mnt/share/content/ts/001.ts srt://10.66.133.30:5000
11:18:56.271357/SRT:RcvQ:worker*E:SRT.c: processAsyncConnectRequest: REJECT reported from HS processing, not processing further.
11:18:56.278221/SRT:RcvQ:worker*E:SRT.c: RendezvousQueue: processAsyncConnectRequest FAILED. Setting TTL as EXPIRED.
Target disconnected
11:18:56.562502/SRT:RcvQ:worker!!FATAL!!:SRT.c: CChannel reported ERROR DURING TRANSMISSION - IPE. INTERRUPTING worker anyway.

After applying your patches:

x86Host$ <path-to-patched-binary>/srt-file-transmit  srt://:5000 .
Source connected (listener), id [001.ts]
Writing output to [/home/jsantiago/tt69/./001.ts]
Source disconnected

ppc64Host$  <path-to-patched-binary>/srt-file-transmit   /mnt/share/content/ts/001.ts srt://10.66.133.30:5000
Target connected (caller)
File sent
Buffers flushed

However, after the "transfer" is finished the two files are not the same.

x86Host$ cmp -l  /mnt/share/content/ts/001.ts ./001.ts
    2 100 107
    3  60 107
    4  26 107
....
36400 203   0
36401 172   0
36402 217  55
cmp: EOF on ./001.ts after byte 36402

And the file sizes of the transfered file is greatly different.

-rw-r--r-- 1 jsantiago users    36402 Apr 17 11:31 ./001.ts
-rw-r--r-- 1 root      root  47903968 Jun 27  2011 /mnt/share/content/ts/001.ts

So there still appears to be some additional issue.

@ethouris
Copy link
Collaborator Author

Ok, so in the beginning it all works exactly failing as I expected, that's some good news.

I don't get what the problem could be with the torn file - this has to be checked later. The srt-test-file application requires --enable-testing during build (in cmake, -DNEABLE_TESTING=1). If you can still see failures like this one, please turn on debug logs (-loglevel debug) and attach them.

@maxsharabayko
Copy link
Collaborator

@jlsantiago0 Thank you for checking this fix!
@ethouris @jlsantiago0 I would suggest to collect a wireshark capture of the stream on the receiving side and on the sending side. This might be easier to collect, and together with the source file will provide a good hint on what is going on.
Just pay attention to start capturing before initiating the transmission. This way the capture will also include SRT handshake packets.

@maxsharabayko maxsharabayko added this to the v.1.3.3 milestone Apr 18, 2019
@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior labels Apr 18, 2019
@maxsharabayko
Copy link
Collaborator

@jlsantiago0 Losses might be related to the recent changes in srt-file-transmit, as reported in #645. I will check.

@jlsantiago0
Copy link
Contributor

jlsantiago0 commented Apr 23, 2019

@jlsantiago0 Losses might be related to the recent changes in srt-file-transmit, as reported in #645. I will check.

OK. Sorry I had not had time to revisit this earlier. Should I wait until you have all that worked out and then I can test again?

@maxsharabayko
Copy link
Collaborator

@jlsantiago0 Should be fixed now by PR #658. Please confirm.

@jlsantiago0
Copy link
Contributor

@jlsantiago0 Should be fixed now by PR #658. Please confirm.

@maxlovic I have tested the srt-file-transmit application in both directions with the current master branch with @ethouris branch dev-fix-streamid-bigendian merged and it successfully transferred files between the BE and LE machines listed above.

@maxsharabayko
Copy link
Collaborator

@jlsantiago0

I have tested the srt-file-transmit application in both directions with the current master branch with @ethouris branch dev-fix-streamid-bigendian merged and it successfully transferred files between the BE and LE machines listed above.

Cool. Thank you!

@rndi rndi merged commit 7c19251 into Haivision:master Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stream ID endian-reversed
4 participants