After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 702330 - mpegvideoparse: Only map input buffer once
mpegvideoparse: Only map input buffer once
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal enhancement
: 1.1.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-06-15 12:32 UTC by Edward Hervey
Modified: 2013-07-22 11:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mpegvideoparse: Only map input buffer once (8.66 KB, patch)
2013-06-15 12:32 UTC, Edward Hervey
needs-work Details | Review
mpegvideoparse: Only map input buffer once (8.00 KB, patch)
2013-07-22 08:47 UTC, Edward Hervey
accepted-commit_now Details | Review

Description Edward Hervey 2013-06-15 12:32:13 UTC
Instead of constantly map/unmapping it a bit everywhere, we pass along
to all functions the GstMapInfo.

Makes mpeg video frame parsing 6% faster
Comment 1 Edward Hervey 2013-06-15 12:32:16 UTC
Created attachment 246885 [details] [review]
mpegvideoparse: Only map input buffer once
Comment 2 Tim-Philipp Müller 2013-06-17 10:47:23 UTC
Comment on attachment 246885 [details] [review]
mpegvideoparse: Only map input buffer once

Hah, nice.

> static gboolean
>-gst_mpegv_parse_process_config (GstMpegvParse * mpvparse, GstBuffer * buf,
>+gst_mpegv_parse_process_config (GstMpegvParse * mpvparse, GstMapInfo * info,
>     guint size)
> {

Minor nitpick: size is now also in info.size, isn't it (if you always pass gst_buffer_get_size() anyway), so doesn't have to be passed separately if info is passed?

Also, I wonder if it wouldn't be even better to pass info.data instead of the whole GstMapInfo *? Then the compiler doesn't have to re-read info>data everytime it's accessed.
Comment 3 Sebastian Dröge (slomo) 2013-06-18 11:51:42 UTC
Just passing the data seems better to me too, otherwise looks good
Comment 4 Edward Hervey 2013-06-19 05:47:32 UTC
I tried to keep the changes to a minimum in this patch, which is why I only replaced GstBuffer * with GstMapInfo *.

There are multiple places where there are local "data" and "size" variables with different meanings/usage than the input buffer, so prefered to keep amount of changes to a minimum to avoid issues and potential bugs.

OK to push as-is and leave further cleanups/optimisations to later fixes ?
Comment 5 Sebastian Dröge (slomo) 2013-06-19 08:16:29 UTC
Ok
Comment 6 Tim-Philipp Müller 2013-06-19 08:21:38 UTC
Sure, but you don't have to call the variable with the data 'data' ;)
Comment 7 Edward Hervey 2013-07-22 08:08:34 UTC
Needs complete change after GstMpegVideoPacket commit :(
Comment 8 Edward Hervey 2013-07-22 08:47:29 UTC
Created attachment 249768 [details] [review]
mpegvideoparse: Only map input buffer once

Instead of constantly map/unmapping it a bit everywhere, we pass along
to all functions the GstMapInfo.

Makes mpeg video frame parsing 6% faster
Comment 9 Edward Hervey 2013-07-22 11:47:53 UTC
commit 1db3d40a4bdaa3f77005ef99f7edda215919ef5e
Author: Edward Hervey <edward@collabora.com>
Date:   Mon Jul 22 10:46:23 2013 +0200

    mpegvideoparse: Only map input buffer once
    
    Instead of constantly map/unmapping it a bit everywhere, we pass along
    to all functions the GstMapInfo.
    
    Makes mpeg video frame parsing 6% faster
    
    https://bugzilla.gnome.org/show_bug.cgi?id=702330