[FFmpeg-devel] lavf/utils: update default wrap-around reference while iterating streams

Submitted by Jan Ekström on Feb. 6, 2019, 11:57 p.m.

Details

Message ID 20190206235740.6357-1-jeebjp@gmail.com
State New
Headers show

Commit Message

Jan Ekström Feb. 6, 2019, 11:57 p.m.
From: Masaki Tanaka <maki.rxrz@gmail.com>

Seems to fix mistaken cases of discontinuity handling in MPEG-TS
when program structure changes.

Additional changes to patch from its original state by Jan Ekström.
---

Had been meaning to post this for comments/discussion for a while, as
this seems to make timestamps continuously rising in at least one of
my samples where the program structure changes mid-stream.

The original version of this patch can be found at:
https://github.com/jeeb/ffmpeg/commit/6117366eaadbaf48bbd88eb2a353dfc852ff3400

The sample for which this helped is available at:
https://kuroko.fushizen.eu/samples/pid_switch_sample.ts

The difference of timestamps received from libavformat can be seen with:
https://kuroko.fushizen.eu/screenshots/ffmpeg/blue_vanilla-orange_patched.png

Jan

 libavformat/utils.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

Comments

Carl Eugen Hoyos Feb. 7, 2019, 12:22 a.m.
2019-02-07 0:57 GMT+01:00, Jan Ekström <jeebjp@gmail.com>:
> From: Masaki Tanaka <maki.rxrz@gmail.com>
>
> Seems to fix mistaken cases of discontinuity handling in MPEG-TS
> when program structure changes.
>
> Additional changes to patch from its original state by Jan Ekström.
> ---
>
> Had been meaning to post this for comments/discussion for a while, as
> this seems to make timestamps continuously rising in at least one of
> my samples where the program structure changes mid-stream.
>
> The original version of this patch can be found at:
> https://github.com/jeeb/ffmpeg/commit/6117366eaadbaf48bbd88eb2a353dfc852ff3400
>
> The sample for which this helped is available at:
> https://kuroko.fushizen.eu/samples/pid_switch_sample.ts
>
> The difference of timestamps received from libavformat can be seen with:
> https://kuroko.fushizen.eu/screenshots/ffmpeg/blue_vanilla-orange_patched.png

Am I correct that even with this patch, the sample cannot
be decoded / played correctly with FFmpeg / FFplay?

No opinion about the patch, just curious, Carl Eugen
Jan Ekström Feb. 7, 2019, 7:18 a.m.
On Thu, Feb 7, 2019, 02:22 Carl Eugen Hoyos <ceffmpeg@gmail.com wrote:

> 2019-02-07 0:57 GMT+01:00, Jan Ekström <jeebjp@gmail.com>:
> > From: Masaki Tanaka <maki.rxrz@gmail.com>
> >
> > Seems to fix mistaken cases of discontinuity handling in MPEG-TS
> > when program structure changes.
> >
> > Additional changes to patch from its original state by Jan Ekström.
> > ---
> >
> > Had been meaning to post this for comments/discussion for a while, as
> > this seems to make timestamps continuously rising in at least one of
> > my samples where the program structure changes mid-stream.
> >
> > The original version of this patch can be found at:
> >
> https://github.com/jeeb/ffmpeg/commit/6117366eaadbaf48bbd88eb2a353dfc852ff3400
> >
> > The sample for which this helped is available at:
> > https://kuroko.fushizen.eu/samples/pid_switch_sample.ts
> >
> > The difference of timestamps received from libavformat can be seen with:
> >
> https://kuroko.fushizen.eu/screenshots/ffmpeg/blue_vanilla-orange_patched.png
>
> Am I correct that even with this patch, the sample cannot
> be decoded / played correctly with FFmpeg / FFplay?
>
> No opinion about the patch, just curious, Carl Eugen
>

Yes, ffmpeg.c at least is static in its stream selection (and probably so
is ffplay.c) so they would ignore the other video stream either before or
after the switch depending on how much you probe and if you do manual
stream selection.

That said, this is a simple sample (no alternatives, just a clear cut
switch) so -merge_pmt_versions 1 should work for it to get both the sd and
hd parts decoded. This attempts to re-use a previous AVStream when a stream
is removed.

We do have the mechanism in mpegts.c to signal that a program's structure
has changed, but unfortunately it has not been utilized in ffmpeg.c.

Jan


>
Carl Eugen Hoyos Feb. 7, 2019, 1:02 p.m.
2019-02-07 8:18 GMT+01:00, Jan Ekström <jeebjp@gmail.com>:
> On Thu, Feb 7, 2019, 02:22 Carl Eugen Hoyos <ceffmpeg@gmail.com wrote:
>
>> 2019-02-07 0:57 GMT+01:00, Jan Ekström <jeebjp@gmail.com>:
>> > From: Masaki Tanaka <maki.rxrz@gmail.com>
>> >
>> > Seems to fix mistaken cases of discontinuity handling in MPEG-TS
>> > when program structure changes.
>> >
>> > Additional changes to patch from its original state by Jan Ekström.
>> > ---
>> >
>> > Had been meaning to post this for comments/discussion for a while, as
>> > this seems to make timestamps continuously rising in at least one of
>> > my samples where the program structure changes mid-stream.
>> >
>> > The original version of this patch can be found at:
>> >
>> https://github.com/jeeb/ffmpeg/commit/6117366eaadbaf48bbd88eb2a353dfc852ff3400
>> >
>> > The sample for which this helped is available at:
>> > https://kuroko.fushizen.eu/samples/pid_switch_sample.ts
>> >
>> > The difference of timestamps received from libavformat can be seen with:
>> >
>> https://kuroko.fushizen.eu/screenshots/ffmpeg/blue_vanilla-orange_patched.png
>>
>> Am I correct that even with this patch, the sample cannot
>> be decoded / played correctly with FFmpeg / FFplay?
>>
>> No opinion about the patch, just curious, Carl Eugen
>>
>
> Yes, ffmpeg.c at least is static in its stream selection (and probably so
> is ffplay.c) so they would ignore the other video stream either before or
> after the switch depending on how much you probe and if you do manual
> stream selection.
>
> That said, this is a simple sample (no alternatives, just a clear cut
> switch) so -merge_pmt_versions 1 should work for it to get both the sd and
> hd parts decoded. This attempts to re-use a previous AVStream when a stream
> is removed.

Thank you, I forgot about this switch: The original timestamp issue
is not reproducible with -merge_pmt_versions, right?

Carl Eugen
(who hopes you can use fieldmatch to watch this...)
Jan Ekström Feb. 7, 2019, 7:26 p.m.
On Thu, Feb 7, 2019 at 3:03 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2019-02-07 8:18 GMT+01:00, Jan Ekström <jeebjp@gmail.com>:
> > On Thu, Feb 7, 2019, 02:22 Carl Eugen Hoyos <ceffmpeg@gmail.com wrote:
> >
> >> 2019-02-07 0:57 GMT+01:00, Jan Ekström <jeebjp@gmail.com>:
> >> > From: Masaki Tanaka <maki.rxrz@gmail.com>
> >> >
> >> > Seems to fix mistaken cases of discontinuity handling in MPEG-TS
> >> > when program structure changes.
> >> >
> >> > Additional changes to patch from its original state by Jan Ekström.
> >> > ---
> >> >
> >> > Had been meaning to post this for comments/discussion for a while, as
> >> > this seems to make timestamps continuously rising in at least one of
> >> > my samples where the program structure changes mid-stream.
> >> >
> >> > The original version of this patch can be found at:
> >> >
> >> https://github.com/jeeb/ffmpeg/commit/6117366eaadbaf48bbd88eb2a353dfc852ff3400
> >> >
> >> > The sample for which this helped is available at:
> >> > https://kuroko.fushizen.eu/samples/pid_switch_sample.ts
> >> >
> >> > The difference of timestamps received from libavformat can be seen with:
> >> >
> >> https://kuroko.fushizen.eu/screenshots/ffmpeg/blue_vanilla-orange_patched.png
> >>
> >> Am I correct that even with this patch, the sample cannot
> >> be decoded / played correctly with FFmpeg / FFplay?
> >>
> >> No opinion about the patch, just curious, Carl Eugen
> >>
> >
> > Yes, ffmpeg.c at least is static in its stream selection (and probably so
> > is ffplay.c) so they would ignore the other video stream either before or
> > after the switch depending on how much you probe and if you do manual
> > stream selection.
> >
> > That said, this is a simple sample (no alternatives, just a clear cut
> > switch) so -merge_pmt_versions 1 should work for it to get both the sd and
> > hd parts decoded. This attempts to re-use a previous AVStream when a stream
> > is removed.
>
> Thank you, I forgot about this switch: The original timestamp issue
> is not reproducible with -merge_pmt_versions, right?
>

Just replicated my tests, and yes you are correct. The change seems to
happen because AVPrograms get updated as the programs change, but
current wrap reference will not get updated accordingly. The patch
seems to help but I am not 100% sure of the logic (given that taking
in both very high and low values makes it possible to pick a different
reference depending on the order of the streams within the AVProgram).

If someone's interested in how I checked (PID 0x121 is the one that
pops up first and thus merge_pmt_versions will only export it):
1. vanilla, with AVStream merging:
ffprobe pid_switch_sample.ts -select_streams "0#0x121" -show_frames
-of json -merge_pmt_versions 1 > 121_track_merged.json
2. vanilla, without AVStream merging:
ffprobe pid_switch_sample.ts -select_streams "0#0x121" -show_frames
-of json > 121_track.json
3. see the difference with a plotting script located @
http://up-cat.net/p/0f8b428a
pts_plotter.py 121_track.json  121_track_merged.json
4. patched, same command as step 2 as 121_track_patched.json or so
5. see the difference with the plotting script
pts_plotter.py 121_track.json  121_track_patched.json

You can also do the same with PID 111 (the HD PID) with regards to
vanilla VS patched. Results are rather similar.

> Carl Eugen
> (who hopes you can use fieldmatch to watch this...)

fieldmatch,decimate should be OK for the show itself, but generally I
am lazy and use yadif for non-encoding (viewing) cases. That makes
sure that all the useless advertising that often is fully interlaced
will also be correctly shown.

Jan
Carl Eugen Hoyos Feb. 7, 2019, 7:42 p.m.
2019-02-07 20:26 GMT+01:00, Jan Ekström <jeebjp@gmail.com>:
> On Thu, Feb 7, 2019 at 3:03 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

>> Carl Eugen
>> (who hopes you can use fieldmatch to watch this...)
>
> fieldmatch,decimate should be OK for the show itself, but generally I

decimate seems unneeded for this specific sample.

> am lazy and use yadif for non-encoding (viewing) cases. That makes
> sure that all the useless advertising that often is fully interlaced
> will also be correctly shown.

Of course!

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 7afef545fe..0efb1aae9c 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -815,8 +815,41 @@  static int update_wrap_reference(AVFormatContext *s, AVStream *st, int stream_in
         while (program) {
             if (program->pts_wrap_reference != pts_wrap_reference) {
                 for (i = 0; i<program->nb_stream_indexes; i++) {
-                    s->streams[program->stream_index[i]]->pts_wrap_reference = pts_wrap_reference;
-                    s->streams[program->stream_index[i]]->pts_wrap_behavior = pts_wrap_behavior;
+                    int64_t *stream_pts_wrap_reference = &(s->streams[program->stream_index[i]]->pts_wrap_reference);
+                    int     *stream_pts_wrap_behavior  = &(s->streams[program->stream_index[i]]->pts_wrap_behavior);
+
+                    if (*stream_pts_wrap_reference != AV_NOPTS_VALUE &&
+                        (*stream_pts_wrap_reference - pts_wrap_reference > 1ULL << (st->pts_wrap_bits - 3) ||
+                         *stream_pts_wrap_reference < pts_wrap_reference)) {
+                        /*
+                         * If we find a defined wrap reference that is
+                         * considerably larger, or that is smaller than the
+                         * current default wrap reference, we update
+                         * the default to current stream's wrap reference.
+                         */
+                        av_log(s, AV_LOG_DEBUG,
+                               "Updating default PTS wrap reference "
+                               "%"PRId64" and PTS wrap behavior %d to "
+                               "stream PTS wrap reference %"PRId64" and "
+                               "PTS wrap behavior %d (program: %d, stream:%d, "
+                               "stream_id: %d)\n", pts_wrap_reference,
+                               pts_wrap_behavior,
+                               *stream_pts_wrap_reference,
+                               *stream_pts_wrap_behavior,
+                               program->id, program->stream_index[i],
+                               s->streams[program->stream_index[i]]->id);
+
+                        pts_wrap_reference = *stream_pts_wrap_reference;
+                        pts_wrap_behavior  = *stream_pts_wrap_behavior;
+                    } else {
+                        /*
+                         * Otherwise, we just utilize and override the
+                         * stream's wrap-around reference with the default
+                         * wrap-around reference.
+                         */
+                        *stream_pts_wrap_reference = pts_wrap_reference;
+                        *stream_pts_wrap_behavior  = pts_wrap_behavior;
+                    }
                 }
 
                 program->pts_wrap_reference = pts_wrap_reference;