1036873 - Add support for standard V4L2 FM deemphasis control

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Michael Wu [:mwu]]

Attachment: Add support for standard V4L2 FM deemphasis control

V4L2 has support for configuring deemphasis. This patch makes our hal code take advantage of that control. Unfortunately, our kernel headers from bionic are not new enough to define this new control.

Comment 1

[Dave Hylands [:dhylands]]

Comment on attachment 8453686 [details] [diff] [review]
Add support for standard V4L2 FM deemphasis control

Review of attachment 8453686 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Just a couple of minor things I commented on.

r=me with those addressed.

::: hal/gonk/GonkFMRadio.cpp
@@ +26,5 @@
>  #include <stdlib.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
>  
> +#ifndef V4L2_CTRL_CLASS_FM_RX

These are the constants that you added due to our headers being too old right?

Could you just add a comment that says that (what you said in the bug description is good - I'd just like to see that in the source here.

@@ +329,5 @@
>    }
>  
> +  int emphasis;
> +  switch (aInfo.preEmphasis()) {
> +  case 0:

nit: case should be indented 2 from the switch according to the coding standars.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

@@ +339,5 @@
> +  case 75:
> +    emphasis = V4L2_DEEMPHASIS_75_uS;
> +    break;
> +  default:
> +    MOZ_CRASH("Invalid preemphasis setting");

Is this something that can originate in any way from the JS side? If so, a crash seems rather severe.

If its purely internal, then the crash is fine.

Comment 2

[Michael Wu [:mwu]]

(In reply to Dave Hylands [:dhylands] from comment #1)
> Comment on attachment 8453686 [details] [diff] [review]
> Add support for standard V4L2 FM deemphasis control
> 
> Review of attachment 8453686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Just a couple of minor things I commented on.
> 
> r=me with those addressed.
> 
> ::: hal/gonk/GonkFMRadio.cpp
> @@ +26,5 @@
> >  #include <stdlib.h>
> >  #include <sys/stat.h>
> >  #include <sys/types.h>
> >  
> > +#ifndef V4L2_CTRL_CLASS_FM_RX
> 
> These are the constants that you added due to our headers being too old
> right?
> 
> Could you just add a comment that says that (what you said in the bug
> description is good - I'd just like to see that in the source here.
> 

Will do.

> @@ +329,5 @@
> >    }
> >  
> > +  int emphasis;
> > +  switch (aInfo.preEmphasis()) {
> > +  case 0:
> 
> nit: case should be indented 2 from the switch according to the coding
> standars.
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#Control_Structures
> 

Oh. This file is already using no indenting for case though.

> @@ +339,5 @@
> > +  case 75:
> > +    emphasis = V4L2_DEEMPHASIS_75_uS;
> > +    break;
> > +  default:
> > +    MOZ_CRASH("Invalid preemphasis setting");
> 
> Is this something that can originate in any way from the JS side? If so, a
> crash seems rather severe.
> 
> If its purely internal, then the crash is fine.


The JS api doesn't have access to these parameters.

Unfortunately, I just realized that nothing on the dom side is setting this properly.. looks like the infrastructure for everything was setup but nobody actually hooked it up. There's also no default value for this AFAICT, so this would actually crash.

I'll take out this MOZ_CRASH until that gets resolved.

Comment 3

[Dave Hylands [:dhylands]]

(In reply to Michael Wu [:mwu] from comment #2)
> (In reply to Dave Hylands [:dhylands] from comment #1)

> > @@ +329,5 @@
> > >    }
> > >  
> > > +  int emphasis;
> > > +  switch (aInfo.preEmphasis()) {
> > > +  case 0:
> > 
> > nit: case should be indented 2 from the switch according to the coding
> > standars.
> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> > Coding_Style#Control_Structures
> > 
> 
> Oh. This file is already using no indenting for case though.

ok - I didn't see that. Consistency is more important

Comment 4

[Michael Wu [:mwu]]

Attachment: Add support for standard V4L2 FM deemphasis control, v2

Comment added. MOZ_CRASH is still here because I'm going to fix the issue in bug 1037646 before landing this.

Comment 5

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f8c2c10aa168

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f8c2c10aa168