Summary: | [GStreamer] <img> tag needs to support video formats | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
Component: | WebKitGTK | Assignee: | Philippe Normand <pnormand> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | annulen, bugs-noreply, calvaris, cgarcia, changseok, eric.carlson, ews-watchlist, glenn, gustavo, gyuyoung.kim, jer.noble, magomez, menard, philipj, pnormand, ryuan.choi, sergio, vjaquez | ||||||||
Priority: | P2 | ||||||||||
Version: | Other | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=224107 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 211995 | ||||||||||
Attachments: |
|
Description
Michael Catanzaro
2017-12-04 12:48:44 PST
Adding fast/images/animated-image-mp4-crash.html here as well, which is timing out. I... kinda... started having a look... :D Created attachment 399588 [details]
Patch
Created attachment 399594 [details]
Patch
Comment on attachment 399594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399594&action=review > Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp:65 > + m_image = ImageGStreamer::createImage(gst_sample_ref(platformSample().sample.gstSample)); From what I am seeing, I think you're leaking the sample here. It is true that the ImageGStreamer::createImage adopts, but it adopts the ImageGStreamer object, not the sample (at least in the Cairo Impl). This said, I think this API is confusing and it could be well using either a const GstSample* or maybe a GRefPtr<GstSample>&&. In this case the pointer would be destroyed after the call because nobody will use it. > Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp:351 > +void ImageDecoderGStreamer::pushEncodedData(SharedBuffer& buffer) Maybe you can switch to const SharedBuffer& all the way in the onion > Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.h:102 > + ImageDecoderGStreamer* m_decoder; Would it make sense to make this refcounted or weak? Created attachment 399822 [details]
Patch
Comment on attachment 399594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399594&action=review >> Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp:65 >> + m_image = ImageGStreamer::createImage(gst_sample_ref(platformSample().sample.gstSample)); > > From what I am seeing, I think you're leaking the sample here. It is true that the ImageGStreamer::createImage adopts, but it adopts the ImageGStreamer object, not the sample (at least in the Cairo Impl). > > This said, I think this API is confusing and it could be well using either a const GstSample* or maybe a GRefPtr<GstSample>&&. In this case the pointer would be destroyed after the call because nobody will use it. Can't be const GstSample* because the gst_sample_* APIs expect a non-const pointer. I don't really see the point of using a GRefPtr either because the sample is already owned by the MediaSampleGStreamer :) Comment on attachment 399822 [details]
Patch
I still see lots of SharedBuffer& not turned into const, please do that.
Those are part of the ImageDecoder API. Why should I change all the implementations for the sake of another one? (In reply to Philippe Normand from comment #9) > Those are part of the ImageDecoder API. Why should I change all the > implementations for the sake of another one? ok, you might go ahead. Committed r261922: <https://trac.webkit.org/changeset/261922> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399822 [details]. Comment on attachment 399594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399594&action=review >> Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.h:102 >> + ImageDecoderGStreamer* m_decoder; > > Would it make sense to make this refcounted or weak? Making it weak wasn't a good idea: https://bugs.webkit.org/show_bug.cgi?id=213178 |