GNOME Bugzilla – Bug 707543
[API] add GstBitWriter
Last modified: 2018-05-06 18:25:25 UTC
Many component may need a bitwriter/reader in media framework. Gstreamer already have a GstBitReader but doesn't have GstBitWriter yet. Encoders need a bitwriter to generate bitstreams.
Created attachment 254162 [details] GstBitWriter header file
Created attachment 254163 [details] GstBitWriter implementation file
I'm trying to create a GstBitWriter for gstreamer and attached them here. https://bugzilla.gnome.org/attachment.cgi?id=254162 (gstbitwriter.h) This is the bitwriter header file. which defined some APIs. please help review and welcome for any input. I'm still working this bitwriter, next step is add more APIs similar with GstBitReader. https://bugzilla.gnome.org/attachment.cgi?id=254163 (gstbitwriter.c) It's the bitwriter implementation file. My first usage is for H.264 encoders. so I added write_ue and write_se here. Any one think whether ok adding it in bitwriter? If not, I will move to encoder files. This files implemented most use cases of the bitwriter. Welcome for any input on APIs and implementations. Thanks, Wind
Created attachment 259536 [details] [review] gstbitwriter.h New GstBitWritter interfaces
Created attachment 259537 [details] gstbitwriter.h New interface of GstBitWriter with _unchecked and _inline functions
Created attachment 259538 [details] gstbitwriter.c Implementation of GstBitWriter
https://bug707543.bugzilla-attachments.gnome.org/attachment.cgi?id=259537 (gstbitwriter.h) defined new interface of GstBitWritter, interfaces as below New interfaces * Create and destroy GstBitWriter gst_bit_writer_new gst_bit_writer_new_fill gst_bit_writer_free * Initialize and clear GstBitWriter gst_bit_writer_init gst_bit_writer_init_fill gst_bit_writer_clear * Functions with _unchecked and _inline in gstbitwriter.h gst_bit_writer_get_size gst_bit_writer_get_data gst_bit_writer_set_pos gst_bit_writer_get_space gst_bit_writer_put_bits_uint8 gst_bit_writer_put_bits_uint16 gst_bit_writer_put_bits_uint32 gst_bit_writer_put_bits_uint64 gst_bit_writer_put_bytes gst_bit_writer_align_bytes https://bug707543.bugzilla-attachments.gnome.org/attachment.cgi?id=259538 (gstbitwriter.c) implement parts of interfaces. Please help review.
Can you provide this as a patch against gstreamer (git format-patch style), and also add unit tests for it? For the put_bits() functions. Are bits put there in little or big endian order? And are the most or least significant bits of the value put?
(In reply to comment #8) > Can you provide this as a patch against gstreamer (git format-patch style), and > also add unit tests for it? yes, i'll change to a patch style of GstBitWriter. unit tests will add soon. > > For the put_bits() functions. Are bits put there in little or big endian order? GstBitWriter APIs are going to match GstBitReader, so all bits put are in big endian order. and it's most cases expected. > And are the most or least significant bits of the value put? if you mean which bits of the value need to put. It should be the least significant.
Created attachment 261389 [details] [review] 0001-bitwriter-add-GstBitWriter add patch of gstbitwrtier.h and gstbitwriter.c files
Created attachment 261390 [details] [review] 0002-bitwriter-add-gstbitwriter-docs add gstbitwriter-docs.h for documents.
Hi, there are some optimizations possible to make. The most important one would be to avoid the load from _gst_bit_writer_bit_filling_mask[], thus removing that static data. From what I understand, _gst_bit_writer_bit_filling_mask[nbits] reduces to (0xff >> (8 - nbits)). i.e. I'd prefer a computation instead of a load from memory, even if it is cached. Last but not least, prior to performing optimizations, proper unit testing are required. Thanks.
Does the the unit-tests and optimization proposed by Gwenole is enough to get these patches in?
I had a chat with Wind Yuan and it seems that he is busy now and not planning to work with this bug soon. If unit-tests are the only parts which is preventing to get this feature in, then I can write some basic unit-tests.
(In reply to comment #14) > I had a chat with Wind Yuan and it seems that he is busy now and not planning > to work with this bug soon. If unit-tests are the only parts which is > preventing to get this feature in, then I can write some basic unit-tests. Hi, Sorry I can't finish the unit test since some busy works in hand. I'm very appreciate for your efforts to work on the left issues. Thanks, Wind
Created attachment 272276 [details] [review] some basic unit-tests for the bitwriter
The optimizations can be done later. Please let me know if other changes needed in the bitwriter implementation.
I will let slomo comment more on the APIs, maybe we'd need to rename a few functions and remove a couple of (unused) interfaces along the way to make it more in-line with the rest of GStreamer APIs. Proposed removals: - gst_bit_writer_set_pos(): unused, an unpracticable if the GstBitWriter memory was wrapped from user memory. i.e. this wouldn't work in this case. Proposed name changes: - gst_bit_writer_init/new(): single variant. If data is NULL, then allocation is maintained by GstBitWriter - gst_bit_writer_get_remaining(): instead of _get_space() That's only what immediately comes out of my mind. Other comments appreciated.
I think it would be good if the new/init and free/clear functions matched the variants and naming in GstByteWriter.
(In reply to comment #18) > Proposed removals: > - gst_bit_writer_set_pos(): unused, an unpracticable if the GstBitWriter memory > was wrapped from user memory. i.e. this wouldn't work in this case. > What is the real issue to have a set_pos() ? It seems that all byte_writer/byte_reader/bit_reader interfaces have the similar API.
(In reply to comment #20) > (In reply to comment #18) > > Proposed removals: > > - gst_bit_writer_set_pos(): unused, an unpracticable if the GstBitWriter memory > > was wrapped from user memory. i.e. this wouldn't work in this case. > > > What is the real issue to have a set_pos() ? It seems that all > byte_writer/byte_reader/bit_reader interfaces have the similar API. Sometime, users want to write bytes like: bytes_of_body_length(4 bytes) + bytes_of_body. before the body was written, not sure the exact length of the body. may just write bytes of '0' as length. after body done, get the length and _set_pos to body header to write the body_length.
Some comments/questions: - should we derive bitwriter from bitreader like we do with bytewriter? - gst_bit_writer_new (guint32 reserved_bits) --> new_with_size (guint32 reserved_bits) - gst_bit_writer_new_fill (guint8 * data, guint bits) --> _new_with_data (guint8 * data, guint bits, gboolean initialized), but the 'initialized' parameter only makes sense if we can/want to make the writer derive from the reader somehow (see above) - gst_bit_writer_free (GstBitWriter * writer, gboolean free_data); -> _free (bw); also add _reset(), _reset_and_get_data(), _reset_and_get_buffer(), _free_and_get_data(), _free_and_get_buffer() ? - _init() -> _init_with_size(), + plain _init() - _init_fill() -> init_with_data(); - gst_bit_writer_clear() -> _reset*() ? (more consistent with ByteWriter, even though we use _clear() elsewhere - gst_bit_writer_set_pos(): gtk-doc chunk has wrong header - gst_bit_writer_get_space() -> get_remaining() to match ByteWriter? - gst_bit_writer_put_bits_uint8|uint16|uint32|uint64 - do we need all these variants, vs. just _put_bits (guint bits, guint nbits) + perhaps put_bits64() ? - gst_bit_writer_put_bytes() -> put_data() perhaps? (maybe also with guint nbits rather than nbytes?) I can help make the changes once we agree what it should look like.
From the API point of view, I agree with Tim's comments here. Nothing else to mention about that. Deriving bitwriter from bitreader would make sense too. Any reasons not to do that?
I have no objection, but more re-write...planning for 1.8?
I worked on these patches a couple weeks ago. I have finished the API changes requested by Tim. Also fixed the gtk-doc generation, meson support, and other minor things. I'll upload the patches for your review. I could try to derive it from bitwriter for coherence.
Created attachment 371708 [details] [review] bitwriter: add GstBitWriter Add new files for bit writer libs/gst/base/gstbitwriter.c libs/gst/base/gstbitwriter.h APIs * Create and destroy GstBitWriter gst_bit_writer_new gst_bit_writer_new_fill gst_bit_writer_free * Initialize and clear GstBitWriter gst_bit_writer_init gst_bit_writer_init_fill gst_bit_writer_clear * Functions gst_bit_writer_get_size gst_bit_writer_get_data gst_bit_writer_set_pos gst_bit_writer_get_space gst_bit_writer_put_bits_uint8 gst_bit_writer_put_bits_uint16 gst_bit_writer_put_bits_uint32 gst_bit_writer_put_bits_uint64 gst_bit_writer_put_bytes gst_bit_writer_align_bytes
Created attachment 371709 [details] [review] bitwriter: add gstbitwriter-docs Docs for unchecked APIs gst_bit_writer_put_bits_uint8_unchecked gst_bit_writer_put_bits_uint16_unchecked gst_bit_writer_put_bits_uint32_unchecked gst_bit_writer_put_bits_uint64_unchecked gst_bit_writer_put_bytes_unchecked gst_bit_writer_align_bytes_unchecked
Created attachment 371710 [details] [review] Add unit tests for bitwriter
Cool, so now we only have to decide if we want this to derive from bitreader or not. I don't know how important that is in practice, I'm inclined to say let's not bother.
(In reply to Tim-Philipp Müller from comment #29) > Cool, so now we only have to decide if we want this to derive from bitreader > or not. > > I don't know how important that is in practice, I'm inclined to say let's > not bother. Agree. Just I realized that the commit messages are useless. I'll re-write them. Another question would be: should I squash these three patches in a single one, adding a field Authors: in the commit log?
Created attachment 371719 [details] [review] bitwriter: Add a generic bit writer GstBitWriter provides a bit writer that can write any number of bits into a memory buffer. It provides functions for writing any number of bits into 8, 16, 32 and 64 bit variables.
Created attachment 371720 [details] [review] Add unit tests for bitwriter
Created attachment 371750 [details] [review] bitwriter: Add a generic bit writer GstBitWriter provides a bit writer that can write any number of bits into a memory buffer. It provides functions for writing any number of bits into 8, 16, 32 and 64 bit variables.
Created attachment 371751 [details] [review] bitwriter: Add unit tests
Attachment 371750 [details] pushed as fb4fc8f - bitwriter: Add a generic bit writer Attachment 371751 [details] pushed as 9ad6340 - bitwriter: Add unit tests
(In reply to Víctor Manuel Jáquez Leal from comment #35) > Attachment 371750 [details] pushed as fb4fc8f - bitwriter: Add a generic bit > writer > Attachment 371751 [details] pushed as 9ad6340 - bitwriter: Add unit tests Finally, it is landed! Great!