Bug 190045 - [GStreamer][MSE] Use GObject for GST_TRACE_OBJECT
Summary: [GStreamer][MSE] Use GObject for GST_TRACE_OBJECT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alicia Boya García
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-27 10:51 PDT by Alicia Boya García
Modified: 2018-09-29 02:43 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.80 KB, patch)
2018-09-27 10:53 PDT, Alicia Boya García
no flags Details | Formatted Diff | Diff
Patch (4.54 KB, patch)
2018-09-28 04:01 PDT, Alicia Boya García
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alicia Boya García 2018-09-27 10:51:17 PDT
Passing a non-GObject object to GST_TRACE_OBJECT() can be
theoretically misunderstood by the GStreamer logging function, so this
patch avoids that.
Comment 1 Alicia Boya García 2018-09-27 10:53:00 PDT
Created attachment 350977 [details]
Patch
Comment 2 Thibault Saunier 2018-09-27 11:06:30 PDT
Comment on attachment 350977 [details]
Patch

Should you also give the pipeline a proper name?
Comment 3 Alicia Boya García 2018-09-27 11:25:30 PDT
(In reply to Thibault Saunier from comment #2)
> Comment on attachment 350977 [details]
> Patch
> 
> Should you also give the pipeline a proper name?

Any suggestion for a useful name?
Comment 4 Philippe Normand 2018-09-28 01:29:45 PDT
mse-append-pipeline-n where n is a monotically increasing number?
Comment 5 Alicia Boya García 2018-09-28 04:00:05 PDT
(In reply to Philippe Normand from comment #4)
> mse-append-pipeline-n where n is a monotically increasing number?

I've chosen to use "append-pipeline-<type>-<container>-<number>", e.g.: append-pipeline-audio-mp4-0; as it's usually useful to know which pipeline handles what thing.
Comment 6 Alicia Boya García 2018-09-28 04:01:16 PDT
Created attachment 351067 [details]
Patch
Comment 7 Thibault Saunier 2018-09-28 05:09:27 PDT
Comment on attachment 351067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351067&action=review

lgtm otherwise.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:144
> +        m_sourceBufferPrivate->type().containerType().replace("/", "-").utf8().data(), appendPipelineCount++);

Increment should probably be atomic.
Comment 8 Alicia Boya García 2018-09-28 06:25:16 PDT
Comment on attachment 351067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351067&action=review

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:144
>> +        m_sourceBufferPrivate->type().containerType().replace("/", "-").utf8().data(), appendPipelineCount++);
> 
> Increment should probably be atomic.

AppendPipeline can only be constructed in the main thread. There is even an assertion for that. No need for atomics here.
Comment 9 Thibault Saunier 2018-09-28 06:27:33 PDT
(In reply to Alicia Boya García from comment #8)
> Comment on attachment 351067 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351067&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:144
> >> +        m_sourceBufferPrivate->type().containerType().replace("/", "-").utf8().data(), appendPipelineCount++);
> > 
> > Increment should probably be atomic.
> 
> AppendPipeline can only be constructed in the main thread. There is even an
> assertion for that. No need for atomics here.

OK, informal r+ here then.
Comment 10 WebKit Commit Bot 2018-09-29 02:43:08 PDT
Comment on attachment 351067 [details]
Patch

Clearing flags on attachment: 351067

Committed r236640: <https://trac.webkit.org/changeset/236640>
Comment 11 WebKit Commit Bot 2018-09-29 02:43:10 PDT
All reviewed patches have been landed.  Closing bug.