[QUESTION] get_video_min_pts function not called anywhere #614

Closed
opened 2026-01-29 16:49:09 +00:00 by claunia · 6 comments
Owner

Originally created by @siv2r on GitHub (Mar 4, 2021).

CCExtractor version: master branch (4 Mar 2021)

Necessary information

  • Is this a regression (i.e. did it work before)? {NO}
  • What platform did you use? {Linux}
  • What were the used arguments? No arguments used.

Additional information

I noticed that the get_video_min_pts function is not used anywhere.
cf84757e02/src/lib_ccx/ts_functions.c (L667-L668)

Also the code which calls get_video_min_pts is commented.
cf84757e02/src/lib_ccx/ts_functions.c (L746)

Is there any specific reason for this?

Originally created by @siv2r on GitHub (Mar 4, 2021). CCExtractor version: master branch (4 Mar 2021) # Necessary information - Is this a regression (i.e. did it work before)? {NO} - What platform did you use? {Linux} - What were the used arguments? `No arguments used.` # Additional information I noticed that the `get_video_min_pts` function is not used anywhere. https://github.com/CCExtractor/ccextractor/blob/cf84757e02889ffa0231624fdf941eafded59906/src/lib_ccx/ts_functions.c#L667-L668 Also the code which calls `get_video_min_pts` is commented. https://github.com/CCExtractor/ccextractor/blob/cf84757e02889ffa0231624fdf941eafded59906/src/lib_ccx/ts_functions.c#L746 Is there any specific reason for this?
Author
Owner

@NilsIrl commented on GitHub (Mar 4, 2021):

may be relevant, the commit where this got commented out: 462f63a294 (diff-8ea29240ddcd2096742634831a5e214ec954ed35d04dc554742d1e65592a026cR734-R735)

@NilsIrl commented on GitHub (Mar 4, 2021): may be relevant, the commit where this got commented out: https://github.com/CCExtractor/ccextractor/commit/462f63a294313021706417b6c186a1c8d33c4a07#diff-8ea29240ddcd2096742634831a5e214ec954ed35d04dc554742d1e65592a026cR734-R735
Author
Owner

@cfsmp3 commented on GitHub (Mar 28, 2021):

Well, the commit says "fixes DVB multiprogram" :-) It's from 4 years ago. @alexbrt did this during GCI. I'm not sure he's still alive? :-)
Alex?

But the commit description itself is a good clue to what to test next.

https://drive.google.com/drive/u/5/folders/0B_61ywKPmI0TQTZEMm5Tajd5RVU

(or other DVB samples, looking specifically for ones with more than one stream)

@cfsmp3 commented on GitHub (Mar 28, 2021): Well, the commit says "fixes DVB multiprogram" :-) It's from 4 years ago. @alexbrt did this during GCI. I'm not sure he's still alive? :-) Alex? But the commit description itself is a good clue to what to test next. https://drive.google.com/drive/u/5/folders/0B_61ywKPmI0TQTZEMm5Tajd5RVU (or other DVB samples, looking specifically for ones with more than one stream)
Author
Owner

@KW781 commented on GitHub (Apr 13, 2021):

I should preface by saying that I'm somewhat new to the CCExtractor codebase, especially compared to some of the major contributors to CCExtractor, but I decided to report my findings anyway.

I spent some time digging through all the files in lib_ccx, and that function wasn't called in any other C file either. So it's clear that the function was written to be used within ts_functions.c only. From looking at the structure definition of ccx_demuxer and at the definition of UINT64_MAX, it looks like UINT64_MAX (the maximum width of an int64 type), is the default value for the got_important_streams_min_pts[VIDEO] member for that structure. So basically, it's checking whether the minimum presentation time stamp for that video stream is the default, and if so, finds the minimum presentation time stamp for that video stream.

To try and figure out why it was commented out, I traced through all the function calls to main(). get_video_min_pts() is called in ts_readstream(), ts_readstream() is called in ts_get_more_data, ts_get_more_data() is called in general_loop(), general_loop() is called in api_start() and then api_start() is called in main(). What I noticed in all of these functions is that the member got_important_streams_min_pts[VIDEO] for *ctx is not referenced at all, except for in that code that is commented out. So my guess is that the code was commented out because that member wasn't being used at all, which is why it didn't need to be assigned a value from get_video_min_pts().

I assume that the function exists just to find the minimum presentation time stamp for any given video stream, if required. We could get rid of the comment on that code, since it might be good practice to populate that member anyway, and to avoid future confusion like this. But it's probably up to the more senior contributors to decide on whether to do this or not.

@KW781 commented on GitHub (Apr 13, 2021): I should preface by saying that I'm somewhat new to the CCExtractor codebase, especially compared to some of the major contributors to CCExtractor, but I decided to report my findings anyway. I spent some time digging through all the files in lib_ccx, and that function wasn't called in any other C file either. So it's clear that the function was written to be used within ts_functions.c only. From looking at the structure definition of ccx_demuxer and at the definition of UINT64_MAX, it looks like UINT64_MAX (the maximum width of an int64 type), is the default value for the got_important_streams_min_pts[VIDEO] member for that structure. So basically, it's checking whether the minimum presentation time stamp for that video stream is the default, and if so, finds the minimum presentation time stamp for that video stream. To try and figure out why it was commented out, I traced through all the function calls to main(). get_video_min_pts() is called in ts_readstream(), ts_readstream() is called in ts_get_more_data, ts_get_more_data() is called in general_loop(), general_loop() is called in api_start() and then api_start() is called in main(). What I noticed in all of these functions is that the member got_important_streams_min_pts[VIDEO] for *ctx is not referenced at all, except for in that code that is commented out. So my guess is that the code was commented out because that member wasn't being used at all, which is why it didn't need to be assigned a value from get_video_min_pts(). I assume that the function exists just to find the minimum presentation time stamp for any given video stream, if required. We could get rid of the comment on that code, since it might be good practice to populate that member anyway, and to avoid future confusion like this. But it's probably up to the more senior contributors to decide on whether to do this or not.
Author
Owner

@alexbrt commented on GitHub (Apr 13, 2021):

@cfsmp3 I am not dead (yet), haha, just very busy with work and university.

@KW781 @siv2r I do have some arcane findings though which could explain the confusion. Fixing overall DVB support was done in 2 stages, the first one being for single-program streams followed by the multi-program commit (462f63a) referenced in this issue.

Quoting from my GCI presentation, the reason I commented out that function:

Instead of having a unique arbitrary PTS for each program we were using a global minimum PTS which was the "minimum PTS" of the first decoded program...

I can only conclude that get_video_min_pts should have been completely deleted instead of being commented out, and is now just a leftover from the old code.

To further clear things up, this is the presentation I'm talking about which briefly summarizes @Izaron's and my work:
https://docs.google.com/presentation/d/14hKczN-fbeTZLd_CeZQILZYz4Ylnhaj05PEEwEd_Q7c/edit?usp=sharing

@alexbrt commented on GitHub (Apr 13, 2021): @cfsmp3 I am not dead (yet), haha, just very busy with work and university. @KW781 @siv2r I do have some arcane findings though which could explain the confusion. Fixing overall DVB support was done in 2 stages, the first one being for single-program streams followed by the multi-program commit (462f63a) referenced in this issue. Quoting from my GCI presentation, the reason I commented out that function: > Instead of having a unique arbitrary PTS for each program we were using a global minimum PTS which was the "minimum PTS" of the first decoded program... I can only conclude that `get_video_min_pts` should have been completely deleted instead of being commented out, and is now just a leftover from the old code. To further clear things up, this is the presentation I'm talking about which briefly summarizes @Izaron's and my work: https://docs.google.com/presentation/d/14hKczN-fbeTZLd_CeZQILZYz4Ylnhaj05PEEwEd_Q7c/edit?usp=sharing
Author
Owner

@KW781 commented on GitHub (Apr 13, 2021):

@alexbrt Ah I see, so that's why that member (got_important_streams_min_pts[VIDEO])for *ctx wasn't being used at all?

@KW781 commented on GitHub (Apr 13, 2021): @alexbrt Ah I see, so that's why that member (got_important_streams_min_pts[VIDEO])for *ctx wasn't being used at all?
Author
Owner

@cfsmp3 commented on GitHub (May 2, 2021):

Function removed

@cfsmp3 commented on GitHub (May 2, 2021): Function removed
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/ccextractor#614