Add support for standard V4L2 FM deemphasis control

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

unspecified
mozilla33
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #8453686 - Flags: review?(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.
Attachment #8453686 - Flags: review?(dhylands) → review+
(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.
(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
Depends on: 1037646
Comment added. MOZ_CRASH is still here because I'm going to fix the issue in bug 1037646 before landing this.
Attachment #8453686 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f8c2c10aa168
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.