Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pushing some changes from meta-rdk-ext layer #697

Closed

Conversation

emutavchi
Copy link

This is a subset of changes from meta-rdk-ext layer from RDK.

Make HTMLMediaElement::ignoreTrackDisplayUpdateRequests() constant time
Web Inspector: Crashes seen under
Inspector::ScriptCallFrame::~ScriptCallFrame
 https://bugs.webkit.org/show_bug.cgi?id=180373
 <rdar://problem/33894170>

* inspector/AsyncStackTrace.cpp:
(Inspector::AsyncStackTrace::truncate):
The `lastUnlockedAncestor->remove()` may release the only reference to it's
parent which we intend to use later but don't hold a RefPtr to. Keep the
parent alive explicitly by protecting it.
And restart streaming thread after configuring seamless segment
Replacing data during async decoding causes random crashes
@emutavchi emutavchi requested a review from woutermeek February 9, 2021 16:04
@woutermeek woutermeek requested review from magomez and eocanha February 9, 2021 16:10
Copy link
Member

@eocanha eocanha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(deleted comment)

Copy link
Member

@eocanha eocanha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing all these patches. There are many interesting fixes among them. All the multimedia related patches are fine except a few of them, where I have posted comments and questions to ask for clarification about some topics.
About the patches for hole punch or those not related to multimedia, somebody acquainted with those areas (@magomez?) should have a look at them.

@@ -301,6 +301,25 @@ void PlaybackPipeline::markEndOfStream(MediaSourcePrivate::EndOfStreamStatus)
gst_app_src_end_of_stream(appsrc);
}

GstPadProbeReturn segmentFixerProbe(GstPad*, GstPadProbeInfo* info, gpointer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The segmentFixerProbe was removed by e81ce7f
Whay kind of issues are you experiencing to need to bring it back? I'd like to discuss them with @ntrrgc

Copy link
Author

@emutavchi emutavchi Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sample replacement doesn't work after e81ce7f. I tried on different GStreamer version (1.10 to 1.16)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "sample replacement" I understand a quality change (to higher quality) where the new samples should replace the old ones in the already playing pipeline, right?
In theory, the reset of the running time plus using a zero base time (what @ntrrgc's patch does) should be equivalent of what the old patch did by keeping the running time and tweaking the base time. What kind of problems are you experiencing? Stalls, long waits, audio/video desynchronization?
As the Nexus multimedia system in Broadcom devices uses a common synchronization system between audio and video, maybe having different running times in the audio and the video streams confuses the Broadcom gst elements or the Nexus system. That might explain why you're having problems but everything looked fine on the Raspberry.

Copy link
Member

@eocanha eocanha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my side, all the topics except the segmentFixerProbe one have been successfully resolved. I'd like to have more information about the wrong behaviour that happens without the segmentFixerProbe patch.

@@ -301,6 +301,25 @@ void PlaybackPipeline::markEndOfStream(MediaSourcePrivate::EndOfStreamStatus)
gst_app_src_end_of_stream(appsrc);
}

GstPadProbeReturn segmentFixerProbe(GstPad*, GstPadProbeInfo* info, gpointer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "sample replacement" I understand a quality change (to higher quality) where the new samples should replace the old ones in the already playing pipeline, right?
In theory, the reset of the running time plus using a zero base time (what @ntrrgc's patch does) should be equivalent of what the old patch did by keeping the running time and tweaking the base time. What kind of problems are you experiencing? Stalls, long waits, audio/video desynchronization?
As the Nexus multimedia system in Broadcom devices uses a common synchronization system between audio and video, maybe having different running times in the audio and the video streams confuses the Broadcom gst elements or the Nexus system. That might explain why you're having problems but everything looked fine on the Raspberry.

Copy link

@magomez magomez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've given a look to the changes non related to MSE (that's eocanha's field) and they look ok to me. This can be merged as soon as eocanha gives the green light.

eocanha added a commit that referenced this pull request Feb 25, 2021
@eocanha
Copy link
Member

eocanha commented Feb 25, 2021

I've been testing all these commits with YouTube TV and other apps and found no regressions. I'm manually merging all the
commits except the ones affecting PlaybackPipeline::flush():

  • Backport [GStreamer][MSE] Actually implement flush() on playback pipeline
  • [MSE][GStreamer] Allow independed flushing of video buffer

It's true that they don't cause apparent regressions, but I'd like to understand better how they (or the lack of them) affect you. We can discuss that in a new pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants