Mailing List Archive: Harmonizing mpegts-mythtv.(c|h) with FFmpeg's mpegts.(c|h)

Harmonizing mpegts-mythtv.(c|h) with FFmpeg's mpegts.(c|h)

Feb 15, 2022, 7:23 PM

Post #1 of 5 (515 views)

Peter,

My harmonization of mpegts-mythtv.* is almost complete. (This is on top
of my rebase/4.4m3 branch, but should have no issues being applied on
top of mythtv/FFmpeg/master.)

See https://github.com/ulmus-scott/FFmpeg/commits/harmonize for the
original commit order and
https://github.com/ulmus-scott/FFmpeg/commits/harmonize2 for reordered
with problematic commits moved to the end.

I have been testing with some of my local stations and the samples from
https://code.mythtv.org/trac/ticket/13557 /
https://github.com/MythTV/mythtv/issues/351 and everything is fine
through mpegts-mythtv.c: harmonize ff_parse_mpeg2_descriptor() part 1
<https://github.com/ulmus-scott/FFmpeg/commit/c22901ac83f9955f3846d33f79f4ddb8a07aeb8d>.

I have been banging my head against the brick wall that is pat_cb() and
pmt_cb(). (It started out fairly straightforward, but dealing with
those two functions and the 17 year old changes buried under updates has
been increasingly frustrating.)

mpegts-mythtv.c: pmt_cb(): harmonize part 6
<https://github.com/ulmus-scott/FFmpeg/commit/58cc290d552169f2388e83384e1a28a22a710ae7>
appears to actually work fine; however, the logic for calling the
streams_changed() callback is wrong, so it isn't called when it was
previously. Correcting it
<https://github.com/ulmus-scott/FFmpeg/commit/8a8dc3d5599bd61368c26ed410248cad20f7fa25>,
sort of (it updates on every different PMT, instead of if the streams
actually changed), causes a segmentation fault in
https://github.com/MythTV/mythtv/blob/709d05c0d6928e927ea1b0cc2b2c46bc80d40055/mythtv/libs/libmythtv/decoders/avformatdecoder.cpp#L2652
.

I suspect that is because the PSIPTable does not create a copy of its
input data, which is then freed while the PMT table is being accessed.

mpegts-mythtv.c: pat_cb() harmonization part 3
<https://github.com/ulmus-scott/FFmpeg/commit/6887b3127bcc2b8211e0fd7ab9828965ffe847f0>
causes a different segmentation fault in FFmpeg (dump.c trying to read
the st->metadata AVDictionary for the "language"). I wouldn't worry
about this one right now since I feel pmt_cb() has a higher priority, so
ff_parse_mpeg2_descriptor() can be minimally modified.

Let me know if you want me to open another set of pull requests with the
"good" commits for easier testing.

Any thoughts on how to complete the harmonization would also be appreciated.

Regards,

Scott

Re: Harmonizing mpegts-mythtv.(c|h) with FFmpeg's mpegts.(c|h) [ In reply to ]

pb.mythtv at gmail

Feb 17, 2022, 6:29 AM

Post #2 of 5 (513 views)

Permalink

On 2/15/22 22:23, Scott Theisen wrote:
> Peter,
>
> My harmonization of mpegts-mythtv.* is almost complete. (This is on
> top of my rebase/4.4m3 branch, but should have no issues being applied
> on top of mythtv/FFmpeg/master.)
>
> See https://github.com/ulmus-scott/FFmpeg/commits/harmonize for the
> original commit order and
> https://github.com/ulmus-scott/FFmpeg/commits/harmonize2 for reordered
> with problematic commits moved to the end.
>
> I have been testing with some of my local stations and the samples
> from https://code.mythtv.org/trac/ticket/13557 /
> https://github.com/MythTV/mythtv/issues/351 and everything is fine
> through mpegts-mythtv.c: harmonize ff_parse_mpeg2_descriptor() part 1
> <https://github.com/ulmus-scott/FFmpeg/commit/c22901ac83f9955f3846d33f79f4ddb8a07aeb8d>.
>
> I have been banging my head against the brick wall that is pat_cb()
> and pmt_cb(). (It started out fairly straightforward, but dealing
> with those two functions and the 17 year old changes buried under
> updates has been increasingly frustrating.)
>
> mpegts-mythtv.c: pmt_cb(): harmonize part 6
> <https://github.com/ulmus-scott/FFmpeg/commit/58cc290d552169f2388e83384e1a28a22a710ae7>
> appears to actually work fine; however, the logic for calling the
> streams_changed() callback is wrong, so it isn't called when it was
> previously. Correcting it
> <https://github.com/ulmus-scott/FFmpeg/commit/8a8dc3d5599bd61368c26ed410248cad20f7fa25>,
> sort of (it updates on every different PMT, instead of if the streams
> actually changed), causes a segmentation fault in
> https://github.com/MythTV/mythtv/blob/709d05c0d6928e927ea1b0cc2b2c46bc80d40055/mythtv/libs/libmythtv/decoders/avformatdecoder.cpp#L2652
> .
>
> I suspect that is because the PSIPTable does not create a copy of its
> input data, which is then freed while the PMT table is being accessed.
>
> mpegts-mythtv.c: pat_cb() harmonization part 3
> <https://github.com/ulmus-scott/FFmpeg/commit/6887b3127bcc2b8211e0fd7ab9828965ffe847f0>
> causes a different segmentation fault in FFmpeg (dump.c trying to read
> the st->metadata AVDictionary for the "language"). I wouldn't worry
> about this one right now since I feel pmt_cb() has a higher priority,
> so ff_parse_mpeg2_descriptor() can be minimally modified.
>
>
> Let me know if you want me to open another set of pull requests with
> the "good" commits for easier testing.
>
> Any thoughts on how to complete the harmonization would also be
> appreciated.
>
> Regards,
>
> Scott

I don't think I can help much here. I have stayed away from the
internals of FFmpeg except where necessary to fix errors when merging
new versions. I do not have any understanding of the internals of
Transport Streams or why MythTV needs to make so many changes to FFmpeg.
The changes that were done are very old, as you have noted, and were
done by others who are no longer around. I don't know if any other
developers of MythTV have better insight into this.

Peter

Re: Harmonizing mpegts-mythtv.(c|h) with FFmpeg's mpegts.(c|h) [ In reply to ]

johnpilk222 at gmail

Feb 17, 2022, 7:01 AM

Post #3 of 5 (513 views)

Permalink

On 17/02/2022 14:29, Peter Bennett wrote:
>
> On 2/15/22 22:23, Scott Theisen wrote:
>> Peter,
>>
>> My harmonization of mpegts-mythtv.* is almost complete. (This is on
>> top of my rebase/4.4m3 branch, but should have no issues being applied
>> on top of mythtv/FFmpeg/master.)
>>
>> See https://github.com/ulmus-scott/FFmpeg/commits/harmonize for the
>> original commit order and
>> https://github.com/ulmus-scott/FFmpeg/commits/harmonize2 for reordered
>> with problematic commits moved to the end.
>>
>> I have been testing with some of my local stations and the samples
>> from https://code.mythtv.org/trac/ticket/13557 /
>> https://github.com/MythTV/mythtv/issues/351 and everything is fine
>> through mpegts-mythtv.c: harmonize ff_parse_mpeg2_descriptor() part 1
>> <https://github.com/ulmus-scott/FFmpeg/commit/c22901ac83f9955f3846d33f79f4ddb8a07aeb8d>.
>>
>> I have been banging my head against the brick wall that is pat_cb()
>> and pmt_cb(). (It started out fairly straightforward, but dealing
>> with those two functions and the 17 year old changes buried under
>> updates has been increasingly frustrating.)
>>
>> mpegts-mythtv.c: pmt_cb(): harmonize part 6
>> <https://github.com/ulmus-scott/FFmpeg/commit/58cc290d552169f2388e83384e1a28a22a710ae7>
>> appears to actually work fine; however, the logic for calling the
>> streams_changed() callback is wrong, so it isn't called when it was
>> previously. Correcting it
>> <https://github.com/ulmus-scott/FFmpeg/commit/8a8dc3d5599bd61368c26ed410248cad20f7fa25>,
>> sort of (it updates on every different PMT, instead of if the streams
>> actually changed), causes a segmentation fault in
>> https://github.com/MythTV/mythtv/blob/709d05c0d6928e927ea1b0cc2b2c46bc80d40055/mythtv/libs/libmythtv/decoders/avformatdecoder.cpp#L2652
>> .
>>
>> I suspect that is because the PSIPTable does not create a copy of its
>> input data, which is then freed while the PMT table is being accessed.
>>
>> mpegts-mythtv.c: pat_cb() harmonization part 3
>> <https://github.com/ulmus-scott/FFmpeg/commit/6887b3127bcc2b8211e0fd7ab9828965ffe847f0>
>> causes a different segmentation fault in FFmpeg (dump.c trying to read
>> the st->metadata AVDictionary for the "language"). I wouldn't worry
>> about this one right now since I feel pmt_cb() has a higher priority,
>> so ff_parse_mpeg2_descriptor() can be minimally modified.
>>
>>
>> Let me know if you want me to open another set of pull requests with
>> the "good" commits for easier testing.
>>
>> Any thoughts on how to complete the harmonization would also be
>> appreciated.
>>
>> Regards,
>>
>> Scott
>
> I don't think I can help much here. I have stayed away from the
> internals of FFmpeg except where necessary to fix errors when merging
> new versions. I do not have any understanding of the internals of
> Transport Streams or why MythTV needs to make so many changes to FFmpeg.
> The changes that were done are very old, as you have noted, and were
> done by others who are no longer around. I don't know if any other
> developers of MythTV have better insight into this.
>
> Peter

I'm not a MythTV developer or a serious coder; I've just been using and
taking an interest in it for many years. But IIRC the project started
using its own 'fork' of ffmpeg because unexpected changes made by the
ffmpeg team could immediately affect MythTV users, whose maintainers
would then have to try to pick up the pieces.

Doesn't this still seem likely if 'harmonisation' goes too far ?

John P
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org

Re: Harmonizing mpegts-mythtv.(c|h) with FFmpeg's mpegts.(c|h) [ In reply to ]

klaas.de.waal at gmail

Feb 17, 2022, 11:11 AM

Post #4 of 5 (513 views)

Permalink

The basic reason why there is a MythTV version of the ffmpeg demuxer is
that the ffmpeg demuxer does not handle changes in streams very well. E.g.
changes in subtitling, changes in video format etc. So the ffmpeg demuxer
works OK for playing a stream that does not change. In general, MythTV
recordings extend beyond single programs and can also include commercials,
other breaks etc. Going from one program to another or from a program to a
commercial to another program is where the pain is. My latest changes to
the MythTV demuxer have been done to support the broadcast streams from YLE
which happen to have lots of changes.
And the remark of JohnP is also valid, for example during the last update
of ffmpeg in december or so there was suddenly the "blocking at skip"
problem which I bisected back to a specific ffmpeg commit. This commit
Peter then excluded from the MythTV version of ffmpeg. If we want to use
the ffmpeg libraries from the system then we must have a way to do runtime
patching the ffmpeg shared libraries. These things can be done using what
is called intercepts and library pre-loads but this must then be done for
every ffmpeg version on every supported system. And this is the kind of
software that should be used for testing and not for products.
So I think that we will have to stick to our own version of ffmpeg.

Klaas.

On Thu, 17 Feb 2022 at 16:02, John Pilkington <johnpilk222@gmail.com> wrote:

> On 17/02/2022 14:29, Peter Bennett wrote:
> >
> > On 2/15/22 22:23, Scott Theisen wrote:
> >> Peter,
> >>
> >> My harmonization of mpegts-mythtv.* is almost complete. (This is on
> >> top of my rebase/4.4m3 branch, but should have no issues being applied
> >> on top of mythtv/FFmpeg/master.)
> >>
> >> See https://github.com/ulmus-scott/FFmpeg/commits/harmonize for the
> >> original commit order and
> >> https://github.com/ulmus-scott/FFmpeg/commits/harmonize2 for reordered
> >> with problematic commits moved to the end.
> >>
> >> I have been testing with some of my local stations and the samples
> >> from https://code.mythtv.org/trac/ticket/13557 /
> >> https://github.com/MythTV/mythtv/issues/351 and everything is fine
> >> through mpegts-mythtv.c: harmonize ff_parse_mpeg2_descriptor() part 1
> >> <
> https://github.com/ulmus-scott/FFmpeg/commit/c22901ac83f9955f3846d33f79f4ddb8a07aeb8d
> >.
> >>
> >> I have been banging my head against the brick wall that is pat_cb()
> >> and pmt_cb(). (It started out fairly straightforward, but dealing
> >> with those two functions and the 17 year old changes buried under
> >> updates has been increasingly frustrating.)
> >>
> >> mpegts-mythtv.c: pmt_cb(): harmonize part 6
> >> <
> https://github.com/ulmus-scott/FFmpeg/commit/58cc290d552169f2388e83384e1a28a22a710ae7>
>
> >> appears to actually work fine; however, the logic for calling the
> >> streams_changed() callback is wrong, so it isn't called when it was
> >> previously. Correcting it
> >> <
> https://github.com/ulmus-scott/FFmpeg/commit/8a8dc3d5599bd61368c26ed410248cad20f7fa25>,
>
> >> sort of (it updates on every different PMT, instead of if the streams
> >> actually changed), causes a segmentation fault in
> >>
> https://github.com/MythTV/mythtv/blob/709d05c0d6928e927ea1b0cc2b2c46bc80d40055/mythtv/libs/libmythtv/decoders/avformatdecoder.cpp#L2652
> >> .
> >>
> >> I suspect that is because the PSIPTable does not create a copy of its
> >> input data, which is then freed while the PMT table is being accessed.
> >>
> >> mpegts-mythtv.c: pat_cb() harmonization part 3
> >> <
> https://github.com/ulmus-scott/FFmpeg/commit/6887b3127bcc2b8211e0fd7ab9828965ffe847f0>
>
> >> causes a different segmentation fault in FFmpeg (dump.c trying to read
> >> the st->metadata AVDictionary for the "language"). I wouldn't worry
> >> about this one right now since I feel pmt_cb() has a higher priority,
> >> so ff_parse_mpeg2_descriptor() can be minimally modified.
> >>
> >>
> >> Let me know if you want me to open another set of pull requests with
> >> the "good" commits for easier testing.
> >>
> >> Any thoughts on how to complete the harmonization would also be
> >> appreciated.
> >>
> >> Regards,
> >>
> >> Scott
> >
> > I don't think I can help much here. I have stayed away from the
> > internals of FFmpeg except where necessary to fix errors when merging
> > new versions. I do not have any understanding of the internals of
> > Transport Streams or why MythTV needs to make so many changes to FFmpeg.
> > The changes that were done are very old, as you have noted, and were
> > done by others who are no longer around. I don't know if any other
> > developers of MythTV have better insight into this.
> >
> > Peter
>
> I'm not a MythTV developer or a serious coder; I've just been using and
> taking an interest in it for many years. But IIRC the project started
> using its own 'fork' of ffmpeg because unexpected changes made by the
> ffmpeg team could immediately affect MythTV users, whose maintainers
> would then have to try to pick up the pieces.
>
> Doesn't this still seem likely if 'harmonisation' goes too far ?
>
> John P
> _______________________________________________
> mythtv-dev mailing list
> mythtv-dev@mythtv.org
> http://lists.mythtv.org/mailman/listinfo/mythtv-dev
> http://wiki.mythtv.org/Mailing_List_etiquette
> MythTV Forums: https://forum.mythtv.org
>

Re: Harmonizing mpegts-mythtv.(c|h) with FFmpeg's mpegts.(c|h) [ In reply to ]

scott.the.elm at gmail

Feb 17, 2022, 11:45 AM

Post #5 of 5 (513 views)

Permalink

On 2/17/22 10:01, John Pilkington wrote:
> I'm not a MythTV developer or a serious coder; I've just been using
> and taking an interest in it for many years. But IIRC the project
> started using its own 'fork' of ffmpeg because unexpected changes made
> by the ffmpeg team could immediately affect MythTV users, whose
> maintainers would then have to try to pick up the pieces.

Maintaining downstream changes is also a maintenance burden.

>
> Doesn't this still seem likely if 'harmonisation' goes too far ?

This is why I reordered the commits, because some proved problematic.

On 2/17/22 14:11, Klaas de Waal wrote:
> The basic reason why there is a MythTV version of the ffmpeg demuxer
> is that the ffmpeg demuxer does not handle changes in streams very
> well. E.g. changes in subtitling, changes in video format etc. So the
> ffmpeg demuxer works OK for playing a stream that does not change. In
> general, MythTV recordings extend beyond single programs and can also
> include commercials, other breaks etc. Going from one program to
> another or from a program to a commercial to another program is where
> the pain is. My latest changes to the MythTV demuxer have been done
> to support the broadcast streams from YLE which happen to have lots of
> changes.

That is why I was testing with those samples. It appeared to play fine
without exporting the PMT for MythTV, but I'll have to look at it again.

> And the remark of JohnP is also valid, for example during the last
> update of ffmpeg in december or so there was suddenly the "blocking at
> skip" problem which I bisected back to a specific ffmpeg commit. This
> commit Peter then excluded from the MythTV version of ffmpeg.

Klaas, you may want to ping that bug https://trac.ffmpeg.org/ticket/9532
since your upload link didn't initially work. Although, it might be
better to test with FFmpeg 5.0 first.

> If we want to use the ffmpeg libraries from the system then we must
> have a way to do runtime patching the ffmpeg shared libraries. These
> things can be done using what is called intercepts and library
> pre-loads but this must then be done for every ffmpeg version on every
> supported system. And this is the kind of software that should be used
> for testing and not for products.

I agree that doesn't seem feasible.

> So I think that we will have to stick to our own version of ffmpeg.
>

For now, yes. I still think being able to use the system's version of
ffmpeg is a good goal to work towards.

Scott
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org