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 590669 - [API] need GstByteWriter or GstChunkWriter
[API] need GstByteWriter or GstChunkWriter
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 0.10.26
Assigned To: Sebastian Dröge (slomo)
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-08-03 18:39 UTC by Tim-Philipp Müller
Modified: 2009-10-07 16:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (11.14 KB, patch)
2009-10-03 11:32 UTC, Sebastian Dröge (slomo)
none Details | Review
bitreader/bytereader: API: Add gst_(bit|byte)_reader_get_size() (4.45 KB, patch)
2009-10-03 14:35 UTC, Sebastian Dröge (slomo)
none Details | Review
bytewriter: Add a generic byte writer (14.72 KB, patch)
2009-10-03 14:35 UTC, Sebastian Dröge (slomo)
none Details | Review
bytereader,bitreader: Remove FIXME 0.11 to remove non-inlined functions (1.34 KB, patch)
2009-10-05 08:39 UTC, Sebastian Dröge (slomo)
committed Details | Review
bitreader/bytereader: API: Add gst_(bit|byte)_reader_get_size() (4.46 KB, patch)
2009-10-05 08:39 UTC, Sebastian Dröge (slomo)
committed Details | Review
bytewriter: Add a generic byte writer (15.19 KB, patch)
2009-10-05 08:40 UTC, Sebastian Dröge (slomo)
none Details | Review
bytewriter: Add a generic byte writer (15.21 KB, patch)
2009-10-05 08:46 UTC, Sebastian Dröge (slomo)
none Details | Review
bytewriter: Add a generic byte writer (15.39 KB, patch)
2009-10-05 09:23 UTC, Sebastian Dröge (slomo)
none Details | Review
bytewriter: Add unit test (10.13 KB, patch)
2009-10-05 09:24 UTC, Sebastian Dröge (slomo)
committed Details | Review
bytewriter: Add a generic byte writer (15.39 KB, patch)
2009-10-05 09:54 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Tim-Philipp Müller 2009-08-03 18:39:48 UTC
Would be nice to have a writing equivalent of GstByteReader (maybe also GstBitReader, but I'm not so bothered about that since I can't think of a place where I'd use it at the moment).
Comment 1 Sebastian Dröge (slomo) 2009-10-03 06:31:15 UTC
Working on that now... I'll use a GByteArray for storage but I guess that can always be changed later.

API will contain things to put all kinds of integer/float types in LE/BE, set a new position, get current size and return a new GstBuffer from it.
Comment 2 Sebastian Dröge (slomo) 2009-10-03 11:32:07 UTC
Created attachment 144661 [details] [review]
patch
Comment 3 Tim-Philipp Müller 2009-10-03 12:24:36 UTC
Nice, thanks for implementing this.

Random unconsidered comments/questions:

 - would it make sense for GstByteWriter to derive from GstByteReader, so that writers can also go back and read things, or is that not really needed?

 - would be nice to allow for the byte writer structure to be stack allocated as well

 - would be nice to allow for a fixed-size mode where the caller passes in a chunk of pre-allocated memory; the byte writer wouldn't be able to grow then of course, but since the functions already return TRUE/FALSE anyway, that should be easy to add

 - would it be more efficient to maintain current/end pointers so that you don't have to do start+current_pos for each write? (Does it matter? Do we care? Was just wondering the same thing about the byte reader the other day)

 - one could merge gst_byte_writer_take_data() and gst_byte_writer_free() g_string_free()-style (ie. with a do_free boolean and a return value). Not sure if it's prettier or more desirable, just mentioning it for completeness. It takes care of the fact that the byte writer will be unusable (or empty again) after using _take_data(), so avoids misunderstandings (Arguably, that should be kind of obvious though, so isn't really an issue)

 - _to_buffer() should be renamed/changed so that it's clearer what happens with the byte writer data IMHO. Maybe it should be _take_data_as_buffer()? (Unless we decide to go for byte_writer_free(), byte_writer_free_and_take_data(), and _free_and_take_buffer() or so).

 - how do we best use this when writing something like ASF or quicktime headers or RIFF chunks (or even the binary registry)? Ie. where we want to write chunks and nest them. Rely on higher API that then uses GstByteWriter? Or would it make sense to embed this somehow? (Not sure how or what would be needed, mabe a parent_chunk pointer or some such, and a hook to update the parent_chunk when a child chunk is finalised). So I guess the question is if we want to cater for this: if we do, then another layer of indirection might be needed (so that e.g. instead of growing the current writer you in fact grow and write to the memory allocation of the parent_chunk or somesuch).
Comment 4 Sebastian Dröge (slomo) 2009-10-03 12:34:12 UTC
(In reply to comment #3)
> Nice, thanks for implementing this.
> 
> Random unconsidered comments/questions:
> 
>  - would it make sense for GstByteWriter to derive from GstByteReader, so that
> writers can also go back and read things, or is that not really needed?

I don't think that's needed, usually you should know what you're writing :)

>  - would be nice to allow for the byte writer structure to be stack allocated
> as well

Right, I'll add that

>  - would be nice to allow for a fixed-size mode where the caller passes in a
> chunk of pre-allocated memory; the byte writer wouldn't be able to grow then of
> course, but since the functions already return TRUE/FALSE anyway, that should
> be easy to add

Ok, I'll add that too. A gst_byte_reader_new_fixed_size() would make sense in that regard too.

>  - would it be more efficient to maintain current/end pointers so that you
> don't have to do start+current_pos for each write? (Does it matter? Do we care?
> Was just wondering the same thing about the byte reader the other day)

I don't know :)

>  - one could merge gst_byte_writer_take_data() and gst_byte_writer_free()
> g_string_free()-style (ie. with a do_free boolean and a return value). Not sure
> if it's prettier or more desirable, just mentioning it for completeness. It
> takes care of the fact that the byte writer will be unusable (or empty again)
> after using _take_data(), so avoids misunderstandings (Arguably, that should be
> kind of obvious though, so isn't really an issue)
>
>  - _to_buffer() should be renamed/changed so that it's clearer what happens
> with the byte writer data IMHO. Maybe it should be _take_data_as_buffer()?
> (Unless we decide to go for byte_writer_free(),
> byte_writer_free_and_take_data(), and _free_and_take_buffer() or so).

I guess I'll go with _free(), _free_and_take_data() and _free_and_take_buffer() then.

>  - how do we best use this when writing something like ASF or quicktime headers
> or RIFF chunks (or even the binary registry)? Ie. where we want to write chunks
> and nest them. Rely on higher API that then uses GstByteWriter? Or would it
> make sense to embed this somehow? (Not sure how or what would be needed, mabe a
> parent_chunk pointer or some such, and a hook to update the parent_chunk when a
> child chunk is finalised). So I guess the question is if we want to cater for
> this: if we do, then another layer of indirection might be needed (so that e.g.
> instead of growing the current writer you in fact grow and write to the memory
> allocation of the parent_chunk or somesuch).

Could you give a real example how this API should look like and what exactly it should do and how it would be used? :)
Comment 5 Sebastian Dröge (slomo) 2009-10-03 14:35:23 UTC
Created attachment 144665 [details] [review]
bitreader/bytereader: API: Add gst_(bit|byte)_reader_get_size()

... and GST_(BYTE|BIT)_READER() casts.
Comment 6 Sebastian Dröge (slomo) 2009-10-03 14:35:32 UTC
Created attachment 144666 [details] [review]
bytewriter: Add a generic byte writer

Fixes bug #590669.
Comment 7 Sebastian Dröge (slomo) 2009-10-04 09:13:52 UTC
This still has some minor problems in the implementation and the API, new patches will follow later or tomorrow (and a start for a unit test).
Comment 8 Tim-Philipp Müller 2009-10-04 10:55:56 UTC
> #define GST_BYTE_WRITER(writer) ((GstByteWriter *) writer)

Not that I can really think of a plausible case where this would matter, but shouldn't it be (writer) ?


> GstByteWriter * gst_byte_writer_new_with_data (guint8 *data, guint size);
>
> #define GST_BYTE_WRITER_INIT_WITH_DATA (data, sized) \
>    { {data, size, 0}, TRUE, size }
> #define GST_BYTE_WRITER_INIT_WITH_BUFFER (buffer) \
>    GST_BYTE_WRITER_INIT_WITH_DATA (GST_BUFFER_DATA (buffer), GST_BUFFER_SIZE(buffer))

I thought of this API more as in "write to, but don't realloc or free the memory provided", not as in "GstByteWriter takes ownership of the memory passed".

I think we need a second flag to signal if the writer owns the memory, so it knows whether to free/return or _dup() it when freeing the writer.


> #define GST_BYTE_WRITER_INIT_WITH_SIZE (size, fixed) \
>    { {g_malloc (GST_BYTE_READER_MIN_SIZE), 0, 0}, fixed, GST_BYTE_READER_MIN_SIZE }

shouldn't that be 'size' here instead of _MIN_SIZE? I don't think _MIN_SIZE should be in the headers btw, the implementation should handle an initial value of 0 find and then re-alloc to MIN_SIZE internally as needed IMO. Not sure I really like the g_malloc here.

Not sure we really need any of the _INIT_WITH* macros, esp. if we have inlined init functions.


As for the nested chunk writing: might be better to do this with a separate API on top of GstByteWriter.
Comment 9 Sebastian Dröge (slomo) 2009-10-04 15:49:19 UTC
(In reply to comment #8)
> > #define GST_BYTE_WRITER(writer) ((GstByteWriter *) writer)
> 
> Not that I can really think of a plausible case where this would matter, but
> shouldn't it be (writer) ?

Yes, thanks
 
> > GstByteWriter * gst_byte_writer_new_with_data (guint8 *data, guint size);
> >
> > #define GST_BYTE_WRITER_INIT_WITH_DATA (data, sized) \
> >    { {data, size, 0}, TRUE, size }
> > #define GST_BYTE_WRITER_INIT_WITH_BUFFER (buffer) \
> >    GST_BYTE_WRITER_INIT_WITH_DATA (GST_BUFFER_DATA (buffer), GST_BUFFER_SIZE(buffer))
> 
> I thought of this API more as in "write to, but don't realloc or free the
> memory provided", not as in "GstByteWriter takes ownership of the memory
> passed".
> 
> I think we need a second flag to signal if the writer owns the memory, so it
> knows whether to free/return or _dup() it when freeing the writer.

Yes and another flag, if the memory is initialized (can be read) or not.

> > #define GST_BYTE_WRITER_INIT_WITH_SIZE (size, fixed) \
> >    { {g_malloc (GST_BYTE_READER_MIN_SIZE), 0, 0}, fixed, GST_BYTE_READER_MIN_SIZE }
> 
> shouldn't that be 'size' here instead of _MIN_SIZE? I don't think _MIN_SIZE
> should be in the headers btw, the implementation should handle an initial value
> of 0 find and then re-alloc to MIN_SIZE internally as needed IMO. Not sure I
> really like the g_malloc here.

Yes, fixed locally already :)

> Not sure we really need any of the _INIT_WITH* macros, esp. if we have inlined
> init functions.

Ok, I only had them because the reader had them too. I'll remove them then :)
Comment 10 Sebastian Dröge (slomo) 2009-10-04 15:59:07 UTC
Btw, another reason why we always need non-inlined functions for every useful piece of API are runtime created bindings, like seed (for JavaScript) or this new Python bindings based on gobject-introspection.
Comment 11 Sebastian Dröge (slomo) 2009-10-05 08:39:35 UTC
Created attachment 144765 [details] [review]
bytereader,bitreader: Remove FIXME 0.11 to remove non-inlined functions

The normal functions are always useful to have for bindings, especially
runtime-created bindings like Seed or new GObject-Introspection based
Python bindings.
Comment 12 Sebastian Dröge (slomo) 2009-10-05 08:39:48 UTC
Created attachment 144766 [details] [review]
bitreader/bytereader: API: Add gst_(bit|byte)_reader_get_size()

... and GST_(BYTE|BIT)_READER() casts.
Comment 13 Sebastian Dröge (slomo) 2009-10-05 08:40:00 UTC
Created attachment 144767 [details] [review]
bytewriter: Add a generic byte writer

Fixes bug #590669.
Comment 14 Sebastian Dröge (slomo) 2009-10-05 08:46:49 UTC
Created attachment 144768 [details] [review]
bytewriter: Add a generic byte writer

Fixes bug #590669.
Comment 15 Sebastian Dröge (slomo) 2009-10-05 09:23:22 UTC
Created attachment 144772 [details] [review]
bytewriter: Add a generic byte writer

Fixes bug #590669.
Comment 16 Sebastian Dröge (slomo) 2009-10-05 09:24:54 UTC
Created attachment 144773 [details] [review]
bytewriter: Add unit test
Comment 17 Tim-Philipp Müller 2009-10-05 09:38:04 UTC
> gst_byte_writer_ensure_writeable_area

I think elsewhere in GStreamer we use the spelling 'writable'. I wonder if there's a better name for this function as well, something that makes it clear that it works from the current write cursor position.


Also, one could use bitfields for the flags to save a few bytes (but maybe it's a bit silly, dunno).
Comment 18 Sebastian Dröge (slomo) 2009-10-05 09:54:53 UTC
Created attachment 144776 [details] [review]
bytewriter: Add a generic byte writer

Fixes bug #590669.
Comment 19 Sebastian Dröge (slomo) 2009-10-07 16:44:09 UTC
commit def3f57c4f51e536c1c33608adc086fed857d9e2
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Wed Oct 7 18:37:28 2009 +0200

    bytewriter: Add to the docs

commit 28c4bfc1c4bfa4e3b5a68e40e0ce739c9144ed92
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Mon Oct 5 11:24:35 2009 +0200

    bytewriter: Add unit test

commit 41f7a3fe09ce0f6a79bb336d6d3001579565426b
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Sat Oct 3 13:30:54 2009 +0200

    bytewriter: Add a generic byte writer
    
    Fixes bug #590669.