RFC: mpegtsenc: Interpret a muxer flush as a request for new PAT/PMT/SDT

Message ID 1328860450-37317-2-git-send-email-martin@martin.st
State Deferred
Headers show

Commit Message

Martin Storsjö Feb. 10, 2012, 7:54 a.m.
These are written when the next packet is written.

This allows for similarity to custom segmentation/flushing as
for the ismv muxer, where flush can be used to output the
buffered data and write a fragment. On the other hand,
flush isn't really the right semantics for this particular
operation, so the AVOption approach might be more right.
---
 libavformat/mpegtsenc.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

Comments

Luca Barbato Feb. 10, 2012, 9:14 a.m. | #1
On 2/9/12 11:54 PM, Martin Storsjö wrote:
> These are written when the next packet is written.
>
> This allows for similarity to custom segmentation/flushing as
> for the ismv muxer, where flush can be used to output the
> buffered data and write a fragment. On the other hand,
> flush isn't really the right semantics for this particular
> operation, so the AVOption approach might be more right.

This one should work better with the segmenter.

lu
Janne Grunau Feb. 10, 2012, 10:16 a.m. | #2
On 2012-02-10 01:14:28 -0800, Luca Barbato wrote:
> On 2/9/12 11:54 PM, Martin Storsjö wrote:
> >These are written when the next packet is written.
> >
> >This allows for similarity to custom segmentation/flushing as
> >for the ismv muxer, where flush can be used to output the
> >buffered data and write a fragment. On the other hand,
> >flush isn't really the right semantics for this particular
> >operation, so the AVOption approach might be more right.
> 
> This one should work better with the segmenter.

how many formats do we want to support with the segmenter? I can see
that a custom AVOption per format is really inconvenient but using
flush semantics for outputting PAT+PMT feels wrong.

I would prefer a shared AVOption "start_new_segment" since I don't
think this warrants its own function pointer in AVOutputFormat.

Janne
Martin Storsjö Feb. 10, 2012, 10:26 a.m. | #3
On Fri, 10 Feb 2012, Janne Grunau wrote:

> On 2012-02-10 01:14:28 -0800, Luca Barbato wrote:
>> On 2/9/12 11:54 PM, Martin Storsjö wrote:
>>> These are written when the next packet is written.
>>>
>>> This allows for similarity to custom segmentation/flushing as
>>> for the ismv muxer, where flush can be used to output the
>>> buffered data and write a fragment. On the other hand,
>>> flush isn't really the right semantics for this particular
>>> operation, so the AVOption approach might be more right.
>>
>> This one should work better with the segmenter.
>
> how many formats do we want to support with the segmenter? I can see
> that a custom AVOption per format is really inconvenient but using
> flush semantics for outputting PAT+PMT feels wrong.
>
> I would prefer a shared AVOption "start_new_segment" since I don't
> think this warrants its own function pointer in AVOutputFormat.

That would be ok with me, too.

The problem with a generic segmenter is that even though the concepts are 
similar on a high level, there's lots and lots of differences in 
behaviour, so while this is a good and less mpegts-specific solution, I 
wouldn't want it for the ismv muxer.

In the ismv muxer, a start_new_segment flag wouldn't work really right, 
(while the flush operation we have there right now fits perfectly), since 
we wouldn't want to write any new data, just flush the existing one. For 
mpegts on the other hand, the muxer always already flushed all data, we 
just need to flag to tell it to resend "headers".

But yes, I'm ok with a flag like "start_new_segment" or "resend_headers" 
which would work for mpegts and would work for any other format with 
similar behaviour, too.

// Martin
Janne Grunau Feb. 10, 2012, 11:52 a.m. | #4
On 2012-02-10 12:26:14 +0200, Martin Storsjö wrote:
> On Fri, 10 Feb 2012, Janne Grunau wrote:
> 
> >On 2012-02-10 01:14:28 -0800, Luca Barbato wrote:
> >>On 2/9/12 11:54 PM, Martin Storsjö wrote:
> >>>These are written when the next packet is written.
> >>>
> >>>This allows for similarity to custom segmentation/flushing as
> >>>for the ismv muxer, where flush can be used to output the
> >>>buffered data and write a fragment. On the other hand,
> >>>flush isn't really the right semantics for this particular
> >>>operation, so the AVOption approach might be more right.
> >>
> >>This one should work better with the segmenter.
> >
> >how many formats do we want to support with the segmenter? I can see
> >that a custom AVOption per format is really inconvenient but using
> >flush semantics for outputting PAT+PMT feels wrong.
> >
> >I would prefer a shared AVOption "start_new_segment" since I don't
> >think this warrants its own function pointer in AVOutputFormat.
> 
> That would be ok with me, too.
> 
> The problem with a generic segmenter is that even though the
> concepts are similar on a high level, there's lots and lots of
> differences in behaviour, so while this is a good and less
> mpegts-specific solution, I wouldn't want it for the ismv muxer.
> 
> In the ismv muxer, a start_new_segment flag wouldn't work really
> right, (while the flush operation we have there right now fits
> perfectly), since we wouldn't want to write any new data, just flush
> the existing one. For mpegts on the other hand, the muxer always
> already flushed all data, we just need to flag to tell it to resend
> "headers".
> 
> But yes, I'm ok with a flag like "start_new_segment" or
> "resend_headers" which would work for mpegts and would work for any
> other format with similar behaviour, too.

I think it is preferable to seperate things needed to be done at the
end of a segment from things needed at the start of the segment.

Janne
Martin Storsjö Feb. 10, 2012, 1:05 p.m. | #5
On Fri, 10 Feb 2012, Janne Grunau wrote:

> On 2012-02-10 12:26:14 +0200, Martin Storsjö wrote:
>> On Fri, 10 Feb 2012, Janne Grunau wrote:
>>
>>> On 2012-02-10 01:14:28 -0800, Luca Barbato wrote:
>>>> On 2/9/12 11:54 PM, Martin Storsjö wrote:
>>>>> These are written when the next packet is written.
>>>>>
>>>>> This allows for similarity to custom segmentation/flushing as
>>>>> for the ismv muxer, where flush can be used to output the
>>>>> buffered data and write a fragment. On the other hand,
>>>>> flush isn't really the right semantics for this particular
>>>>> operation, so the AVOption approach might be more right.
>>>>
>>>> This one should work better with the segmenter.
>>>
>>> how many formats do we want to support with the segmenter? I can see
>>> that a custom AVOption per format is really inconvenient but using
>>> flush semantics for outputting PAT+PMT feels wrong.
>>>
>>> I would prefer a shared AVOption "start_new_segment" since I don't
>>> think this warrants its own function pointer in AVOutputFormat.
>>
>> That would be ok with me, too.
>>
>> The problem with a generic segmenter is that even though the
>> concepts are similar on a high level, there's lots and lots of
>> differences in behaviour, so while this is a good and less
>> mpegts-specific solution, I wouldn't want it for the ismv muxer.
>>
>> In the ismv muxer, a start_new_segment flag wouldn't work really
>> right, (while the flush operation we have there right now fits
>> perfectly), since we wouldn't want to write any new data, just flush
>> the existing one. For mpegts on the other hand, the muxer always
>> already flushed all data, we just need to flag to tell it to resend
>> "headers".
>>
>> But yes, I'm ok with a flag like "start_new_segment" or
>> "resend_headers" which would work for mpegts and would work for any
>> other format with similar behaviour, too.
>
> I think it is preferable to seperate things needed to be done at the
> end of a segment from things needed at the start of the segment.

Indeed. So which option name do you prefer, resend_headers or 
start_new_segment?

// Martin

Patch

diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index 8232cbc..cf1bb3b 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -928,7 +928,7 @@  static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
     avio_flush(s->pb);
 }
 
-static int mpegts_write_packet(AVFormatContext *s, AVPacket *pkt)
+static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
 {
     AVStream *st = s->streams[pkt->stream_index];
     int size = pkt->size;
@@ -1046,6 +1046,17 @@  static int mpegts_write_packet(AVFormatContext *s, AVPacket *pkt)
     return 0;
 }
 
+static int mpegts_write_packet(AVFormatContext *s, AVPacket *pkt)
+{
+    MpegTSWrite *ts = s->priv_data;
+    if (!pkt) {
+        ts->pat_packet_count = ts->pat_packet_period - 1;
+        ts->sdt_packet_count = ts->sdt_packet_period - 1;
+        return 1;
+    }
+    return mpegts_write_packet_internal(s, pkt);
+}
+
 static int mpegts_write_end(AVFormatContext *s)
 {
     MpegTSWrite *ts = s->priv_data;
@@ -1091,4 +1102,5 @@  AVOutputFormat ff_mpegts_muxer = {
     .write_packet      = mpegts_write_packet,
     .write_trailer     = mpegts_write_end,
     .priv_class = &mpegts_muxer_class,
+    .flags             = AVFMT_ALLOW_FLUSH,
 };