Re: [PATCH]: Add large files support.

[Jens Georg <mail jensge org>]

Hi,

thanks for this patch, makes completely sense :) For the future, please
file patches in bugzilla because we have a nice review infrastructure
there :)

One question though: Why are you using the Posix stuff and not the GLib
file functions?

The simple engine was more meant as a demo implementation. If you want
to use it in production, please note that there is a race condition in
the memory hand-over from streaming to the main thread where sometimes
the memory is freed early.

> Hi Rygel Members,
>
> I've tested Rygel media server with success, thank you for this project.
> I would like to submit a patch regarding large files support in rygel 
> media server.
> The major part of this patch is done inside rygel-simple-data-source 
> where we try to mmap a large file in the process virtual address space 
> (4GB).
> As we can't mmap file > 4GB, this patch replaces mmap with simple read 
> syscall.
>
>  From 81a2fbbd077f92289f3cc5b263246676b6e788be Mon Sep 17 00:00:00 2001
> From: Jean-Baptiste Dubois <jean-baptiste dubois parrot com>
> Date: Mon, 7 Oct 2013 09:49:01 +0200
> Subject: [PATCH]: Add large files (> 4GB) streaming support.
>
> ---
>   configure.ac                                       |    2 +
>   src/media-engines/simple/Makefile.am               |    1 +
>   .../simple/rygel-simple-data-source.vala           |   39 
> +++++++++++++++-----
>   3 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 3e4982e..a460a80 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -27,6 +27,8 @@ dnl Disable generation of static libraries
>   LT_PREREQ([2.2.6])
>   LT_INIT([dlopen disable-static])
>
> +AC_SYS_LARGEFILE
> +
>   dnl Required versions of library packages
>   dnl Not all of these are actually used, depending on the configure 
> options.
>   GLIB_REQUIRED=2.31.13
> diff --git a/src/media-engines/simple/Makefile.am 
> b/src/media-engines/simple/Makefile.am
> index 3a6aa8e..67b3dfb 100644
> --- a/src/media-engines/simple/Makefile.am
> +++ b/src/media-engines/simple/Makefile.am
> @@ -8,6 +8,7 @@ librygel_media_engine_simple_la_SOURCES = \
>       rygel-simple-data-source.vala
>
>   librygel_media_engine_simple_la_VALAFLAGS = \
> +    --pkg posix \
>       $(RYGEL_COMMON_LIBRYGEL_SERVER_VALAFLAGS) \
>       $(RYGEL_COMMON_VALAFLAGS)
>
> diff --git a/src/media-engines/simple/rygel-simple-data-source.vala 
> b/src/media-engines/simple/rygel-simple-data-source.vala
> index d0ccbd4..e4b8cf5 100644
> --- a/src/media-engines/simple/rygel-simple-data-source.vala
> +++ b/src/media-engines/simple/rygel-simple-data-source.vala
> @@ -34,8 +34,8 @@ internal class Rygel.SimpleDataSource : DataSource, 
> Object {
>       private Thread<void*> thread;
>       private Mutex mutex = Mutex ();
>       private Cond cond = Cond ();
> -    private uint64 first_byte = 0;
> -    private uint64 last_byte = 0;
> +    private Posix.off_t first_byte = 0;
> +    private Posix.off_t last_byte = 0;
>       private bool frozen = false;
>       private bool stop_thread = false;
>       private HTTPSeek offsets = null;
> @@ -103,13 +103,25 @@ internal class Rygel.SimpleDataSource : 
> DataSource, Object {
>       private void* thread_func () {
>           var file = File.new_for_commandline_arg (this.uri);
>           debug ("Spawning new thread for streaming file %s", this.uri);
> +        int fd = -1;
>           try {
> -            var mapped = new MappedFile (file.get_path (), false);
> +            fd = Posix.open (file.get_path (), Posix.O_RDONLY, 0);
> +            if (fd < 0)
> +                throw new IOError.FAILED("Failed to open file '%s': %s",
> +                                         file.get_path (),
> + Posix.strerror(Posix.errno));
> +
>               if (this.offsets != null) {
> -                this.first_byte = this.offsets.start;
> -                this.last_byte = this.offsets.stop + 1;
> +                this.first_byte = (Posix.off_t)this.offsets.start;
> +                this.last_byte = (Posix.off_t)this.offsets.stop + 1;
>               } else {
> -                this.last_byte = mapped.get_length ();
> +                this.first_byte = 0;
> +                this.last_byte = Posix.lseek (fd, 0, Posix.SEEK_END);
> +                Posix.lseek(fd, 0, Posix.SEEK_SET);
> +            }
> +
> +            if (this.first_byte != 0) {
> +                 Posix.lseek(fd, this.first_byte, Posix.SEEK_SET);
>               }
>
>               while (true) {
> @@ -134,9 +146,14 @@ internal class Rygel.SimpleDataSource : DataSource, 
> Object {
>                       stop = this.last_byte;
>                   }
>
> -                unowned uint8[] data = (uint8[]) mapped.get_contents ();
> -                data.length = (int) mapped.get_length ();
> -                uint8[] slice = data[start:stop];
> +                var slice = new uint8[stop - start];
> +                var len = (int)Posix.read(fd, slice, slice.length);
> +                if (len < 0)
> +                    throw new IOError.FAILED("Failed to read file '%s': 
> %s",
> +                                             file.get_path (),
> + Posix.strerror(Posix.errno));
> +
> +                slice.length = len;
>                   this.first_byte = stop;
>
>                   // There's a potential race condition here.
> @@ -149,7 +166,9 @@ internal class Rygel.SimpleDataSource : DataSource, 
> Object {
>                   });
>               }
>           } catch (Error error) {
> -            warning ("Failed to map file: %s", error.message);
> +            warning ("Failed to stream file: %s", error.message);
> +        } finally {
> +            Posix.close(fd);
>           }
>
>           // Signal that we're done streaming
> _______________________________________________
> rygel-list mailing list
> rygel-list gnome org
> https://mail.gnome.org/mailman/listinfo/rygel-list