GNOME Bugzilla – Bug 796544
qtdemux: Add comment about qtdemux->segment
Last modified: 2018-11-03 15:30:55 UTC
qtdemux->segment is far from a trivial field. A little documentation about how it's used in qtdemux won't hurt.
Created attachment 372606 [details] [review] qtdemux: Add comment about qtdemux->segment
Review of attachment 372606 [details] [review]: ::: gst/isomp4/qtdemux.h @@ -119,1 +119,8 @@ - /* configured playback region */ + /* Configured playback region. + * + * segment.format is always GST_FORMAT_TIME. ... 5 more ... Sounds like somethings that might (likely?) get obsoleted. @@ +153,3 @@ + * + * qtdemux->segment is modified in response to seeks and when a TIME segment + * event is sent from upstream. In the latter case, the upstream segment is s/is//
Review of attachment 372606 [details] [review]: ::: gst/isomp4/qtdemux.h @@ -119,1 +119,8 @@ - /* configured playback region */ + /* Configured playback region. + * + * segment.time is never read. ... 5 more ... What do you mean?
Created attachment 372607 [details] [review] qtdemux: Add comment about qtdemux->segment qtdemux->segment is far from a trivial field. A little documentation about how it's used in qtdemux won't hurt.
In the context of fixing the latest regressions I've been trying to make sense of qtdemux->segment usages. Please read the comment in the attached patch and confirm it matches also your assumptions, or challenge those you think are wrong, so as to make sure we are in the same page.
Review of attachment 372607 [details] [review]: Looks like a pretty good comment to me. ::: gst/isomp4/qtdemux.h @@ +119,3 @@ + /* Configured playback region. + * + * segment.format is always GST_FORMAT_TIME. I know Thibault said otherwise, but reading basesrc code, FORMAT_BYTES seems to be used there. And FORMAT_PERCENT is implemented also in there, so I have my doubt about deprecation. For the demuxer output format, I think always TIME is perfect. @@ +125,3 @@ + * + * segment.time is never read. It's not useful in this context, since the + * mapping from buffer time to stream time changes from track to track. It's never read, though will it be of the correct value if you want to translate .start/stop to streaming time ? Or is that always == start ? @@ +131,3 @@ + * + * segment.duration contains a translation of qtdemux->duration to + * nanoseconds. Both fields should be updated at the same time. Good to document, seems error prone to have two copies of the same value. @@ +139,3 @@ + * time (FIXME). This field is used to reply position queries. It's also used + * (with a 2 extra second allowance) to signal EOS at the end of a track in + * gst_qtdemux_sync_streams(). What does the FIXME means ? A bug ? @@ +146,3 @@ + * segments. (FIXME: In consequence, edits with rate != 1.0 will have wrong + * stream time... although probably nobody uses such an obscure feature with + * so little player support anyway). I'd say "edits with rate different then 1.0 are not currently supported". We could have a todo list comment at the top, as we do in some other sources.
(In reply to Nicolas Dufresne (ndufresne) from comment #6) > Review of attachment 372607 [details] [review] [review]: > > Looks like a pretty good comment to me. > > ::: gst/isomp4/qtdemux.h > @@ +119,3 @@ > + /* Configured playback region. > + * > + * segment.format is always GST_FORMAT_TIME. > > I know Thibault said otherwise, but reading basesrc code, FORMAT_BYTES seems > to be used there. And FORMAT_PERCENT is implemented also in there, so I have > my doubt about deprecation. For the demuxer output format, I think always > TIME is perfect. Where did I say otherwise? The format should always be TIME afaict. (In reply to Alicia Boya García from comment #3) > Review of attachment 372606 [details] [review] [review]: > > ::: gst/isomp4/qtdemux.h > @@ -119,1 +119,8 @@ > - /* configured playback region */ > + /* Configured playback region. > + * > + * segment.time is never read. > ... 5 more ... > > What do you mean? Stating that we do not read it in that comment doesn't bring much fmpov, that is basically what I meant. I remember it is set in the dash case, not sure we care at all but I would prefer to avoid to add comments that can outdate.
(In reply to Thibault Saunier from comment #7) > > (In reply to Nicolas Dufresne (ndufresne) from comment #6) > > Review of attachment 372607 [details] [review] [review] [review]: > > > > Looks like a pretty good comment to me. > > > > ::: gst/isomp4/qtdemux.h > > @@ +119,3 @@ > > + /* Configured playback region. > > + * > > + * segment.format is always GST_FORMAT_TIME. > > > > I know Thibault said otherwise, but reading basesrc code, FORMAT_BYTES seems > > to be used there. And FORMAT_PERCENT is implemented also in there, so I have > > my doubt about deprecation. For the demuxer output format, I think always > > TIME is perfect. > > Where did I say otherwise? The format should always be TIME afaict. In reference to comment #2, "Sounds like somethings that might (likely?) get obsoleted." I don't think it's a candidate for that no.
(In reply to Nicolas Dufresne (ndufresne) from comment #8) > (In reply to Thibault Saunier from comment #7) > > Where did I say otherwise? The format should always be TIME afaict. > > In reference to comment #2, "Sounds like somethings that might (likely?) get > obsoleted." I don't think it's a candidate for that no. That comment was about "segment.time is never read." nothing more :-)
Review of attachment 372607 [details] [review]: ::: gst/isomp4/qtdemux.h @@ +119,3 @@ + /* Configured playback region. + * + * segment.format is always GST_FORMAT_TIME. Seeks with BYTES format are allowed, but they are transformed into a TIME seek by searching the offsets in the sample table by gst_qtdemux_convert_seek() before any change to qtdemux->segment is done. @@ +125,3 @@ + * + * segment.time is never read. It's not useful in this context, since the + * "segment.time is never read" is actually not entirely true... There is an exception: GST_QUERY_SEGMENT returns its start and stop values in streaming time, which implicitly reads the time field. I should change that. But for the rest of qtdemux, AFAIK, stream time in qtdemux->segment is currently devoid of meaning. `time = start` is set just in case and to make the former case work... @@ +139,3 @@ + * time (FIXME). This field is used to reply position queries. It's also used + * (with a 2 extra second allowance) to signal EOS at the end of a track in + * segment.base contains the running time when playback started. The first Yes. segment.position is sometimes unedited buffer time of the track (QTSAMPLE_PTS) and sometimes movie time... so you can never be sure what you are reading. @@ +146,3 @@ + * segments. (FIXME: In consequence, edits with rate != 1.0 will have wrong + * stream time... although probably nobody uses such an obscure feature with + * nanoseconds. Both fields should be updated at the same time. Edit lists with rate different than 1.0 are supported in the sense that there is code that handles them and that code is run... that code just happens to have a bug -- or so I think, I have not devoted time to make a testcase for this, I've just run the math for the stream time formula and saw a problem.
Created attachment 372608 [details] [review] qtdemux: Add comment about qtdemux->segment qtdemux->segment is far from a trivial field. A little documentation about how it's used in qtdemux won't hurt.
^Updated segment.time comment.
Review of attachment 372608 [details] [review]: ::: gst/isomp4/qtdemux.h @@ +128,3 @@ + * stream time in qtdemux->segment does not have a defined purpose. Currently + * the only code converting values in this segment to stream time is the code + * handling GST_QUERY_SEGMENT. Hm.... the values in a TIME seek are always expressed in "stream time". Are you 100% certain it's never used except for segment queries ? @@ +142,3 @@ + * time (FIXME). This field is used to reply position queries. It's also used + * (with a 2 extra second allowance) to signal EOS at the end of a track in + * gst_qtdemux_sync_streams(). Also very importantly, the segment.position can be updated by gst_segment_do_seek(). The last field (&update) will tell you whether it was modified or not. @@ +149,3 @@ + * segments. (FIXME: In consequence, edits with rate != 1.0 will have wrong + * stream time... although probably nobody uses such an obscure feature with + * so little player support anyway). *cough* you might want to remove that comment ? Also you have some cases where upstream seek handlers (such as DLNA sources, or anything) can push applied_rate != 1.0 (they will have applied the requested rate, but what you receive should be decoded normally). @@ +159,3 @@ + * event is sent from upstream. In the latter case, the upstream segment + * replaces all qtdemux->segment. That is the case when upstream is + * adaptivedemux. This is *generally* the case, but not always. You could have a DLNA-compatible element that can handle seeks, or an appsrc, or some element you don't know about.
As mentioned on IRC, the segment.offset handling in qtdemux is currently broken and I'm fixing that. As such it's also wrongly described in this commit (well, the comment describes what is done, not what should be done) :) After my changes, demux->segment will not be manually modified by qtdemux anymore but is only ever what falls out of gst_segment_do_seek(), and the stream specific segments are then mapped to that similar to now. Generally what you describe in the comment maps to the reality of the qtdemux element right now.
(In reply to Edward Hervey from comment #13) > Review of attachment 372608 [details] [review] [review]: > > ::: gst/isomp4/qtdemux.h > @@ +128,3 @@ > + * stream time in qtdemux->segment does not have a defined purpose. > Currently > + * the only code converting values in this segment to stream time is the > code > + * handling GST_QUERY_SEGMENT. > > Hm.... the values in a TIME seek are always expressed in "stream time". Are > you 100% certain it's never used except for segment queries ? Stream time is always relative to a given segment. In qtdemux we have a seek segment (qtdemux->segment) and a (per-track) stream segment (stream->segment). The stream time of the stream segment matches movie time, which matches indistintly buffer time and stream time of the seek segment. About my certainty about the usages of stream time of the seek segment: I grep'ped for usages of segment->time, filtered those that operate in the stream segment and found no reads. Then I looked for usages of *_stream_time* functions like gst_segment_to_stream_time() and only found the usages in GST_QUERY_SEGMENT handling. Hopefully I'm not missing anything. > @@ +142,3 @@ > + * time (FIXME). This field is used to reply position queries. It's also > used > + * (with a 2 extra second allowance) to signal EOS at the end of a track > in > + * gst_qtdemux_sync_streams(). > > Also very importantly, the segment.position can be updated by > gst_segment_do_seek(). The last field (&update) will tell you whether it was > modified or not. That's true, I'll update the comment. > Also you have some cases where upstream seek handlers (such as DLNA sources, > or anything) can push applied_rate != 1.0 (they will have applied the > requested rate, but what you receive should be decoded normally). Assuming you are referring to TIME segments comming from upstream, in that case *all fields* are overwritten. This is mentioned later in the comment. > > @@ +159,3 @@ > + * event is sent from upstream. In the latter case, the upstream segment > + * replaces all qtdemux->segment. That is the case when upstream is > + * adaptivedemux. > > This is *generally* the case, but not always. You could have a > DLNA-compatible element that can handle seeks, or an appsrc, or some element > you don't know about. Could you elaborate? I've not worked with DLNA elements so far in GStreamer, so I don't know how they work different than other sources.
I'm just pointing out that your comments are *only* regarding this *specific* element implementation, not generic rules/usages. Also please use the review tool when replying to reviews, it helps having context.
(In reply to Edward Hervey from comment #16) > I'm just pointing out that your comments are *only* regarding this > *specific* element implementation, not generic rules/usages. Of course, what made you think otherwise?
(In reply to Alicia Boya García from comment #17) > (In reply to Edward Hervey from comment #16) > > I'm just pointing out that your comments are *only* regarding this > > *specific* element implementation, not generic rules/usages. > > Of course, what made you think otherwise? You're assuming specific usages of qtdemux, that's all. Don't forget it's a black box, and you can't predict how an element will be used. People can create any pipeline they want :)
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/482.