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

[IMPROVEMENT] Remove useless if statement #1175

Open
wants to merge 2 commits into
base: master
from

Conversation

@NilsIrl
Copy link
Contributor

NilsIrl commented Jan 10, 2020

  • 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 used CCExtractor just a couple of times.

Increases readability (IMO).

I was told by canihavesomecoffee that code improvement PRs are always welcome so I opened this.

@ccextractor-bot

This comment has been minimized.

Copy link
Collaborator

ccextractor-bot commented Jan 10, 2020

CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results:

Report Name Tests Passed
Broken 0/13
DVB 0/7
DVR-MS 0/2
General 0/27
Hauppage 0/3
MP4 0/3
NoCC 0/10
Teletext 0/21
WTV 0/13
XDS 0/34
CEA-708 0/14
DVD 0/3
Options 0/86

It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you).

Your PR breaks these cases:


Check the result page for more info.
@ccextractor-bot

This comment has been minimized.

Copy link
Collaborator

ccextractor-bot commented Jan 10, 2020

CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results:

Report Name Tests Passed
Broken 13/13
DVB 4/7
DVR-MS 2/2
General 27/27
Hauppage 3/3
MP4 3/3
NoCC 10/10
Teletext 21/21
WTV 13/13
XDS 33/34
CEA-708 14/14
DVD 3/3
Options 84/86

It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you).

Your PR breaks these cases:


Check the result page for more info.
@@ -202,8 +202,7 @@ int api_start(struct ccx_s_options api_options)
#ifdef ENABLE_FFMPEG
case CCX_SM_FFMPEG:
#endif
if (!api_options.use_gop_as_pts) // If !0 then the user selected something
api_options.use_gop_as_pts = 0;
api_options.use_gop_as_pts = 0;

This comment has been minimized.

Copy link
@canihavesomecoffee

canihavesomecoffee Jan 10, 2020

Member

I'm not so sure if this is the correct logic.

if the use_gop_as_pts is set to 1, it'll skip the statement under old conditions. Now it'd be set to 0.
It seems more logic to me to just delete the two lines.

@cfsmp3 What do you think?

This comment has been minimized.

Copy link
@canihavesomecoffee

canihavesomecoffee Jan 10, 2020

Member

And since this has been in since the original commit of the repo, we definitely need @cfsmp3 to tell something about it 👍

This comment has been minimized.

Copy link
@NilsIrl

NilsIrl Jan 10, 2020

Author Contributor

I wrote this more than 1 month ago and just realised it doesn't make sense at all.

if it is 0 then !0 == 1

so the variable is set to 0

And if it is 1 (or anything else) nothing happens.

This comment has been minimized.

Copy link
@NilsIrl

NilsIrl Jan 10, 2020

Author Contributor

I removed the lines.

Also my previous commit was totally wrong as it would zero the variable when it is one which it didn't before

@NilsIrl NilsIrl force-pushed the NilsIrl:random branch from b1edcc5 to 8988c33 Jan 10, 2020
@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Jan 11, 2020

This is hiding a bug, not fixing it. Yes, that if is useless, but it's useless because it's not doing what it should be doing, which is passing a user option to the right place:

We have this parameter:

    -nogt --nogoptime: Never use GOP timing (use PTS), even if ccextractor
                       detects GOP timing is the reasonable choice.

Which I haven't tried but my guess is that it's broken since that if doesn't do anything.

You can grep that variable to get an idea

/lib_ccx/es_functions.c:		if (ccx_options.use_gop_as_pts==1)
./lib_ccx/es_functions.c:	if (ccx_options.use_gop_as_pts!=1)
./lib_ccx/ccx_common_option.c:	options->use_gop_as_pts = 0; // Use GOP instead of PTS timing (0=do as needed, 1=always, -1=never)
./lib_ccx/ccx_common_option.h:	int use_gop_as_pts;               // Use GOP instead of PTS timing (0=do as needed, 1=always, -1=never)
./lib_ccx/params_dump.c:	switch (ccx_options.use_gop_as_pts)
./lib_ccx/configuration.c:	{"GOP_TIME",offsetof(struct ccx_s_options,use_gop_as_pts),set_int},
./lib_ccx/params.c:			opt->use_gop_as_pts = 1;
./lib_ccx/params.c:			opt->use_gop_as_pts = -1; // Don't use even if we would want to
./lib_ccx/sequencing.c:	if (ccx_options.use_gop_as_pts==1)
./lib_ccx/sequencing.c:		// Since use_gop_as_pts this is not needed anymore, but left
./ccextractor.c:                if (!api_options.use_gop_as_pts) // If !0 then the user selected something
./ccextractor.c:                    api_options.use_gop_as_pts = 1; // Force GOP timing for ES
./ccextractor.c:                if (!api_options.use_gop_as_pts) // If !0 then the user selected something
./ccextractor.c:                    api_options.use_gop_as_pts = 0;

So can you fix the actual problem?

Copy link
Contributor

cfsmp3 left a comment

(see comment on conversation)

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 11, 2020

Note: this was added on the initial commit

@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Jan 11, 2020

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 11, 2020

Just because code is old or original it doesn't mean it's correct

Of course. It's just that I git blamed it to see why it had been changed to what it is.

And I thought it was best to report this to prevent someone wasting their time trying to figure out the same thing.

@NilsIrl

This comment has been minimized.

Copy link
Contributor Author

NilsIrl commented Jan 13, 2020

What's the difference between pts and gop?

There are files where the option is checked and stuff are done or aren't done depending on the value of the option.

@cfsmp3

This comment has been minimized.

Copy link
Contributor

cfsmp3 commented Jan 13, 2020

@NilsIrl NilsIrl force-pushed the NilsIrl:random branch from f70cedc to f992bc3 Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.