Summary: | [GStreamer] gst_buffer_unmap: assertion 'GST_IS_BUFFER (buffer)' failed | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Diego Pino <dpino> | ||||||||||
Component: | Media | Assignee: | Xabier Rodríguez Calvar <calvaris> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gustavo, hta, jer.noble, menard, philipj, pnormand, sergio, tommyw, vjaquez, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Diego Pino
2020-06-18 20:42:15 PDT
Created attachment 404443 [details]
Patch
Comment on attachment 404443 [details]
Patch
This looks like a workaround for a bug in the InitData class. As the init data is quite small (IIRC), doesn't it pay off to have mapped buffer there? Can't we just copy it? Also I see the append() method assuming the m_payload is mutable, while the createSharedBuffer() has a comment mentioning it should be considered immutable.
(In reply to Philippe Normand from comment #2) > Comment on attachment 404443 [details] > Patch > > This looks like a workaround for a bug in the InitData class. As the init > data is quite small (IIRC), doesn't it pay off to have mapped buffer there? > Can't we just copy it? We could buy I don't think it is necessary because of I am going to comment below. > Also I see the append() method assuming the m_payload > is mutable, while the createSharedBuffer() has a comment mentioning it > should be considered immutable. The data inside the SharedBuffer should be immutable, not the SharedBuffer itself. That's why we assert on the mapping to be not WRITE. Actually, appending SharedBuffers as we do is quite efficient as no data is copied, only segments are appended or until it is read if needed. About keeping the mapped buffer? It is already done, it leaves inside the DataSegment of the SharedBuffer. GstMappedBuffer needs that he buffer outlives the mapped buffer of you will have the warning you're getting (which is a symptom of a deeper problem and can lead to unwanted reads). In many cases we just create the mapped buffer, use it and let it go. In this case, it is kept and as we're seeing in this case the buffer is not outliving the mapped buffer, which creates the problem. Considering all this, I think reffing the buffer and guaranteeing it lives protected inside the mapped buffer is the right solution. (In reply to Xabier Rodríguez Calvar from comment #3) > (In reply to Philippe Normand from comment #2) > > Comment on attachment 404443 [details] > > Patch > > > > This looks like a workaround for a bug in the InitData class. As the init > > data is quite small (IIRC), doesn't it pay off to have mapped buffer there? > > Can't we just copy it? > > We could buy I don't think it is necessary because of I am going to comment > below. > > > Also I see the append() method assuming the m_payload > > is mutable, while the createSharedBuffer() has a comment mentioning it > > should be considered immutable. > > The data inside the SharedBuffer should be immutable, not the SharedBuffer > itself. That's why we assert on the mapping to be not WRITE. Actually, > appending SharedBuffers as we do is quite efficient as no data is copied, > only segments are appended or until it is read if needed. > > About keeping the mapped buffer? It is already done, it leaves inside the > DataSegment of the SharedBuffer. I meant the GstMappedBuffer which goes out of scope in the InitData constructor. (In reply to Philippe Normand from comment #4) > (In reply to Xabier Rodríguez Calvar from comment #3) > > (In reply to Philippe Normand from comment #2) > > > Comment on attachment 404443 [details] > > > Patch > > > > > > This looks like a workaround for a bug in the InitData class. As the init > > > data is quite small (IIRC), doesn't it pay off to have mapped buffer there? > > > Can't we just copy it? > > > > We could buy I don't think it is necessary because of I am going to comment > > below. > > > > > Also I see the append() method assuming the m_payload > > > is mutable, while the createSharedBuffer() has a comment mentioning it > > > should be considered immutable. > > > > The data inside the SharedBuffer should be immutable, not the SharedBuffer > > itself. That's why we assert on the mapping to be not WRITE. Actually, > > appending SharedBuffers as we do is quite efficient as no data is copied, > > only segments are appended or until it is read if needed. > > > > About keeping the mapped buffer? It is already done, it leaves inside the > > DataSegment of the SharedBuffer. > > I meant the GstMappedBuffer which goes out of scope in the InitData > constructor. Yes, that one. It apparently goes out of scope, but if you follow the SharedBuffer onion, you'll see a & goes down to the DataSegment, it is implicitly becomes a Ref&& and moved into the segment. Yeah right, so this is happening because the SharedBuffer doesn't keep a reference of the buffer. It would be nicer to have a wrapper shared buffer mechanism perhaps? We could then increment the buffer ref when creating the shared buffer and then have a destroy-notify callback to unref it. Created attachment 404934 [details]
Patch
Comment on attachment 404934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404934&action=review > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:121 > + uint8_t* data() { ASSERT(m_isValid); return static_cast<uint8_t*>(m_info.data); } Should this one be moved to GstMappedOwnedBuffer? Created attachment 405035 [details] Patch (In reply to Philippe Normand from comment #8) > Comment on attachment 404934 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=404934&action=review > > > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:121 > > + uint8_t* data() { ASSERT(m_isValid); return static_cast<uint8_t*>(m_info.data); } > > Should this one be moved to GstMappedOwnedBuffer? No, owned is only for readonly, if something is for writing is the superclass. I re-uploaded the patch with some more checks for memory safety. Committed r264762: <https://trac.webkit.org/changeset/264762> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405035 [details]. Problem persists. Created attachment 405134 [details]
Patch
Committed r264816: <https://trac.webkit.org/changeset/264816> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405134 [details]. |