[FEATURE] Support for Source-Specific Multicast #802

squarebracket · 2017-10-31T22:55:04Z

I have read and understood the contributors guide.
I have checked that another pull request for this purpose does not exist.
I have considered, and confirmed that this submission will be valuable to others.
I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
I give this submission freely, and claim no ownership to its content.

My familiarity with the project is as follows (check one):

I have never used CCExtractor.
I have used CCExtractor just a couple of times.
I absolutely love CCExtractor, but have not contributed previously.
I am an active contributor to CCExtractor.

This is a simple change to allow for binding to source-specific multicast UDP feeds.

I've modified the GUI to allow for enough characters to bind to a SSM address, but it doesn't seem to work, even for simple UDP feeds. Can anyone confirm that the GUI works with UDP?

Also, I tried to validate that it works with Windows, but was unable. I see the packets in wireshark, but all of ccextractor, vlc, and mpv do not seem to get anything. If someone with more Windows-fu can validate that would be great.

anshul1912 · 2017-12-15T16:17:06Z

src/lib_ccx/ccx_common_option.h

 	LLONG debug_mask;                                   // dbg_print will use this mask to print or ignore different types
 	LLONG debug_mask_on_debug;                          // If we're using temp_debug to enable/disable debug "live", this is the mask when temp_debug=1
 	/* Networking */
+    char *udpsrc;


check surrounding indentation

anshul1912 · 2017-12-15T16:20:15Z

src/lib_ccx/params_dump.c

 		case CCX_DS_NETWORK:
-			if (ccx_options.udpaddr == NULL)
+            if (ccx_options.udpsrc != NULL)
+                mprint ("Network, %s@%s:%d", ccx_options.udpsrc, ccx_options.udpaddr, ccx_options.udpport);


udpaddr could be null, first check for null or in if add condition

I'm not sure I follow. Is params_dump.c used without params.c being called? Unless I messed up the parameter parsing, if you have a udpsrc you must have a udpaddr -- even if it's just \0 -- so you won't end up in a condition where udpaddr is null if udpsrc is not also null.

Of course, if there's a path to params_dump.c which doesn't first use params.c, then yeah it might be null, but I'm assuming that's not the case.

anshul1912 · 2017-12-15T16:21:15Z

src/lib_ccx/params.c

 		/* Network stuff */
 		if (strcmp (argv[i],"-udp")==0 && i<argc-1)
 		{
+            char *at = strchr(argv[i + 1], '@');


Look for surrounding indentation

anshul1912 · 2017-12-15T16:27:22Z

src/lib_ccx/networking.c

-		group.imr_multiaddr.s_addr = htonl(addr);
-		group.imr_interface.s_addr = htonl(INADDR_ANY);
-		if (setsockopt(sockfd, IPPROTO_IP, IP_ADD_MEMBERSHIP, (char *)&group, sizeof(group)) < 0)
+        int setsockopt_return = 0;


Look for surrounding indentation

anshul1912 · 2017-12-15T16:32:44Z

One of the build error I see is "/usr/bin/ld: cannot find -lz"

saurabhshri · 2017-12-15T18:37:18Z

@anshul1912 You probably are using cmake to build. Earlier we were using zlib installed in the environment, hence that flag was required. Now we are using the supplied ones so that flag probably is obsolete. Could you try building again after removing this line : https://github.com/CCExtractor/ccextractor/blob/master/src/CMakeLists.txt#L86 ?

squarebracket · 2017-12-15T19:05:47Z

Agh, tab spacing :( will fix

squarebracket · 2017-12-15T20:42:11Z

whitespace issues are fixed

anshul1912 · 2017-12-16T10:08:21Z

src/lib_ccx/params_dump.c

 		case CCX_DS_NETWORK:
-			if (ccx_options.udpaddr == NULL)
+			if (ccx_options.udpsrc != NULL)
+				mprint ("Network, %s@%s:%d", ccx_options.udpsrc, ccx_options.udpaddr, ccx_options.udpport);


check for ccx_options.udpaddr to be null is still missing.
I would suggest to put (ccx_options.udpsrc != NULL) in else if that would make sure if (ccx_options.udpaddr == NULL) has failed

Please see my reply to your previous comment, which was hidden due to white space changes.

anshul1912 · 2017-12-17T05:20:23Z

@squarebracket : Sorry for incovinence, I arrpove your changes.
Windows and linux difference of outputs test have not passed, can you please sync up with Willem and run them manually

canihavesomecoffee · 2017-12-17T11:19:10Z

@squarebracket If you'd make one small change (e.g. add your improvement to the changelog) and push that, the tests should be run again, and then we can see how it's impacting the program 👍

squarebracket · 2017-12-18T15:43:01Z

Should I just add it as Unreleased?

canihavesomecoffee · 2017-12-18T17:36:26Z

Just add it on the next version number or so :)

squarebracket · 2017-12-19T23:09:22Z

build errors are due to the python api stuff. I'd already preparing a PR to fix the build system to not break from the python stuff, would you like me to submit that first?

canihavesomecoffee · 2017-12-20T16:28:05Z

That would be awesome 👍

cfsmp3 · 2017-12-23T00:25:02Z

Please reopen (or better, send a new PR) when the build errors are fixed. Other than that the changes seem reasonable.

squarebracket · 2017-12-23T05:44:25Z

Build errors are unrelated to changes; the changes to the python API I'm talking about above are due to the currently broken automake caused by missing python symbols when building the code. Those I will submit in a different PR, since they are entirely unrelated to these changes.

Bumps [coverage](https://github.com/nedbat/coveragepy) from 7.2.5 to 7.2.6. - [Release notes](https://github.com/nedbat/coveragepy/releases) - [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst) - [Commits](coveragepy/coveragepy@7.2.5...7.2.6) --- updated-dependencies: - dependency-name: coverage dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Willem <github@canihavesome.coffee>

Support for Source-Specific Multicast

1dd4201

cfsmp3 requested a review from Izaron November 1, 2017 22:50

anshul1912 requested changes Dec 15, 2017

View reviewed changes

fixing whitespace issues

d38246e

anshul1912 requested changes Dec 16, 2017

View reviewed changes

anshul1912 approved these changes Dec 17, 2017

View reviewed changes

updating changelog

1f23402

cfsmp3 closed this Dec 23, 2017

cfsmp3 reopened this Dec 24, 2017

cfsmp3 merged commit 59b8f81 into CCExtractor:master Dec 24, 2017

[FEATURE] Support for Source-Specific Multicast #802

[FEATURE] Support for Source-Specific Multicast #802

Uh oh!

Conversation

squarebracket commented Oct 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anshul1912 commented Dec 15, 2017

Uh oh!

saurabhshri commented Dec 15, 2017

Uh oh!

squarebracket commented Dec 15, 2017

Uh oh!

squarebracket commented Dec 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anshul1912 commented Dec 17, 2017

Uh oh!

canihavesomecoffee commented Dec 17, 2017

Uh oh!

squarebracket commented Dec 18, 2017

Uh oh!

canihavesomecoffee commented Dec 18, 2017

Uh oh!

squarebracket commented Dec 19, 2017

Uh oh!

canihavesomecoffee commented Dec 20, 2017

Uh oh!

cfsmp3 commented Dec 23, 2017

Uh oh!

squarebracket commented Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

squarebracket commented Dec 23, 2017 •

edited

Loading