[Spice-devel,v16,03/23] mjpeg: Use src_area as the authoritative source for the frame dimensions

Submitted by Francois Gouget on June 7, 2016, 1:58 p.m.

Details

Message ID fc04185ae3704db2f4aa8671795a8411480afa28.1465306176.git.fgouget@free.fr
State New
Headers show
Series "Add GStreamer support for video streaming" ( rev: 22 21 20 19 18 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget June 7, 2016, 1:58 p.m.
Video frames correspond to QXL_DRAW_COPY operations where the frame area
is defined by the SpiceCopy.src_area field.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---
 server/mjpeg-encoder.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index e3646db..7dcea50 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -706,7 +706,7 @@  static void mjpeg_encoder_adjust_fps(MJpegEncoder *encoder, uint64_t now)
  */
 static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
                                      SpiceBitmapFmt format,
-                                     int width, int height,
+                                     const SpiceRect *src,
                                      uint8_t **dest, size_t *dest_len,
                                      uint32_t frame_mm_time)
 {
@@ -777,10 +777,12 @@  static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
         return VIDEO_ENCODER_FRAME_UNSUPPORTED;
     }
 
+    encoder->cinfo.image_width = src->right - src->left;
+    encoder->cinfo.image_height = src->bottom - src->top;
     if (encoder->pixel_converter != NULL) {
-        unsigned int stride = width * 3;
+        JDIMENSION stride = encoder->cinfo.image_width * 3;
         /* check for integer overflow */
-        if (stride < width) {
+        if (stride < encoder->cinfo.image_width) {
             return VIDEO_ENCODER_FRAME_UNSUPPORTED;
         }
         if (encoder->row_size < stride) {
@@ -790,9 +792,6 @@  static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
     }
 
     spice_jpeg_mem_dest(&encoder->cinfo, dest, dest_len);
-
-    encoder->cinfo.image_width      = width;
-    encoder->cinfo.image_height     = height;
     jpeg_set_defaults(&encoder->cinfo);
     encoder->cinfo.dct_method       = JDCT_IFAST;
     quality = mjpeg_quality_samples[encoder->rate_control.quality_id];
@@ -935,8 +934,7 @@  static int mjpeg_encoder_encode_frame(VideoEncoder *video_encoder,
 {
     MJpegEncoder *encoder = (MJpegEncoder*)video_encoder;
 
-    int ret = mjpeg_encoder_start_frame(encoder, bitmap->format,
-                                        width, height,
+    int ret = mjpeg_encoder_start_frame(encoder, bitmap->format, src,
                                         outbuf, outbuf_size,
                                         frame_mm_time);
     if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) {

Comments

Hi Francois,

On Tue, 2016-06-07 at 15:58 +0200, Francois Gouget wrote:
> Video frames correspond to QXL_DRAW_COPY operations where the frame area
> is defined by the SpiceCopy.src_area field.

Do you think it can have an effect on the bug:
 https://bugs.freedesktop.org/show_bug.cgi?id=94372

Thanks,
Pavel

> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  server/mjpeg-encoder.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> index e3646db..7dcea50 100644
> --- a/server/mjpeg-encoder.c
> +++ b/server/mjpeg-encoder.c
> @@ -706,7 +706,7 @@ static void mjpeg_encoder_adjust_fps(MJpegEncoder
> *encoder, uint64_t now)
>   */
>  static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
>                                       SpiceBitmapFmt format,
> -                                     int width, int height,
> +                                     const SpiceRect *src,
>                                       uint8_t **dest, size_t *dest_len,
>                                       uint32_t frame_mm_time)
>  {
> @@ -777,10 +777,12 @@ static int mjpeg_encoder_start_frame(MJpegEncoder
> *encoder,
>          return VIDEO_ENCODER_FRAME_UNSUPPORTED;
>      }
>  
> +    encoder->cinfo.image_width = src->right - src->left;
> +    encoder->cinfo.image_height = src->bottom - src->top;
>      if (encoder->pixel_converter != NULL) {
> -        unsigned int stride = width * 3;
> +        JDIMENSION stride = encoder->cinfo.image_width * 3;
>          /* check for integer overflow */
> -        if (stride < width) {
> +        if (stride < encoder->cinfo.image_width) {
>              return VIDEO_ENCODER_FRAME_UNSUPPORTED;
>          }
>          if (encoder->row_size < stride) {
> @@ -790,9 +792,6 @@ static int mjpeg_encoder_start_frame(MJpegEncoder
> *encoder,
>      }
>  
>      spice_jpeg_mem_dest(&encoder->cinfo, dest, dest_len);
> -
> -    encoder->cinfo.image_width      = width;
> -    encoder->cinfo.image_height     = height;
>      jpeg_set_defaults(&encoder->cinfo);
>      encoder->cinfo.dct_method       = JDCT_IFAST;
>      quality = mjpeg_quality_samples[encoder->rate_control.quality_id];
> @@ -935,8 +934,7 @@ static int mjpeg_encoder_encode_frame(VideoEncoder
> *video_encoder,
>  {
>      MJpegEncoder *encoder = (MJpegEncoder*)video_encoder;
>  
> -    int ret = mjpeg_encoder_start_frame(encoder, bitmap->format,
> -                                        width, height,
> +    int ret = mjpeg_encoder_start_frame(encoder, bitmap->format, src,
>                                          outbuf, outbuf_size,
>                                          frame_mm_time);
>      if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) {
On Wed, 8 Jun 2016, Pavel Grunt wrote:

> Hi Francois,
> 
> On Tue, 2016-06-07 at 15:58 +0200, Francois Gouget wrote:
> > Video frames correspond to QXL_DRAW_COPY operations where the frame area
> > is defined by the SpiceCopy.src_area field.
> 
> Do you think it can have an effect on the bug:
>  https://bugs.freedesktop.org/show_bug.cgi?id=94372

Maybe. You should try.
On Thu, 2016-06-09 at 07:51 +0200, Francois Gouget wrote:
> On Wed, 8 Jun 2016, Pavel Grunt wrote:
> 
> > Hi Francois,
> > 
> > On Tue, 2016-06-07 at 15:58 +0200, Francois Gouget wrote:
> > > Video frames correspond to QXL_DRAW_COPY operations where the frame area
> > > is defined by the SpiceCopy.src_area field.
> > 
> > Do you think it can have an effect on the bug:
> >  https://bugs.freedesktop.org/show_bug.cgi?id=94372
> 
> Maybe. You should try.
> 

ok, I just wanted to know if it fixes some issue or just simplify the code

Pavel
On Thu, 9 Jun 2016, Pavel Grunt wrote:

> On Thu, 2016-06-09 at 07:51 +0200, Francois Gouget wrote:
> > On Wed, 8 Jun 2016, Pavel Grunt wrote:
> > 
> > > Hi Francois,
> > > 
> > > On Tue, 2016-06-07 at 15:58 +0200, Francois Gouget wrote:
> > > > Video frames correspond to QXL_DRAW_COPY operations where the frame area
> > > > is defined by the SpiceCopy.src_area field.
> > > 
> > > Do you think it can have an effect on the bug:
> > >  https://bugs.freedesktop.org/show_bug.cgi?id=94372
> > 
> > Maybe. You should try.
> > 
> 
> ok, I just wanted to know if it fixes some issue or just simplify the code

The part that may fix things is patch 01. This one just simplifies the 
code.