Skip to content
Snippets Groups Projects

winegstreamer: Use provided PTS and duration in video_decoder.

Open Brendan McGrath requested to merge redmcg/wine:bm_cw_24708_mft into master
5 unresolved threads

This MR modifies winegstreamer to match Windows behaviour in that:

  1. the video decoders will output the PTS and duration of the input sample (if provided); and
  2. the WMV decoder will set any value not provided to zero

It also adds support for supplying a DTS value to the MFTs.

I've marked this as draft as it fixes the tests in MR !7563 (merged) (in addition to fixing some existing test_wmv_decoder tests). Also, as demonstrated in MR !7569 (merged), our demuxers output different timestamps to Windows. This change will result in those different timestamps being forwarded from the decoder. So we may also want to address that difference prior to accepting this MR.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • **** requested review from @nsivov

    requested review from @nsivov

  • Brendan McGrath added 41 commits

    added 41 commits

    Compare with previous version

  • Brendan McGrath marked this merge request as ready

    marked this merge request as ready

  • Author Developer

    MR !7563 (merged) has been merged, so I've marked this MR as "ready".

    Edited by Brendan McGrath
  • Brendan McGrath added 251 commits

    added 251 commits

    Compare with previous version

  • Brendan McGrath added 2 commits

    added 2 commits

    • ef99a073 - mf/tests: Add negative timestamp tests for h264.
    • 8de11a12 - winegstreamer: Use provided PTS and duration in video_decoder.

    Compare with previous version

    • Author Developer

      When testing these changes with FFmpeg as the demuxer, I found FFmpeg can produce negative PTS and DTS. And then on testing Windows, I discovered that Windows supports negative PTS and DTS, however, gstreamer does not.

      So I've made some adjustments to this MR:

      1. To add tests for negative PTS and DTS; and
      2. To convert PTS and DTS to be zero or above on input to gstreamer and then convert back the output so that our MFTs can support negative values
    • Are negative timestamps encoded in media data directly?

      P.S. we'll need someone to review winegstreamer parts.

    • Author Developer

      This is in the MFT, which can be used by the application directly. In theory, they can assign any value they want (which is how I test the negative values on Windows).

      But in terms of media data, I'll use the MPEG4 container as an example (as that's the one where I saw FFmpeg produce negative values). You get given three values/tables:

      1. Media Time;
      2. Time to Sample; and
      3. Composition Offset

      The composition offset is only used when you have a codec that needs decoding in a different order than presentation. For example, H.264 which uses B-Frames.

      And it seems Windows, GStreamer and FFmpeg all use these values differently and produce different results.

      As an example, say we have:

      1. Media Time = 0.666666s
      2. Time to Sample 1 = 0s
      3. Composition Offset for Sample 1 = 0.333333s

      DTS is calculated as PTS minus composition offset.

      Windows asserts, due to a Media Time of 0.666666s, then that will be the first PTS:

      PTS = 0.666666s, DTS = 0.333333

      GStreamer asserts that the first DTS will be 0s:

      PTS = 0.333333s, DTS = 0s

      FFmeg assets that the first PTS will be 0s:

      PTS = 0, DTS = -0.333333s

      In addition, I've seen FFmpeg produce a negative PTS on audio, but I'll have to double check the code to see how it got there. But I did note a couple of possibilities:

      1. The value was the negative value of Media Time;
      2. The value was the negative value of the duration, and the packet was flagged with AV_PKT_FLAG_DISCARD
    • Please register or sign in to reply
  • Brendan McGrath added 194 commits

    added 194 commits

    • 8de11a12...78fe1014 - 192 commits from branch wine:master
    • dc3d601a - mf/tests: Add negative timestamp tests for h264.
    • 4618f0a6 - winegstreamer: Use provided PTS and duration in video_decoder.

    Compare with previous version

  • Brendan McGrath requested review from @rbernon

    requested review from @rbernon

  • Rémi Bernon
    Rémi Bernon @rbernon started a thread on commit 4618f0a6
22 22 DEFINE_GUID(CLSID_decodebin_parser, 0xf9d8d64e, 0xa144, 0x47dc, 0x8e, 0xe0, 0xf5, 0x34, 0x98, 0x37, 0x2c, 0x29);
23 23 DEFINE_GUID(CLSID_mpeg_layer3_decoder, 0x38be3000, 0xdbf4, 0x11d0, 0x86, 0x0e, 0x00, 0xa0, 0x24, 0xcf, 0xef, 0x6d);
24 24 DEFINE_GUID(WINESUBTYPE_Gstreamer, 0xffffffff, 0x128f, 0x4dd1, 0xad, 0x22, 0xbe, 0xcf, 0xa6, 0x6c, 0xe7, 0xaa);
25 DEFINE_GUID(MFSampleExtension_WG_Timestamp, 0x42554ce2, 0xed8c, 0x43ee, 0x8a, 0x04, 0xd6, 0xa1, 0x68, 0xf1, 0x8f, 0xee);
  • Do we really need this custom GUID? Can't the fixups be confined on the unix side only?

  • Author Developer

    Because we can receive samples in a different order to what was sent (for example, on B-Frames), I was thinking only GStreamer can preserve state from input to output. But please correct me if I'm wrong.

  • Author Developer

    I'll just add, that the state I'm referring to is the sample PTS. On Windows, the application can provide any arbitrary value and when the decoder produces the output, it needs to preserve the provided value.

  • Author Developer

    Oops - sorry. I got the GUID mixed up with timestamp/x-wg-transform.

    The GUID might not be necessary actually. I might be able to use the preserve_timestamps boolean instead. I think the only difference would be that if GStreamer lost the timestamp/x-wg-transform state, we would fall back to using the gstreamer values. Which is possibly better.

  • Author Developer

    Oops (again). I just tried using the preserve_timestamps boolean and it doesn't work. The timestamp/x-wg-transform is a flag that lets video_decoder.c know that an input timestamp has been provided (and that it should use it). It will otherwise fall back to the behavior defined by provide_timestamps.

    But if we want to get rid of the need for the GUID, I guess we could move the existing logic in video_decoder.c (for generating our own timestamp) in to wg_transform.c.

  • Hmm, I'm not sure that moving timestamp generation to the unix side is better. Can't we simply fill the sample timestamps from the input provided timestamps instead of the GStreamer-generated timestamps we don't really care about?

    Edited by Rémi Bernon
  • Author Developer

    I did think of another way to do this; I could simply return another value from wg_transform_read_mf (in addition to the sample) to indicate that a timestamp has been provided. We could potentially even use the flags variable that is already being passed. Let me know if you'd prefer one of those options.

  • Please register or sign in to reply
  • Rémi Bernon
    Rémi Bernon @rbernon started a thread on commit 4618f0a6
  • 179 179 WG_SAMPLE_FLAG_HAS_DURATION = 4,
    180 180 WG_SAMPLE_FLAG_SYNC_POINT = 8,
    181 181 WG_SAMPLE_FLAG_DISCONTINUITY = 0x10,
    182 WG_SAMPLE_FLAG_HAS_DTS = 0x20,
    183 WG_SAMPLE_FLAG_WG_TIMESTAMP = 0x40,
    182 184 };
    183 185
    184 186 struct wg_sample
    185 187 {
    186 188 /* timestamp and duration are in 100-nanosecond units. */
    187 UINT64 pts;
    188 UINT64 duration;
    189 INT64 pts;
    190 INT64 duration;
    191 INT64 dts;
    • How useful are DTS? Do we really need to pass them through?

    • Author Developer

      Good question. Unfortunately I must admit to not being an expert on that. Obviously it seems to decode fine without it, so it may be more useful for muxing than decoding. Although, the only codec with B-Frames that I've tested is H.264; maybe there's another codec that's more dependent on it. But I'll experiment and see if I can discern any difference between its inclusion vs. exclusion.

    • Author Developer

      Testing H.264 decoding, I've been unable to discern any difference. I only tested with a small sample of videos, but it seems to work both with and without and with no notable difference in performance.

    • Author Developer

      So should we keep DTS here? I guess my only motivation for including it was because the industry went to the effort of providing it.

    • Please register or sign in to reply
  • Rémi Bernon
    Rémi Bernon @rbernon started a thread on an outdated change in commit 4618f0a6
  • 863 866 buffer_add_video_meta(buffer, &video_info);
    864 867 }
    865 868
    869 /* we perform DTS first as it should never be later than PTS, so any required timestamp offset should be the maximum here */
    870 if (sample->flags & WG_SAMPLE_FLAG_HAS_DTS)
    871 {
    872 dts = sample->dts += transform->ts_offset;
    873 if (dts < 0)
    874 {
    875 transform->ts_offset -= dts;
  • Rémi Bernon left review comments

    left review comments

  • Rémi Bernon
    Rémi Bernon @rbernon started a thread on commit 4618f0a6
  • 867 GST_BUFFER_PTS(buffer) = sample->pts * 100;
    881 {
    882 pts = sample->pts += transform->ts_offset;
    883 if (pts < 0)
    884 {
    885 transform->ts_offset -= pts;
    886 pts = 0;
    887 }
    888 GST_BUFFER_PTS(buffer) = pts * 100;
    889
    890 if (transform->attrs.preserve_timestamps)
    891 {
    892 transform_timestamp = gst_caps_new_empty_simple("timestamp/x-wg-transform");
    893 gst_buffer_add_reference_timestamp_meta(buffer, transform_timestamp, pts * 100,
    894 sample->flags & WG_SAMPLE_FLAG_HAS_DURATION ? sample->duration * 100 : GST_CLOCK_TIME_NONE);
    895 gst_caps_unref(transform_timestamp);
    • Comment on lines +892 to +895

      Have you tried GstVideoTimeCodeMeta / do you know what it does?

    • Author Developer

      No, I didn't look at that. But taking a look now, it looks like it might be good for Video PTS/DTS, but wouldn't work for audio. I'm not sure for duration either.

    • Author Developer

      I think gst_buffer_add_reference_timestamp_meta is a better fit for this use case. So I plan to leave this as is. Let me know if you agree.

    • Please register or sign in to reply
  • Rémi Bernon left review comments

    left review comments

  • Brendan McGrath added 81 commits

    added 81 commits

    • 4618f0a6...0e7654e5 - 79 commits from branch wine:master
    • 2cf590fd - mf/tests: Add negative timestamp tests for h264.
    • ed9069fe - winegstreamer: Use provided PTS and duration in video_decoder.

    Compare with previous version

  • Brendan McGrath added 1 commit

    added 1 commit

    • 2be9abb9 - winegstreamer: Use provided PTS and duration in video_decoder.

    Compare with previous version

  • Brendan McGrath added 1 commit

    added 1 commit

    • b544981c - winegstreamer: Use provided PTS and duration in video_decoder.

    Compare with previous version

  • Please register or sign in to reply
    Loading