flacenc sends byte segments upon receiving segment events.
For comparison, vorbisenc does not.
Removing the sending of these byte segments does not seem to have any adverse effect,,, (famous last words).

flacenc (and any other encoder) should use TIME segments anyway

(In reply to comment #2)
> flacenc (and any other encoder) should use TIME segments anyway

In general, yes, but I guess it is doing the BYTE stuff to perform seeking for (header) finalization when writing a "standalone flac" file.

AFAIK, best approach is to return FALSE to these segments if not interested in BYTE (or have a whole separate muxer doing the standalone flac stuff instead of flacenc).

What I don't get is that the incoming segment start/stop are ignored, and 0/-1 are sent. It doesn't make much sense to me.

As for the segment from the seeking callback, it is indeed used for the header, there are three seeks on EOS near the start. These must be in bytes, which causes asserts if previous segments were in time.

I'll go on rummaging in here to see if I can find a good way to make everything happy.

I think one could make quite a hack there:

We assume seeking will be used only for getting back near the start.
We keep a copy of the first kilobyte or so of what we write first.
When asked to seek in bytes at offset OFFS near the start, we ask to seek in time to 0 (which should seek in bytes to 0), then write OFFS bytes from the data we remembered at start.

That should ensure that we only output time segments, and that we can still mimic a seek in bytes to near the start. We only need to remember enough bytes to always rewrite from 0 (I see three seeks here, all to under 30 bytes offset, which should be constrained by the header format).

It's hacky but should work. Right ? :)

What's actually a new problem here ?

AFAIK, this situation has always been around and never been a problem.
In particular, flacenc ! oggmux worked and so did flacenc ! filesink.

If something has changed due to GstCollectPads2 then it would suffice to tweak it to handle this "properly" (as before).

W.r.t. to approach above; not sure whether seeking in TIME to 0 will have filesink seeking to 0 in bytes.  Also, need to avoid having this rewritten header data ending up in a muxed format.

So, another hack might be; send a 0 BYTE based segment at start (i.e. early on);
if that is handled (returns TRUE) then assume a filesink situation, otherwise assume muxer based situation (and then maybe also pass along more TIME newsegment stuff than is done currently).

I don't know if it's new or not, I'd never tried to mux flac in Matroska before the port to GetCollectPads2. Both muxing to Ogg and raw work curently.
It is indeed peculiar that muxing in Ogg would not throw these errors, as it also uses GstCollectPads2. I'll look into why.

Ah, oggmux sets the sink pads event function to its own, and will not forward non TIME segments to collectpads2, whereas matroskamux will let collectpads2 handle events first, which will hit this.

This hints that the error is in fact not really an error, and that what should be an error is to receive a buffer without having received a time segment first, after non time segments were ignored.

However, oggmux does not receive a time segment either, only byte segments, which it ignores. I find this odd, but I'm not knowledgeable enough to know if it's wrong or not.

I guess a salient point is that, despite those errors, the muxed output seems correct... It plays via gst, vlc, and ffplay.

While it's not ideal/nice to have mixed TIME/BYTE segments in general, it's kind of ok (IMO) in this hackish case to ignore the BYTE ones (which in this case informs upstream not to pester downstream with re-writes).

It is a bit more problematic not having received TIME segments (before data).
Probably oggmux assumes stuff starts from 0 (if missing TIME), or falls back to the granule stuff.

Anyway, therefore as previously mentioned we might also have flacenc pass along TIME segments either unconditionally, if filesink or so won't choke on those, or conditionally as commented earlier.

This mess probably also stems from the fact that (iirc) flac frames have their own (embedded) timestamps (only 0-based ?) which compete with context/container based ones.  Hence the current hacks hoping that all will be nicely 0-based and work out well.

... or of course simply toning down GstCollectPads2 complaints or so and leaving it at that might then also work ;)

Some random thoughts:

 - this has come up in some form or another a few
    times (as part of muxers having ported to
    collectpads2), e.g. people doing appsrc ! muxer
    or appsrc ! encoder ! muxer and not telling
    appsrc to send a TIME newsegment. So those
    use cases need to keep working really.

 - the GST_ERROR should probably be downgraded
    to a GST_WARNING

 - cp2/muxers should ignore BYTE newsegments
   (and do a GST_WARNING or post a warning
    message on the bus maybe), and assume some
    generic 0-based TIME segment if they don't
    get another TIME segment IMHO

 - as for flacenc and similar elements (faac?),
   two ideas:

     (a) do a get_allowed_caps() downstream,
     and if it's ANY it's *probably* filesink or so.

     (b) make basesink/filesink etc. implement
     the SEEKING query, so encoders/muxers
     that need to seek back to fix up headers
     can query if downstream can seek back.

Created attachment 206694 [details] [review]
collectpads2: assume 0 based segment if no time segment was provided

This seems to work, though I'm not 100% sure of the semantics of the segment flag in collectpads2.
So this should take care of the collectpads2 side.

In fact, I think that's all that's needed, since muxing into ogg and matroska seems fine already, so the seek+rewrite code can be done unconditionally in case support is there downstream.
There is already code in flacenc that will forward an error to libFLAC in case downstreams returns an error in seeking.

same error with adpcmenc and qtmux, the patch avoid the error for me too, however the pipeline works with and without the patch

please note that I'm doing ... ! adpcmenc ! appsink and then appsrc ! qtmux this error doesn't happen (without the patch) if I set format 3 on appsrc (as seuggested by Tim in a previous comment)

In summary, following commit should then settle this (i.e. make things less alarming as they already work at present).  IIRC some more SEEKING query stuff has already been sprinkled and some more will be sprinkled in 1.0.

commit b642b875798e93f302b4b452b89f7ddb0e02db6b
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Fri Feb 3 17:08:35 2012 +0000

    collectpads2: assume 0 based segment if no time segment was provided
    
    https://bugzilla.gnome.org/show_bug.cgi?id=669305