| Message ID | 59884611570452895@iva5-58d151f416d2.qloud-c.yandex.net |
|---|---|
| State | New |
| Headers | show |
Anything else I have to do to make this patch eventually taken in? Thanks, Vasily пн, 7 окт. 2019 г. в 15:54, <just.one.man@yandex.ru>: > When incoming media has non-zero start PTS, > segment muxer would fail to correctly calculate > the point where to chunk segments, as it always > assumed that media starts with PTS==0. > > This change removes this assumption by remembering > the PTS of the very first frame passed through the muxer. > > Also fix starting points of first segment > --- > libavformat/segment.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/libavformat/segment.c b/libavformat/segment.c > index e308206..8b985df 100644 > --- a/libavformat/segment.c > +++ b/libavformat/segment.c > @@ -87,6 +87,7 @@ typedef struct SegmentContext { > int64_t last_val; ///< remember last time for wrap around > detection > int cut_pending; > int header_written; ///< whether we've already called > avformat_write_header > + int64_t start_pts; ///< pts of the very first packet processed, > used to compute correct segment length > > char *entry_prefix; ///< prefix to add to list entry filenames > int list_type; ///< set the list type > @@ -702,6 +703,7 @@ static int seg_init(AVFormatContext *s) > if ((ret = parse_frames(s, &seg->frames, &seg->nb_frames, > seg->frames_str)) < 0) > return ret; > } else { > + seg->start_pts = AV_NOPTS_VALUE; > /* set default value if not specified */ > if (!seg->time_str) > seg->time_str = av_strdup("2"); > @@ -914,7 +916,15 @@ calc_times: > seg->cut_pending = 1; > seg->last_val = wrapped_val; > } else { > - end_pts = seg->time * (seg->segment_count + 1); > + if (seg->start_pts != AV_NOPTS_VALUE) { > + end_pts = seg->start_pts + seg->time * > (seg->segment_count + 1); > + } else if (pkt->stream_index == seg->reference_stream_index > && pkt->pts != AV_NOPTS_VALUE) { > + // this is the first packet of the reference stream we > see, initialize start point > + seg->start_pts = av_rescale_q(pkt->pts, st->time_base, > AV_TIME_BASE_Q); > + seg->cur_entry.start_time = (double)pkt->pts * > av_q2d(st->time_base); > + seg->cur_entry.start_pts = seg->start_pts; > + end_pts = seg->start_pts + seg->time * > (seg->segment_count + 1); > + } > } > } > > -- > 1.7.9.5 > > Now I tried to re-format as Nicolas George said, hopefully this is okay > and won't make a third entry in patchwork (if it does - how do I stop it > from doing so?) > > 07.10.2019, 15:15, "Nicolas George" <george@nsup.org>: > > just.one.man@yandex.ru (12019-10-07): > >> Updated patch > > > > This should be after the triple dash, because it does not belong in the > > commit message. > > > >> --- > >> > >> When incoming media has non-zero start PTS, > >> segment muxer would fail to correctly calculate > >> the point where to chunk segments, as it always > >> assumed that media starts with PTS==0. > >> > >> This change removes this assumption by remembering > >> the PTS of the very first frame passed through the muxer. > >> > >> Also fix starting points of first segment > >> --- > > > > This should be before the triple dash, because it does belong in the > > commit message. > > > > I did not look at the code. > > > > Regards, > > > > -- > > Nicolas George > > , > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Can I have _any_ response please? It was more than a week of silence... :( Thanks, Vasily чт, 10 окт. 2019 г., 13:05 Vasily <just.one.man@yandex.ru>: > Anything else I have to do to make this patch eventually taken in? > > Thanks, > Vasily > > пн, 7 окт. 2019 г. в 15:54, <just.one.man@yandex.ru>: > >> When incoming media has non-zero start PTS, >> segment muxer would fail to correctly calculate >> the point where to chunk segments, as it always >> assumed that media starts with PTS==0. >> >> This change removes this assumption by remembering >> the PTS of the very first frame passed through the muxer. >> >> Also fix starting points of first segment >> --- >> libavformat/segment.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/libavformat/segment.c b/libavformat/segment.c >> index e308206..8b985df 100644 >> --- a/libavformat/segment.c >> +++ b/libavformat/segment.c >> @@ -87,6 +87,7 @@ typedef struct SegmentContext { >> int64_t last_val; ///< remember last time for wrap around >> detection >> int cut_pending; >> int header_written; ///< whether we've already called >> avformat_write_header >> + int64_t start_pts; ///< pts of the very first packet processed, >> used to compute correct segment length >> >> char *entry_prefix; ///< prefix to add to list entry filenames >> int list_type; ///< set the list type >> @@ -702,6 +703,7 @@ static int seg_init(AVFormatContext *s) >> if ((ret = parse_frames(s, &seg->frames, &seg->nb_frames, >> seg->frames_str)) < 0) >> return ret; >> } else { >> + seg->start_pts = AV_NOPTS_VALUE; >> /* set default value if not specified */ >> if (!seg->time_str) >> seg->time_str = av_strdup("2"); >> @@ -914,7 +916,15 @@ calc_times: >> seg->cut_pending = 1; >> seg->last_val = wrapped_val; >> } else { >> - end_pts = seg->time * (seg->segment_count + 1); >> + if (seg->start_pts != AV_NOPTS_VALUE) { >> + end_pts = seg->start_pts + seg->time * >> (seg->segment_count + 1); >> + } else if (pkt->stream_index == seg->reference_stream_index >> && pkt->pts != AV_NOPTS_VALUE) { >> + // this is the first packet of the reference stream we >> see, initialize start point >> + seg->start_pts = av_rescale_q(pkt->pts, st->time_base, >> AV_TIME_BASE_Q); >> + seg->cur_entry.start_time = (double)pkt->pts * >> av_q2d(st->time_base); >> + seg->cur_entry.start_pts = seg->start_pts; >> + end_pts = seg->start_pts + seg->time * >> (seg->segment_count + 1); >> + } >> } >> } >> >> -- >> 1.7.9.5 >> >> Now I tried to re-format as Nicolas George said, hopefully this is okay >> and won't make a third entry in patchwork (if it does - how do I stop it >> from doing so?) >> >> 07.10.2019, 15:15, "Nicolas George" <george@nsup.org>: >> > just.one.man@yandex.ru (12019-10-07): >> >> Updated patch >> > >> > This should be after the triple dash, because it does not belong in the >> > commit message. >> > >> >> --- >> >> >> >> When incoming media has non-zero start PTS, >> >> segment muxer would fail to correctly calculate >> >> the point where to chunk segments, as it always >> >> assumed that media starts with PTS==0. >> >> >> >> This change removes this assumption by remembering >> >> the PTS of the very first frame passed through the muxer. >> >> >> >> Also fix starting points of first segment >> >> --- >> > >> > This should be before the triple dash, because it does belong in the >> > commit message. >> > >> > I did not look at the code. >> > >> > Regards, >> > >> > -- >> > Nicolas George >> > , >> > >> > _______________________________________________ >> > ffmpeg-devel mailing list >> > ffmpeg-devel@ffmpeg.org >> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > >> > To unsubscribe, visit link above, or email >> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > >
Can anyone answer me?.. :( чт, 17 окт. 2019 г., 12:24 Vasily <just.one.man@yandex.ru>: > Can I have _any_ response please? It was more than a week of silence... :( > > Thanks, > Vasily > > чт, 10 окт. 2019 г., 13:05 Vasily <just.one.man@yandex.ru>: > >> Anything else I have to do to make this patch eventually taken in? >> >> Thanks, >> Vasily >> >> пн, 7 окт. 2019 г. в 15:54, <just.one.man@yandex.ru>: >> >>> When incoming media has non-zero start PTS, >>> segment muxer would fail to correctly calculate >>> the point where to chunk segments, as it always >>> assumed that media starts with PTS==0. >>> >>> This change removes this assumption by remembering >>> the PTS of the very first frame passed through the muxer. >>> >>> Also fix starting points of first segment >>> --- >>> libavformat/segment.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavformat/segment.c b/libavformat/segment.c >>> index e308206..8b985df 100644 >>> --- a/libavformat/segment.c >>> +++ b/libavformat/segment.c >>> @@ -87,6 +87,7 @@ typedef struct SegmentContext { >>> int64_t last_val; ///< remember last time for wrap around >>> detection >>> int cut_pending; >>> int header_written; ///< whether we've already called >>> avformat_write_header >>> + int64_t start_pts; ///< pts of the very first packet processed, >>> used to compute correct segment length >>> >>> char *entry_prefix; ///< prefix to add to list entry filenames >>> int list_type; ///< set the list type >>> @@ -702,6 +703,7 @@ static int seg_init(AVFormatContext *s) >>> if ((ret = parse_frames(s, &seg->frames, &seg->nb_frames, >>> seg->frames_str)) < 0) >>> return ret; >>> } else { >>> + seg->start_pts = AV_NOPTS_VALUE; >>> /* set default value if not specified */ >>> if (!seg->time_str) >>> seg->time_str = av_strdup("2"); >>> @@ -914,7 +916,15 @@ calc_times: >>> seg->cut_pending = 1; >>> seg->last_val = wrapped_val; >>> } else { >>> - end_pts = seg->time * (seg->segment_count + 1); >>> + if (seg->start_pts != AV_NOPTS_VALUE) { >>> + end_pts = seg->start_pts + seg->time * >>> (seg->segment_count + 1); >>> + } else if (pkt->stream_index == seg->reference_stream_index >>> && pkt->pts != AV_NOPTS_VALUE) { >>> + // this is the first packet of the reference stream we >>> see, initialize start point >>> + seg->start_pts = av_rescale_q(pkt->pts, st->time_base, >>> AV_TIME_BASE_Q); >>> + seg->cur_entry.start_time = (double)pkt->pts * >>> av_q2d(st->time_base); >>> + seg->cur_entry.start_pts = seg->start_pts; >>> + end_pts = seg->start_pts + seg->time * >>> (seg->segment_count + 1); >>> + } >>> } >>> } >>> >>> -- >>> 1.7.9.5 >>> >>> Now I tried to re-format as Nicolas George said, hopefully this is okay >>> and won't make a third entry in patchwork (if it does - how do I stop it >>> from doing so?) >>> >>> 07.10.2019, 15:15, "Nicolas George" <george@nsup.org>: >>> > just.one.man@yandex.ru (12019-10-07): >>> >> Updated patch >>> > >>> > This should be after the triple dash, because it does not belong in the >>> > commit message. >>> > >>> >> --- >>> >> >>> >> When incoming media has non-zero start PTS, >>> >> segment muxer would fail to correctly calculate >>> >> the point where to chunk segments, as it always >>> >> assumed that media starts with PTS==0. >>> >> >>> >> This change removes this assumption by remembering >>> >> the PTS of the very first frame passed through the muxer. >>> >> >>> >> Also fix starting points of first segment >>> >> --- >>> > >>> > This should be before the triple dash, because it does belong in the >>> > commit message. >>> > >>> > I did not look at the code. >>> > >>> > Regards, >>> > >>> > -- >>> > Nicolas George >>> > , >>> > >>> > _______________________________________________ >>> > ffmpeg-devel mailing list >>> > ffmpeg-devel@ffmpeg.org >>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> > >>> > To unsubscribe, visit link above, or email >>> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> >>> To unsubscribe, visit link above, or email >>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> >>
On Mon, 7 Oct 2019, just.one.man@yandex.ru wrote: Please use a proper commit title: e.g: avformat/segment: fix non-zero start pts Also make sure you provide the author name you want when you send the patch email. (you only provided an email address in the From field, not a full name, I guess this is not intentional). > When incoming media has non-zero start PTS, > segment muxer would fail to correctly calculate > the point where to chunk segments, as it always > assumed that media starts with PTS==0. > > This change removes this assumption by remembering > the PTS of the very first frame passed through the muxer. > > Also fix starting points of first segment > --- > libavformat/segment.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/libavformat/segment.c b/libavformat/segment.c > index e308206..8b985df 100644 > --- a/libavformat/segment.c > +++ b/libavformat/segment.c > @@ -87,6 +87,7 @@ typedef struct SegmentContext { > int64_t last_val; ///< remember last time for wrap around detection > int cut_pending; > int header_written; ///< whether we've already called avformat_write_header > + int64_t start_pts; ///< pts of the very first packet processed, used to compute correct segment length > > char *entry_prefix; ///< prefix to add to list entry filenames > int list_type; ///< set the list type > @@ -702,6 +703,7 @@ static int seg_init(AVFormatContext *s) > if ((ret = parse_frames(s, &seg->frames, &seg->nb_frames, seg->frames_str)) < 0) > return ret; > } else { > + seg->start_pts = AV_NOPTS_VALUE; > /* set default value if not specified */ > if (!seg->time_str) > seg->time_str = av_strdup("2"); > @@ -914,7 +916,15 @@ calc_times: > seg->cut_pending = 1; > seg->last_val = wrapped_val; > } else { > - end_pts = seg->time * (seg->segment_count + 1); > + if (seg->start_pts != AV_NOPTS_VALUE) { > + end_pts = seg->start_pts + seg->time * (seg->segment_count + 1); > + } else if (pkt->stream_index == seg->reference_stream_index && pkt->pts != AV_NOPTS_VALUE) { What happens if the first packet is not from a reference stream? As far as I see in that case the output packet timestamp will be 0 based until we get a packet from the refence stream... Maybe you should accept a packet from any stream here? > + // this is the first packet of the reference stream we see, initialize start point > + seg->start_pts = av_rescale_q(pkt->pts, st->time_base, AV_TIME_BASE_Q); > + seg->cur_entry.start_time = (double)pkt->pts * av_q2d(st->time_base); > + seg->cur_entry.start_pts = seg->start_pts; > + end_pts = seg->start_pts + seg->time * (seg->segment_count + 1); > + } > } > } Regards, Marton
Hi Marton, > Please use a proper commit title: e.g: > > avformat/segment: fix non-zero start pts > > Also make sure you provide the author name you want when you send the > patch email. (you only provided an email address in the From field, not a > full name, I guess this is not intentional). I've fixed "From" and subject (so it's just my first name, I don't want to share my family name etc), hope it's okay now (or do I need to send a completely new patch with fixed From and subject instead?). > What happens if the first packet is not from a reference stream? As far as > I see in that case the output packet timestamp will be 0 based until we > get a packet from the refence stream... Maybe you should accept a packet > from any stream here? I am not changing output packet timestamps here, I only change the way the cutting point for segment muxer is calculated. Cutting is performed when a packet in a reference stream has greater pts than one determined as cutting point (and some other conditions are met), and before my patch that cutting point always assumed reference stream started at zero. So, as cutting point is used to determine whether current packet has to begin a new segment only by looking at reference stream, it doesn't seem correct to me to use any other streams as sources of "pts of first packet", as other streams could potentially use different start times. Cutting point should make segments as close to desired length as possible IMHO. --- Thanks, Vasily
On Thu, 31 Oct 2019, Vasily wrote: > Hi Marton, > >> Please use a proper commit title: e.g: >> >> avformat/segment: fix non-zero start pts >> >> Also make sure you provide the author name you want when you send the >> patch email. (you only provided an email address in the From field, not a >> full name, I guess this is not intentional). > > I've fixed "From" and subject (so it's just my first name, I don't want to share my family name etc), hope it's okay now (or do I need to send a completely new patch with fixed From and subject instead?). Yes, you should send a new patch. > > >> What happens if the first packet is not from a reference stream? As far as >> I see in that case the output packet timestamp will be 0 based until we >> get a packet from the refence stream... Maybe you should accept a packet >> from any stream here? > > I am not changing output packet timestamps here, I only change the way > the cutting point for segment muxer is calculated. You are setting these: + seg->cur_entry.start_time = (double)pkt->pts * av_q2d(st->time_base); + seg->cur_entry.start_pts = seg->start_pts; These can affect the packet PTSes of the first segment, because later there is code like this: /* compute new timestamps */ offset = av_rescale_q(seg->initial_offset - (seg->reset_timestamps ? seg->cur_entry.start_pts : 0), AV_TIME_BASE_Q, st->time_base); if (pkt->pts != AV_NOPTS_VALUE) pkt->pts += offset; if (pkt->dts != AV_NOPTS_VALUE) pkt->dts += offset; Regards, Marton
diff --git a/libavformat/segment.c b/libavformat/segment.c index e308206..8b985df 100644 --- a/libavformat/segment.c +++ b/libavformat/segment.c @@ -87,6 +87,7 @@ typedef struct SegmentContext { int64_t last_val; ///< remember last time for wrap around detection int cut_pending; int header_written; ///< whether we've already called avformat_write_header + int64_t start_pts; ///< pts of the very first packet processed, used to compute correct segment length char *entry_prefix; ///< prefix to add to list entry filenames int list_type; ///< set the list type @@ -702,6 +703,7 @@ static int seg_init(AVFormatContext *s) if ((ret = parse_frames(s, &seg->frames, &seg->nb_frames, seg->frames_str)) < 0) return ret; } else { + seg->start_pts = AV_NOPTS_VALUE; /* set default value if not specified */ if (!seg->time_str) seg->time_str = av_strdup("2"); @@ -914,7 +916,15 @@ calc_times: seg->cut_pending = 1; seg->last_val = wrapped_val; } else { - end_pts = seg->time * (seg->segment_count + 1); + if (seg->start_pts != AV_NOPTS_VALUE) { + end_pts = seg->start_pts + seg->time * (seg->segment_count + 1); + } else if (pkt->stream_index == seg->reference_stream_index && pkt->pts != AV_NOPTS_VALUE) { + // this is the first packet of the reference stream we see, initialize start point + seg->start_pts = av_rescale_q(pkt->pts, st->time_base, AV_TIME_BASE_Q); + seg->cur_entry.start_time = (double)pkt->pts * av_q2d(st->time_base); + seg->cur_entry.start_pts = seg->start_pts; + end_pts = seg->start_pts + seg->time * (seg->segment_count + 1); + } } }